GeoTools

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. Text File
    reindex.patch
    17/Oct/09 8:43 AM
    6 kB
    Jody Garnett
  2. Java Source File
    ShapefileAttributeReaderTest.java
    14/Oct/09 4:14 PM
    2 kB
    Emily Gouge

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Vote (1)
Watch (0)

Dates

  • Created:
    Updated:
    Resolved: