-
Notifications
You must be signed in to change notification settings - Fork 53
Updates to Filter Extension for CQL2 #225
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
Conversation
|
Should review here wait on the symbolic operators change upstream? |
My understanding is that the current published spec allows either symbolic or alphabetic, and that's changing to only allow symbolic, so these are currently valid wrt that. |
|
Quickly looked at it and looks good to me, but I leave the full review and final approval up to an implementor. |
|
Looking good @philvarner! I think it would be good to include examples for BETWEEN, IN and the use of UPPER/LOWER. Having these examples is really helpful when building things out and also gives a great starting place for building out tests (ie these are used directly in the pgstac tests in the PR for adding CQL2 support) |
fragments/filter/README.md
Outdated
| "op": "eq", | ||
| "args": [ | ||
| { | ||
| "function" : "lower", |
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.
So upper and lower are implemented as "function" not with "op"?
As these are standard, shouldn't they use "op"? My impression is that "function" was for platform defined functions.
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.
Correct. They're not operators since they don't return a boolean value -- they return a string value. However, I looked it up in the spec and I did define them wrong -- they're not operators or functions, they're a separate thing, which is confusing, because the text syntax is exactly the same as if they were functions. I'll update the examples.
The spec is here:
https://github.com/opengeospatial/ogcapi-features/blob/b752479d850c52f372c4164202f041ecbe906e46/cql2/standard/schema/cql2.json
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 going to ask the CQL2 folks about this, because it seems like it would be simpler if it was just a pre-defined function.
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.
Filed this issue opengeospatial/ogcapi-features#656
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.
ah, so this is changing in CQL2 opengeospatial/ogcapi-features#641
UPPER/LOWER will be replaced with a function CASEI
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.
After looking at CASEI, I have some significant concerns about it. I'm just going to yank it for beta.5, and re-add after it's stabilitzed in CQL2. I wrote up the concerns here if you're interested
opengeospatial/ogcapi-features#641 (comment)
Related Issue(s): #222
Proposed Changes:
http://www.opengis.net/spec/cql2/insteadof
http://www.opengis.net/spec/ogcapi-features-3/http://www.opengis.net/spec/ogcapi-features-3/1.0/conf/basic-cql,http://www.opengis.net/spec/ogcapi-features-3/1.0/conf/cql-text, andhttp://www.opengis.net/spec/ogcapi-features-3/1.0/conf/cql-jsonhave hadcqlreplacedwith
cql2(in addition to the prefix change) tobecome
http://www.opengis.net/spec/cql2/1.0/conf/basic-cql2,http://www.opengis.net/spec/cql2/1.0/conf/cql2-text, andhttp://www.opengis.net/spec/cql2/1.0/conf/cql2-jsonopandargsstructureINTERSECTSis nowS_INTERSECTSANYINTERACTSis nowT_INTERSECTSProperty-Property Comparisons conformance class
(
http://www.opengis.net/spec/cql2/1.0/conf/case-insensitive-comparison) conformance classthat adds functions LOWER and UPPER for case-insensitive comparison
PR Checklist:
stac-specdirectory (these are included as a subtree and should be updated directly in radiantearth/stac-spec)