GeoTools
  1. GeoTools
  2. GEOT-2782

IndexedShapefileDataStore Query Bug

    Details

    • Type: Bug Bug
    • Status: Closed Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 2.6.0
    • Component/s: shapefile plugin
    • Labels:
      None
    • Environment:
      Windows XP, Java 1.5.0_16
    • Testcase included:
      yes

      Description

      If I query an IndexedShapefileDataStore for a subset of attributes and no geometry, the first attribute in the resulting feature collection is always the wkt version of the geometry. This does not happen if I use a ShapefileDataStore.

      For example in this code:

      IndexedShapefileDataStore indexedstore = new IndexedShapefileDataStore(TestData.url(STATEPOP));
      final Query query = new DefaultQuery(indexedstore.getSchema().getName().getLocalPart(), idFilter, new String[]

      { "STATE_NAME"}

      );
      final FeatureCollection<SimpleFeatureType, SimpleFeature> indexedfeatures = indexedstore.getFeatureSource().getFeatures(query);

      indexedfeatures.iterator().next().getAttribute("STATE_NAME") returns 'POLYGON((......))' which is not correct.

      I've attached a test case that provides an example of this issue.

      This bug is producing invalid results in the uDig Table View. The first attribute column is always displaying the geometry wkt instead of the attribute.

      1. reindex.patch
        6 kB
        Jody Garnett
      2. ShapefileAttributeReaderTest.java
        2 kB
        Emily Gouge

        Activity

        Hide
        Andrea Aime added a comment -
        I'll be flying to Sydney for FOSS4G shortly, then participate to FOSS4G, then one week vacation... Not sure I'll have any time to look into this soon.
        Show
        Andrea Aime added a comment - I'll be flying to Sydney for FOSS4G shortly, then participate to FOSS4G, then one week vacation... Not sure I'll have any time to look into this soon.
        Hide
        Jody Garnett added a comment -
        This is one of two down checks for the release candidate for the uDig team.
        Show
        Jody Garnett added a comment - This is one of two down checks for the release candidate for the uDig team.
        Hide
        Jody Garnett added a comment -
        In tracing through it looks like IndexShapefileAttributeReader is not being wrapped up in a ReType ... I suspect it wants to do everything itself for speed?

        So two solutions:
        - layer ReTypeFeatureReader around IndexedShapefileAttributeReader; or
        - The FDFeatureReader ends up calling read( 0 ); and the ShapeFileAttributeReader has a hard coded check so that 0 always returns the geometry.

        Jody

        Notes taken while debugging
        ----------------------------------------------------------
        FeatureReaderIterator - > FIDFeatureReader -> IndexedShapefileAttributeReader

        With IndexedShapefileAttributeReader holding on to:
        - IndexedDbaseFileReader
        - ShapefileReader


        Compare with ....

        FeatureReaderIterator -> ReTypeFeatureReader -> FilteringFeatureReader -> FIDFeatureReader -> ShapefileAttributeReader

        WIth FIDFeatureReader holding on to:
        - DefaultFIDReader
        - ShapefileAttributeReader
        -- DbaseFileReader
        -- ShapefileReader

        implement IndexedShapefileAttributeReader.read( int param ) with more logic so case 0 is not always geometry

        The current implementation of ShapefileAttributeReader.read is as follows:

           public Object read(int param) throws IOException,
                   java.lang.ArrayIndexOutOfBoundsException {
               switch (param) {
               case 0:
                   return geometry;

               default:
                   if (row != null) {
                       return row.read(dbfindexes[param]);
                   }
                   return null;
             }

        IndexedShapefileDataStore creates one of readers and has the complete query to pass in.
        Show
        Jody Garnett added a comment - In tracing through it looks like IndexShapefileAttributeReader is not being wrapped up in a ReType ... I suspect it wants to do everything itself for speed? So two solutions: - layer ReTypeFeatureReader around IndexedShapefileAttributeReader; or - The FDFeatureReader ends up calling read( 0 ); and the ShapeFileAttributeReader has a hard coded check so that 0 always returns the geometry. Jody Notes taken while debugging ---------------------------------------------------------- FeatureReaderIterator - > FIDFeatureReader -> IndexedShapefileAttributeReader With IndexedShapefileAttributeReader holding on to: - IndexedDbaseFileReader - ShapefileReader Compare with .... FeatureReaderIterator -> ReTypeFeatureReader -> FilteringFeatureReader -> FIDFeatureReader -> ShapefileAttributeReader WIth FIDFeatureReader holding on to: - DefaultFIDReader - ShapefileAttributeReader -- DbaseFileReader -- ShapefileReader implement IndexedShapefileAttributeReader.read( int param ) with more logic so case 0 is not always geometry The current implementation of ShapefileAttributeReader.read is as follows:    public Object read(int param) throws IOException,            java.lang.ArrayIndexOutOfBoundsException {        switch (param) {        case 0:            return geometry;        default:            if (row != null) {                return row.read(dbfindexes[param]);            }            return null;      } IndexedShapefileDataStore creates one of readers and has the complete query to pass in.
        Hide
        Jody Garnett added a comment - - edited
        Okay I think I got a strategy - assign all the attributes (including geometry) a dbf index. Yes you could consider "0" as the indicating that (and the 1st implementor did). Since IndexedShapefile is actually doing everything itself we can no longer assume 0 is a good marker value.

        Changing this to -1; and making the switch statement based on the reindexed value seems to fix the problem (and break many other test cases).

        Looking into the details; it is only the writer test cases are busted. It feels like this is the 1/2 way point - can continue pushing in this direction; or back back and try and insert ReType (and take the performance hit).
        Show
        Jody Garnett added a comment - - edited Okay I think I got a strategy - assign all the attributes (including geometry) a dbf index. Yes you could consider "0" as the indicating that (and the 1st implementor did). Since IndexedShapefile is actually doing everything itself we can no longer assume 0 is a good marker value. Changing this to -1; and making the switch statement based on the reindexed value seems to fix the problem (and break many other test cases). Looking into the details; it is only the writer test cases are busted. It feels like this is the 1/2 way point - can continue pushing in this direction; or back back and try and insert ReType (and take the performance hit).
        Hide
        Jody Garnett added a comment -
        I ran this patch in uDig and it *does* fix the problem. I will try and talk this one through with Andrea tomorrow and see what a good direction is.
        Show
        Jody Garnett added a comment - I ran this patch in uDig and it *does* fix the problem. I will try and talk this one through with Andrea tomorrow and see what a good direction is.
        Hide
        Andrea Aime added a comment -
        I guess the retyper has to be introduced anyways since one might filter on top of the geometry attribute without requiring it among the extracted properties ("give me the names of the countries touching the french borders").
        Show
        Andrea Aime added a comment - I guess the retyper has to be introduced anyways since one might filter on top of the geometry attribute without requiring it among the extracted properties ("give me the names of the countries touching the french borders").
        Hide
        Andrea Aime added a comment -
        Mixed patches togheter, fixed a few issues in each, committed.
        The test was working only in Eclipse since it was not actually reading from the copied over shapefile but from the classpath one instead,
        also it did not close the iterators (hint, never use plain itererators, always use FeatureIterator and close it)
        The actual patch was good minus a off by one error in the reindexing section
        Show
        Andrea Aime added a comment - Mixed patches togheter, fixed a few issues in each, committed. The test was working only in Eclipse since it was not actually reading from the copied over shapefile but from the classpath one instead, also it did not close the iterators (hint, never use plain itererators, always use FeatureIterator and close it) The actual patch was good minus a off by one error in the reindexing section
        Hide
        Jody Garnett added a comment -
        verified fix in uDig.
        Show
        Jody Garnett added a comment - verified fix in uDig.

          People

          • Assignee:
            Andrea Aime
            Reporter:
            Emily Gouge
          • Votes:
            1 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: