-
Notifications
You must be signed in to change notification settings - Fork 47
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
Align to OGC CQL #51
Align to OGC CQL #51
Conversation
TODO: Update examples.md and any other examples to use CQL |
Ok, I think this is ready for review. It's the biggest PR for the release, so I may have forgotten some bits, so review is appreciated @m-mohr and @matthewhanson I think this process got me to figure out openapi more, and I ran it through the stac I marked the query extension as 'deprecated', but then I did remove it from the openapi build, since it seemed like it'd be confusing to have it in there too. I suppose I could update its openapi document to say 'deprecated' everywhere if we want, but I feel like there's no reason to confuse new users, and implementors of older versions will figure out that it's deprecated. |
}, | ||
"filter": { | ||
"intersects": { | ||
"property": "geometry", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will this be required? since it's GeoJSON it will always be geometry
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty sure it will be. Even with GeoJSON you could have another property that is a geometry I believe. We can try to get them to change it in CQL, but I'm not sure if it makes sense to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@matthewhanson with the Queryables mechanism in Part 3, it's only named geometry
because the API advertises it is -- an API could just as well decide that the Item/geometry property is named "footprint" or "foobar".
Also, this syntax for cql-json isn't correct -- it should be:
"intersects": [
{ "property": "geometry" },
{
"type": "Polygon",
"coordinates": [[ ... ]]
}
]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Though in GeoJSON it is required that the geometry is called 'geometry' so a GeoJSON/STAC compliant item wouldn't be able to replace 'geometry' with 'footprint'. But it could have two geometries, 'geometry' and 'foobar'.
In GML they let you define any field name as the geometry, and Features API is agnostic, but I think practically if you're using GeoJSON then the property will always be set to geometry. Since CQL is similarly designed to be agnostic to the encoding it makes sense to require setting the property name. But for all STAC geometry stuff it will be 'geometry' We probably should discuss this somewhere in the API spec, indeed there's like a section on CQL / Queryables where we 'lock' it a bit more to the STAC content model than open the ability to redefine everything, so simpler clients can easily interact with STAC core fields through the API without having to fully understand 'queryables'.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Though in GeoJSON it is required that the geometry is called 'geometry' so a GeoJSON/STAC compliant item wouldn't be able to replace 'geometry' with 'footprint'. But it could have two geometries, 'geometry' and 'foobar'.
Correct, the Item would have to have a field with the name geometry, but with the Queryables mechanism, the implementer can define the name of variable that binds to the geometry field that is used in expressions to be whatever they want it to be. They can just as legally say "foobar
is bound to the field Item/geometry
" as saying "geometry
is bound to the field Item/geometry
", which they must state for every term that's valid in an expression.
### cql-text (GET) | ||
|
||
```http | ||
/search?filter=eo:cloud_cover < 10 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With CQL you can query top-level fields, such as geometry with the intersects parameter. Do other filters all assume they are property names?
How would one query another top-level field like assets
with CQL?
Or do normal properties have to be qualified, e.g., "properties.eo:cloud_cover"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty sure CQL assumes the other filters all assume they are property names. With OGC I think it follows the 'feature model', which says a geometry and zero or more attributes - which in GeoJSON map to properties. But it's a good question - would be good for CQL to clarify its use with GeoJSON.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to follow up on this, Part 3 requires that only queryables be used in expressions, so the implementer defines what the queryable-to-STAC field mapping is.
I need to dig more into how (or if) one can do queries like "item has asset with band where common_band red", since assets
is an array and eo:bands
is an array.
Finished reviewing, left some comments. Biggest thing is leaving in info on query filter but marking as deprecated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to wait for opengeospatial/ogcapi-features#461 to be merged and then check/update examples and OpenAPI $refs.
Looks like they will not get this in this week - opengeospatial/ogcapi-features#460 (comment) So I think we should just save this PR for beta.2, but mention that it's coming in a blog post. This one is also going to need be reworked with #60 landing. |
Just wanted to put a note on the status of this:
|
I created a new branch and PR #128 , since this one is against master and there was a large amount of rework to reapply it to develop. |
Closing this in favor #128 |
Related Issue(s): #32
Proposed Changes:
PR Checklist:
npm run generate-all
to update the generated OpenAPI files.