Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CQL2: Basic Spatial Operators issues #733

Closed
jerstlouis opened this issue Jul 6, 2022 · 5 comments · Fixed by #781
Closed

CQL2: Basic Spatial Operators issues #733

jerstlouis opened this issue Jul 6, 2022 · 5 comments · Fixed by #781

Comments

@jerstlouis
Copy link
Member

jerstlouis commented Jul 6, 2022

  • In 7.5.2 where a link to the WKT standard is provided, it would be very insightful to specifically refer to Section 7 (mention it in the link) of Simple Feature Access - Part 1: Common Architecture, which is where WKT is defined within that huge standard -- It wasn't clear to me at first that this was the right document where WKT is defined, since the title of the link differs significantly from the title of the standard (it would also be good to mention "Simple Features Access" as the OGC standard defining WKT in the reference). (at least 10,000 people are wondering where WKT is defined).

  • In A.5.1 Conformance Test 22, ENVELOPE is used with this syntax: ENVELOPE(-180,-90,180,90). In Simple Features Access, Envelope() is defined as a method to retrieve the envelop of the geometry. It is also a property of annotated text. It is not defined for the WKT encoding. This spatial4j issue mentions the ENVELOPE syntax being introduced in Catalogue Service CommonQL this way: ENVELOPE( minX, maxX, maxY, minY ) (with hatred for this order mentioned). The current Abstract Test is inconsistent with that order, but I later discovered^ in the Annex B BNF that the order has been changed to match the Features BBOX. No requirement introduces ENVELOPE specifically, it is not defined in the referenced Simple Features Access WKT, and ENVELOPE is also missing from the list of spatial data types for spatialLiteral in 7.5.1. To avoid order confusion with the original Catalogue Service CommonQL ENVELOPE (which has a different order), as well as with Envelope() being a method rather than a geometry type in Simple Features Access, could ENVELOPE perhaps be renamed to BBOX() in CQL2 (that can take 4 or 6 parameters, consistent with Features)? It would also be good to clarify that BBOX (or ENVELOPE) is a spatial literal that CQL2 introduces that is not defined in WKT / Simple Features Access.

    • Original production from Catalog Service CommonCQL: <Envelope Text> := EMPTY | <left paren> <WestBoundLongitude><comma> <EastBoundLongitude><comma> <NorthBoundLatitude><comma> <SouthBoundLatitude> <right paren>
    • (^Side note: the BNF appendix is not where us as implementers / readers of the spec are first inclined to look for answers -- it is an annex (even though it is normative), and it is made up of many production rules, which require following several of them to make sense of something, and it is easy to miss something, as in the case of intervals part of temporalLiteral, but instants part of scalarExpression. I think this is one of the main difficulties we have faced understanding the specification: parts of the CQL2 definitions not being explained in the relevant conformance classes text, but depending solely on deciphering the Annex B BNF)
  • Because implementing support for MultiPoints, Polygons (complete with holes), MultiPolygons, LineString, MultiLineString, GeometryCollection, and intersections for all these with all geometry types used on the server, from scratch still requires a great deal of effort, I would suggest to move these to the (full) Spatial Operators conformance class, and only keep POINT and the proposed BBOX (or ENVELOPE otherwise) along with S_INTERSECTS() in Basic Spatial Operators conformance class. This would also be consistent with the approach taken for the more complex INTERVAL temporal geometry literals and Temporal Operators vs. Basic CQL2.

    • If a comprehensive spatial library is used in the implementation, implementing all other operators alongside these more complex geometry types should not be difficult. (Unless there has been feedback that implementing the other required spatial operators is a challenge for some implementation that already implements intersection of all geometry types?)
    • If the implementation is done from scratch, intersecting POINT and BBOX literals against all geometry types available on the server is the 80%+ use case that is very easy to implement.
    • While it was previously mentioned that Features already has the bbox= query parameter for filtering based on bounding box or point, that did not consider the fact that:
      • a) CQL2 will likely be used extensively outside of OGC API - Features, in contexts where such a separate bbox query parameter might not be available or applicable, and
      • b) the separate bbox= query parameter is an AND at the very end of the "WHERE" clause, and it does not support scenarios where intersections are needed as part of more complex logical expressions. (actually, this was considered)
@cportele
Copy link
Member

Meeting 2022-07-22:

  • Agreed to explicitly state section 7.
  • ENVELOPE: Change to BBOX and be explicit that it is not defined in WKT and it was not part of the original CQL. BBOX content is consistent with the bbox parameter and the bounding box values in the Collection resource. Update the list in 7.5.1 Spatial data types and literal types.
  • There was disagreement how much simpler it would be support the intersection of a MultiPolygon with holes with a box vs another MultiPolygon. So far, this issue has not been raised before and it is late in process to open this issue at this time. To justify just a change we would need more feedback and support for this. If you think we should change the Basic Spatial Operators requirements class, please add your comments to this issue.

@jerstlouis
Copy link
Member Author

jerstlouis commented Jul 18, 2022

On the last point, leaving aside disagreement on MultiPolygon X MultiPolygon vs. MultiPolygon X BBOX complexity, the number of cases on which to support intersection testing (as well as support for geometry literals) in the implementation matrix is currently:

  • (Server Geometry Type) X (Point, BBOX, MultiPoint, LineString, MultiLineString, Polygon, MultiPolygon, GeometryCollection).

My suggestion here was to simplify this matrix for Basic Spatial Operators to:

  • (Server Geometry Type) X (POINT and BBOX).

Feedback from other implementors about whether this would be a better division line would be very valuable.
i.e., whether they think it would facilitate having more implementations of Basic Spatial Operators sooner, and whether the capabilities suggested to be included would still be useful enough to justify this different division.

cportele added a commit that referenced this issue Jul 18, 2022
cportele added a commit that referenced this issue Jul 18, 2022
this addresses the second item of #733
@cportele
Copy link
Member

cportele commented Aug 1, 2022

Meeting 2022-08-01: We are considering to reduce the requirements class "Basic Spatial Operator" to reduce the second operand to a point or a bbox to make it simpler for APIs that have no need for more complex geometries. We look for feedback on this and plan to make a decision in the next meeting on August 15.

@cportele
Copy link
Member

Meeting 2022-08-15: There was no feedback, but we really need feedback to make such a decision at this stage. We will leave this issue open for a few more weeks (likely until after the Code Sprint September 14-16) and then make a decision.

@cportele cportele self-assigned this Nov 21, 2022
@cportele cportele moved this from In progress to To be drafted in Features Part 3: Filtering / Common Query Language (CQL2) Nov 21, 2022
@cportele
Copy link
Member

Meeting 2022-11-21: We think having an "entry level" for spatial filtering is a good idea. Since there were no comments against the change we will implement it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging a pull request may close this issue.

2 participants