GeoTools

ContentFeatureStore fixes

Details

  • Type: Bug Bug
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Fixed
  • Affects Version/s: 8.0-M0
  • Fix Version/s: 8.0-M1
  • Component/s: data
  • Labels:
    None

Description

These three fixes need to be made to ContentFeatureStore to support CSV datastore:

1. removeFeatures() has this loop:

            //remove everything
            while( writer.hasNext() ) {
                writer.next();
                writer.remove();
                writer.write();
            }

but it really should look like this:

            //remove everything
            while( writer.hasNext() ) {
                writer.next();
                writer.remove();
            }

The writer.write() should not be there.

2. ContentFeatureStore.getWriter(Query, int) needs to have (at least) a FilteringWriter. See attached patch.

3. There's a minor tweak in ContentFeatureStore.getWriter(Filter, int). Instead of "new DefaultQuery(..." it should be "new Query(.." – DefaultQuery is deprecated.

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Justin Deoliveira added a comment -
The changes all look good to me.
Show
Justin Deoliveira added a comment - The changes all look good to me.
Hide
Permalink
Andrea Aime added a comment -
Not sure if Lee has commit access. Lee, do you?
Show
Andrea Aime added a comment - Not sure if Lee has commit access. Lee, do you?
Hide
Permalink
Lee Breisacher added a comment -
I have commit access just for the CSV (unsupported) project. I'm a newbie to geotools.
Show
Lee Breisacher added a comment - I have commit access just for the CSV (unsupported) project. I'm a newbie to geotools.
Hide
Permalink
Andrea Aime added a comment -
Lee, roger that. Justin, who maintains those classes, said the patch was ok, I would interpret it as a "go ahead an commit it" but I may be wrong.
Justin, can you clarify?
Show
Andrea Aime added a comment - Lee, roger that. Justin, who maintains those classes, said the patch was ok, I would interpret it as a "go ahead an commit it" but I may be wrong. Justin, can you clarify?
Hide
Permalink
Jody Garnett added a comment -
I actually maintain those classes; but wanted justin's viewpoint as so much of jdbc depends on them. Commit applied as of -r37262.

Thanks for submitting as a Jira Lee.
Show
Jody Garnett added a comment - I actually maintain those classes; but wanted justin's viewpoint as so much of jdbc depends on them. Commit applied as of -r37262. Thanks for submitting as a Jira Lee.
Hide
Permalink
Justin Deoliveira added a comment -
Well when we found those classes they really were just a proof of concept and the jdbc datastores took them to something that actually worked. They now form the base for our most robust set of datastores, and are also the new official starting point for new datastores. So I dont; expect major changes to be made to them without giving myself or andrea a chance to review. So let's say we are co-maintaining them.
Show
Justin Deoliveira added a comment - Well when we found those classes they really were just a proof of concept and the jdbc datastores took them to something that actually worked. They now form the base for our most robust set of datastores, and are also the new official starting point for new datastores. So I dont; expect major changes to be made to them without giving myself or andrea a chance to review. So let's say we are co-maintaining them.
Hide
Permalink
Jody Garnett added a comment -
Yep (and I did send a warning to the email list that you should expect some instability, and we created this issue to give you a chance to review :-D ).
Show
Jody Garnett added a comment - Yep (and I did send a warning to the email list that you should expect some instability, and we created this issue to give you a chance to review :-D ).
Hide
Permalink
Justin Deoliveira added a comment -
Instability? This looks like a simple patch to me. What else is coming? To be honest you send so many emails that I simply don't have enough time to read over all of them. All I can do is watch jira and review issues that people assign to me or add me as a watcher. If you plan on adding "instability" we'll have to fork the current ContentDataStore.
Show
Justin Deoliveira added a comment - Instability? This looks like a simple patch to me. What else is coming? To be honest you send so many emails that I simply don't have enough time to read over all of them. All I can do is watch jira and review issues that people assign to me or add me as a watcher. If you plan on adding "instability" we'll have to fork the current ContentDataStore.
Hide
Permalink
Jody Garnett added a comment -
To be honest I don't know what is coming; this is the first time we are writing a non JDBC FeatureStore; I imagine a few more controlled wrapper such as provided in this patch. At the very least for TransactionStateDiff (as that is not something JDBC uses). I am looking for collaboration here Justin; Lee and I are here as volunteers, as are you, this is a background task welcoming Lee to the code base as it were :-)

So for any changes to ContentDataStore we will be submitting Jiras and asking you to check.
Show
Jody Garnett added a comment - To be honest I don't know what is coming; this is the first time we are writing a non JDBC FeatureStore; I imagine a few more controlled wrapper such as provided in this patch. At the very least for TransactionStateDiff (as that is not something JDBC uses). I am looking for collaboration here Justin; Lee and I are here as volunteers, as are you, this is a background task welcoming Lee to the code base as it were :-) So for any changes to ContentDataStore we will be submitting Jiras and asking you to check.
Hide
Permalink
Justin Deoliveira added a comment -
Sounds good. Sorry I came off as harsh... just started to sound like you were going to run wild in that module and i got thrown off. The work is appreciated and I look forward to reviewing future patches.
Show
Justin Deoliveira added a comment - Sounds good. Sorry I came off as harsh... just started to sound like you were going to run wild in that module and i got thrown off. The work is appreciated and I look forward to reviewing future patches.

People

  • Assignee:
    Justin Deoliveira
    Reporter:
    Lee Breisacher
Vote (0)
Watch (0)

Dates

  • Created:
    Updated:
    Resolved: