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

        Hide
        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
        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
        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
        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
        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
        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
        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
        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
        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
        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
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: