The method ShapefileFeatureSource.getReadSchema(Query) that looks for the
attributes required for filtering relied on a blank attPath giving the
default geometry. I have added a fix that includes all geometry attributes
if a geometry filter is used.
This does bring up another question:
If we have a BBox query that does include a geometry property, should we
still be testing the BBox against all geometries (Assuming expression1 of
the BBox filter is blank), or should we test against only included geometry
properties?
Current changes are here: https://github.com/geotools/geotools/pull/659
On Tue, Dec 23, 2014 at 4:31 PM, Jody Garnett <[email protected]>
wrote:
> Yeah we do, it is kind of like doing an SQL query against one attribute,
> and then only returning the attributes you plan to use.
>
> There *should* be a method in the shapefile code that looks at what
> attributes are needed for filtering and ensures they are fetched and parsed
> (even if the value is thrown away after being used for filter (and not
> returned).
> --
> Jody Garnett
> Senior Software Engineer | Boundless <http://boundlessgeo.com/>
>
> On 23 December 2014 at 15:37, Torben Barsballe <
> [email protected]> wrote:
>
>> The only geotools test failure I got was in the shapefile data store
>> implementation:
>> There was a test of a BBox query, where the query specified only
>> non-geometry attributes in its property list. According to the test
>> implementation, this was intended to return a nonzero number of features
>> (presumably because the blank expression in the filter was intended to be
>> translated into the default geometry and then added to the schema during
>> retyping). With my change, the blank expression is not converted into the
>> default geometry. Then, the retyped scema only contains non-geometry
>> attribures, and the BBox query has no geometries to test against, therefore
>> returning no results.
>>
>> This seems like a bit of an edge case. Do we actually expect to return a
>> BBox match if no geometry properties are included in our query?
>>
>> I'll look into JDBCDataStore, but based on the tests it seems to be
>> working fine after this fix (perhaps the tests are insufficient?)
>>
>> Torben
>>
>> On Tue, Dec 23, 2014 at 3:05 PM, Jody Garnett <[email protected]>
>> wrote:
>>
>>> So we are into the PropertyName expression - which surprisingly does not
>>> take a property name - instead it takes an XPath expressions. So the
>>> question is not so much about using null, but ensuring we correctly handle
>>> an empty XPath expression.
>>>
>>> I expect ContentFeatureSource returns the default geometry in an effort
>>> to make JDBCDataStore implementation go faster. JDBCDataStore was the first
>>> production use of ContentDataStore so some optimizations may of been made.
>>>
>>> I like the idea of resolvePropertyNames returning a blank property name
>>> (since that is accurate). The only failures I would expect would be in the
>>> JDBCDataStore implementation - if we find that JDBCDataStore really wants
>>> to restrict itself to the default geometry (for performance rit can handle
>>> that when encoding SQL.
>>>
>>> There is a jdbc-notes.txt
>>> <https://github.com/geotools/geotools/blob/master/modules/plugin/jdbc/jdbc-postgis/jdbc-notes.txt>
>>> in the code base which describes JDBCDataStore using an
>>> encodeGeometryColumn method. Perhaps we can use this to catch an empty
>>> string being passed in.
>>> --
>>> Jody
>>>
>>> --
>>> Jody Garnett
>>> Senior Software Engineer | Boundless <http://boundlessgeo.com/>
>>>
>>> On 23 December 2014 at 13:17, Torben Barsballe <
>>> [email protected]> wrote:
>>>
>>>> The functionality to read and compare against multiple geometry
>>>> features exists, it is merely being ignored/bypassed due to the
>>>> resolvePropertyNames method in ContentFeatureSource. This method is
>>>> intended to resolve propertyName references to simple attribute names
>>>> against the schema of the feature source; eg. "gml:name" becomes simply
>>>> "name". However this method also turns a blank property name into the
>>>> default geometry name (I am not sure this is intentional or not).
>>>>
>>>> The WFS BBox request uses a query with two expression - An
>>>> AttributeExpression with a blank name and a BBox. When this query is put
>>>> through resolvePropertyNames() it makes the blank AttributeExpression refer
>>>> to the default geometry.
>>>>
>>>> Then, when ContentFeatureSource creates a FilteringFeatureReader, it
>>>> only checks against the default geometry. Note that the
>>>> AbstractFeatureSource implementation simply did not have an equivalent of
>>>> resolvePropertyNames, resulting in the blank name never being set to the
>>>> default geometry.
>>>>
>>>>
>>>> I am working on a "fix" that makes the resolvePropertyNames method
>>>> return a blank property name when it is given a blank property name by
>>>> altering the PropertyNameResolvingVisitor.visit() method. This fixed the
>>>> WFS test cases in question, although may have caused some other test
>>>> failures in geotools.
>>>>
>>>> I am not sure that this is the right way to fix this. Based on the
>>>> implementation of Query/Filter/Expression, I am thinking that maybe setting
>>>> the first expression in the Query to "null" when the Query is first created
>>>> might also work (I am going to try this next...)?
>>>>
>>>> Torben Barsballe
>>>>
>>>> On Tue, Dec 23, 2014 at 11:00 AM, Jody Garnett <
>>>> [email protected]> wrote:
>>>>
>>>>> It is not very common, occasionally you will get the same feature
>>>>> represented multiple times:
>>>>> - say a city represented as an urban area polygon and a point location
>>>>> for city hall.
>>>>> - or the same feature represented at different level of details
>>>>>
>>>>> In these cases often the feature has the same bounding box for the
>>>>> geometries.
>>>>>
>>>>> We can ask at the next Skype meeting, or hope others join this
>>>>> conversation.
>>>>>
>>>>> If you like you can try and prep a patch to allow ContentDataStore to
>>>>> match the expected Filter 2.0.2 functionality.
>>>>> --
>>>>> Jody
>>>>>
>>>>> On Mon, Dec 22, 2014 at 4:51 PM Torben Barsballe <
>>>>> [email protected]> wrote:
>>>>>
>>>>>> Your suggested fix seems to have gotten rid of most of the bounding
>>>>>> box problems (Although there are some other tests that assume the first
>>>>>> geometry is the default one that will have to be changed). However, I am
>>>>>> a
>>>>>> bit concerned about how reliable this change is. As you can see below,
>>>>>> the
>>>>>> dataset has 3 different geometry columns. Each of these columns is <null>
>>>>>> for at least one line. Given this, I am surprised that setting the point
>>>>>> column to be the default actually works.
>>>>>>
>>>>>> The other question would be is how often do we actually have multiple
>>>>>> geometries in a single feature type? If this is somewhat comen, then som
>>>>>> proper support might be a good idea. If this rarely occurs, then it might
>>>>>> be advisable to change the test data so it isn't using multiple geometry
>>>>>> features in one feature type.
>>>>>>
>>>>>> This is the dataset used for the WFS tests:
>>>>>>
>>>>>>
>>>>>> _=description:String,name:
>>>>>> String,surfaceProperty:Polygon:srid=4326,
>>>>>> pointProperty:Point:srid=4326,curveProperty:LineString:srid=
>>>>>> 4326,intProperty:Integer,uriProperty:java.net.URI,measurand:String,
>>>>>> dateTimeProperty:java.sql.Timestamp,dateProperty:java.
>>>>>> sql.Date,decimalProperty:Double,booleanProperty:Boolean
>>>>>> PrimitiveGeoFeature.f001=description-f001|name-f001|<null>|POINT(39.73245
>>>>>> 2.00342)|<null>|155|http://www.opengeospatial.org/|12765|
>>>>>> <null>|2006-10-25|5.03|true
>>>>>> PrimitiveGeoFeature.f002=description-f002|name-f002|<null>|POINT(59.41276
>>>>>> 0.22601)|<null>|154|http://www.opengeospatial.org/|12769|
>>>>>> <null>|2006-10-23|4.02|false
>>>>>> PrimitiveGeoFeature.f003=description-f003|name-f003|<null>|<null>|LINESTRING(46.074
>>>>>> 9.799,46.652 10.466,47.114 11.021)|180|<null>|672.1|<
>>>>>> null>|2006-09-01|12.92|true
>>>>>> PrimitiveGeoFeature.f008=description-f008|name-f008|POLYGON((45.174
>>>>>> 30.899,45.652 30.466,45.891 30.466,45.174
>>>>>> 30.899))|<null>|<null>|300|<null>|783.5|2006-06-27
>>>>>> 22:08:00-07|2006-12-12|18.92|true
>>>>>> PrimitiveGeoFeature.f015=<null>|<null>|<null>|POINT(34.94
>>>>>> -10.52)|<null>|-900|<null>|2.4|<null>|<null>|7.9|<null>
>>>>>>
>>>>>>
>>>>>> On Mon, Dec 22, 2014 at 3:26 PM, Jody Garnett <
>>>>>> [email protected]> wrote:
>>>>>>
>>>>>>> Good debugging Torben, you have hit on an oddness in the WFS
>>>>>>> specification. Much of the specification is written using a geometry
>>>>>>> expression against a feature ... and then it becomes a bit murky. As an
>>>>>>> web
>>>>>>> / xml specification it was written with the idea we would explore all
>>>>>>> the
>>>>>>> attributes (each expressed as an XML snippet). Since we are boiling this
>>>>>>> down into Filter and SQL queries we like the idea of a specific
>>>>>>> geometry to
>>>>>>> test against for speed / indexing / sanity.
>>>>>>>
>>>>>>> So the "correct" thing to do would be to look at the feature type
>>>>>>> definition and determine which attributes are "geometry" attributes and
>>>>>>> test against both.
>>>>>>> The "quick" thing to do is guess a default geometry (often the first
>>>>>>> one unless the user says different) and only test against that one.
>>>>>>>
>>>>>>> As you can see AbstractDataStore and ContentDataStore made a
>>>>>>> different choice!
>>>>>>>
>>>>>>> Aside: The WFS Specification is split into sections, the filter
>>>>>>> stuff is factored out so it can be used in a couple specifications
>>>>>>> (such as
>>>>>>> WFS and SLD).
>>>>>>>
>>>>>>> From the WFS Tutorial[1]: *This is known as the "bounding box" or
>>>>>>> BBOX. The BBOX allows us to ask for only such Features that are
>>>>>>> contained
>>>>>>> (or partially contained) inside a box of the coordinates we specify. The
>>>>>>> form of the bbox query is "bbox=a1,b1,a2,b2" where a, b, c, and d refer
>>>>>>> to
>>>>>>> coordinates.*
>>>>>>>
>>>>>>> The Filter [2] specification actually fixed things up to be clear
>>>>>>> this time: BBOX operator
>>>>>>>
>>>>>>> The fes:BBOX element is defined as a convenient and more compact way
>>>>>>> of encoding the very common bounding box constraint based on the
>>>>>>> gml:Envelope geometry. It is equivalent to the spatial operation
>>>>>>> <fes:Not><fes:Disjoint> … </fes:Disjoint></fes:Not> meaning that the
>>>>>>> fes:BBOX operator shall identify all geometries that spatially interact
>>>>>>> with the box. If there is only one argument (i.e. one child element
>>>>>>> specified for the BBOX operator, the calling service shall apply the
>>>>>>> operator to the geometric values of all the spatial properties of the
>>>>>>> resource. In this case, the operator shall evaluate to true if all
>>>>>>> tested
>>>>>>> spatial property values fulfill the spatial relationship implied by the
>>>>>>> operator. Otherwise, the BBOX operator shall evaluate the specified
>>>>>>> arguments (i.e. the two child elements) and test whether their geometric
>>>>>>> values satisfy the implied spatial relationship.
>>>>>>>
>>>>>>> I also note that Filter 2.0.2 matches our GeoTools understanding of
>>>>>>> Filter - i.e. no longer expects one of the expressions to be a
>>>>>>> propertyName.
>>>>>>>
>>>>>>> Links:
>>>>>>> [1] http://www.ogcnetwork.net/wfstutorial
>>>>>>> [2] http://docs.opengeospatial.org/is/09-026r2/09-026r2.html#60 <--
>>>>>>> finally html welcome to 1999 OGC you made it!
>>>>>>>
>>>>>>> To return the matter at hand, we may be able to mark which geometry
>>>>>>> is the default in the property file, and enable these tests to pass
>>>>>>> while
>>>>>>> still maintaining the ContendDataStore design decisions (focused on
>>>>>>> being
>>>>>>> quick and clear).
>>>>>>>
>>>>>>> The DataUtilities method createType
>>>>>>> <http://docs.geotools.org/latest/javadocs/org/geotools/data/DataUtilities.html#createType(java.lang.String,%20java.lang.String)>
>>>>>>> has
>>>>>>> the following javadoc examples:
>>>>>>>
>>>>>>> Examples:
>>>>>>>
>>>>>>> name:"",age:0,geom:Geometry,centroid:Point,url:java.io.URL
>>>>>>> id:String,polygonProperty:Polygon:srid=32615
>>>>>>> identifier:UUID,location:Point,*area:MultiPolygon,created:Date
>>>>>>> uuid:UUID,name:String,description:String,time:java.sql.Timestamp
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> See how the first one has both geom and centroid attributes? The
>>>>>>> javadocs also indicate you can use a * to mark one of these as a default
>>>>>>> geometry.
>>>>>>>
>>>>>>> The following would mark centroid as the default geometry:
>>>>>>> name:"",age:0,geom:Geometry,*centroid:Point,url:java.io.URL
>>>>>>>
>>>>>>> Making a similar change to the dataset used for your failed
>>>>>>> geoserver-devel test would allow it to pass.
>>>>>>> --
>>>>>>> Jody Garnett
>>>>>>> Senior Software Engineer | Boundless <http://boundlessgeo.com/>
>>>>>>>
>>>>>>> On 22 December 2014 at 14:12, Torben Barsballe <
>>>>>>> [email protected]> wrote:
>>>>>>>
>>>>>>>> While attempting to migrate geoserver to use gt-property-ng, I have
>>>>>>>> ran into a bit of an issue with the
>>>>>>>> ContentFeatureSource.resolvePropertyNames()
>>>>>>>> method, when it is called by ContentFeatureCollection.size().
>>>>>>>>
>>>>>>>> Geoserver is running a BBox query against a FeatureCollection
>>>>>>>> containing multiple geometries. expression1 of the BBox is a blank
>>>>>>>> attribute, expression2 is the BBox.
>>>>>>>>
>>>>>>>> When using gt-property and AbstractFeatureStore, expression1
>>>>>>>> remains blank throught execution, and the BBox query is tested against
>>>>>>>> each
>>>>>>>> geometry in the featureCollection
>>>>>>>>
>>>>>>>> When using gt-property-ng and ContentFeatureStore, the
>>>>>>>> resolvePropertyNames() method is called, and results in expression1
>>>>>>>> being
>>>>>>>> set to the first geometry. Then, when ContentFeatureCollection.size()
>>>>>>>> runs, the BBox query is only tested against this first geometry, and
>>>>>>>> fails
>>>>>>>> to return any results.
>>>>>>>>
>>>>>>>> It seems that the implementation of
>>>>>>>> DefaultGeometrySimpleFeaturePropertyAccessorFactory
>>>>>>>> is only intended to return a single geometry.
>>>>>>>> Upon analysis of the code, I feel that there are two places this
>>>>>>>> could probably be fixed:
>>>>>>>>
>>>>>>>> 1. Change DefaultGeometrySimpleFeaturePropertyAccessorFactory.get()
>>>>>>>> to handle multiple geometries properly, such that it tryAccessor
>>>>>>>> should set
>>>>>>>> value to an AttributeDescriptor of "" when xpath is "". However, I am
>>>>>>>> not
>>>>>>>> sure exactly what this class interacts with, and it looks like it is
>>>>>>>> doing
>>>>>>>> what it should be doing right now, so I am hesitant to change anything.
>>>>>>>>
>>>>>>>> Looking at how accessor caching and accessor factories are handled,
>>>>>>>> it is also possible that the Accessor caching implementation in
>>>>>>>> AttributeExpressionImpl.evaluate() is returning the wrong type of
>>>>>>>> accessor (DefaultGeometrySimpleFeaturePropertyAccessorFactory) but
>>>>>>>> this is not being caught because the value of "target" being tested
>>>>>>>> against
>>>>>>>> is null (causing the wrong accessor to (incorectly) return true for
>>>>>>>> accessor.canHandle();
>>>>>>>>
>>>>>>>> 2. Skip the resolvePropertyNames in ContentFeatureSource.getCount().
>>>>>>>> However, this seems to set some values that are required by later
>>>>>>>> calls,
>>>>>>>> and may require more significant rewriting.
>>>>>>>>
>>>>>>>> Alternatively, I could add a special check prior to
>>>>>>>> resolvePropertyNames such that it is not called in this specific
>>>>>>>> circumstance (Blank xpath, etc.) but this seems a bit hacky.
>>>>>>>>
>>>>>>>> For now, I will continue to compare the new implementation with the
>>>>>>>> implementation used by AbstractFeatureSource to see if I can come up
>>>>>>>> with
>>>>>>>> any ideas.
>>>>>>>>
>>>>>>>> Torben Barsballe
>>>>>>>>
>>>>>>>
>>>>>> ------------------------------------------------------------
>>>>>> ------------------
>>>>>> Dive into the World of Parallel Programming! The Go Parallel Website,
>>>>>> sponsored by Intel and developed in partnership with Slashdot Media,
>>>>>> is your
>>>>>> hub for all things parallel software development, from weekly thought
>>>>>> leadership blogs to news, videos, case studies, tutorials and more.
>>>>>> Take a
>>>>>> look and join the conversation now. http://goparallel.sourceforge.net
>>>>>> _______________________________________________
>>>>>> Geoserver-devel mailing list
>>>>>> [email protected]
>>>>>> https://lists.sourceforge.net/lists/listinfo/geoserver-devel
>>>>>>
>>>>>
>>>>
>>>>
>>>> ------------------------------------------------------------------------------
>>>> Dive into the World of Parallel Programming! The Go Parallel Website,
>>>> sponsored by Intel and developed in partnership with Slashdot Media, is
>>>> your
>>>> hub for all things parallel software development, from weekly thought
>>>> leadership blogs to news, videos, case studies, tutorials and more.
>>>> Take a
>>>> look and join the conversation now. http://goparallel.sourceforge.net
>>>> _______________________________________________
>>>> GeoTools-Devel mailing list
>>>> [email protected]
>>>> https://lists.sourceforge.net/lists/listinfo/geotools-devel
>>>>
>>>>
>>>
>>
>>
>> ------------------------------------------------------------------------------
>> Dive into the World of Parallel Programming! The Go Parallel Website,
>> sponsored by Intel and developed in partnership with Slashdot Media, is
>> your
>> hub for all things parallel software development, from weekly thought
>> leadership blogs to news, videos, case studies, tutorials and more. Take a
>> look and join the conversation now. http://goparallel.sourceforge.net
>> _______________________________________________
>> GeoTools-Devel mailing list
>> [email protected]
>> https://lists.sourceforge.net/lists/listinfo/geotools-devel
>>
>>
>
------------------------------------------------------------------------------
Dive into the World of Parallel Programming! The Go Parallel Website,
sponsored by Intel and developed in partnership with Slashdot Media, is your
hub for all things parallel software development, from weekly thought
leadership blogs to news, videos, case studies, tutorials and more. Take a
look and join the conversation now. http://goparallel.sourceforge.net
_______________________________________________
Geoserver-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/geoserver-devel
