-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Conversation
@1ec5, thanks for your PR! By analyzing this pull request, we identified @boundsj, @incanus and @friedbunny to be potential reviewers. |
auto forwardFilter = forwardPredicate.mgl_filter; | ||
NSPredicate *forwardPredicateAfter = [NSPredicate mgl_predicateWithFilter:forwardFilter]; | ||
if (mustRoundTrip) { | ||
// Aggregates should round-trip, but for some reason only their format strings do. |
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.
The forwardPredicate
rhs expression is a NSConstantValueExpression(NSArray)
containing plain NSNumber
s initialized with ints but the forwardPredicateAfter
is a NSConstantValueExpression(NSArray)
containing NSConstantValueExpression(NSNumber)
s initialized with longs. The wrong data type could be another bug in -[NSExpression(MGLAdditions) mgl_constantMBGLValue]
? Also, shouldn't FilterEvaluator::getValues()
make use of +[NSExpression expressionForAggregate:]
if necessary?
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.
For the BETWEEN
case, forwardPredicateAfter
is already an aggregate expression under the hood, due to the use of braces here. For the IN
case, I’ll add +[NSExpression expressionForAggregate:]
for consistency with BETWEEN
.
I’m comfortable with the SDK “upgrading” int
s to long
s and literal collections to aggregates when round-tripping. While it’s unfortunate that Foundation makes distinctions here, it’s basically Postel’s Law: accepting either collections or aggregates in input, but only outputting aggregates; accepting either int
s or long
s in input, but only outputting long
s.
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.
All set.
Could you cherry pick the filter operators from https://github.com/mapbox/mapbox-gl-native/compare/deref? I need both |
ba67c03
to
ea4b63c
Compare
When converting predicates to filters, symmetric comparison predicates can now compare a value to a key in addition to the usual key-to-value order. Added error checking for unhandled combinations like key-to-key. Fixed a crash converting a CONTAINS predicate into a filter. Added support for constant value expressions inside aggregate expressions. Allow sets as aggregate expressions just like arrays, except in BETWEEN predicates where order matters. Flatten NOT predicates into more specialized filters. When converting filters to predicates, use constant value expressions inside aggregate expressions. Convert to a BETWEEN predicate when possible. Replaced predicate round-tripping integration tests with systematic unit tests for converting in either direction, plus unit tests for round-tripping and symmetry. Refined exception names and messages. Realphabetized files in groups.
There’s also an “l” operator for locale sensitivity.
ea4b63c
to
3d4305b
Compare
This PR rewrites the predicate-to-filter conversion code on iOS and macOS to support more of the predicate language without crashing and also adds comprehensive unit tests for this code.
When converting predicates to filters, symmetric comparison predicates can now compare a value to a key in addition to the usual key-to-value order. Added error checking for unhandled combinations like key-to-key. Fixed a crash converting a
CONTAINS
predicate into a filter. Added support for constant value expressions inside aggregate expressions. Allow sets as aggregate expressions just like arrays, except inBETWEEN
predicates where order matters. FlattenNOT
predicates into more specialized filters.When converting filters to predicates, use constant value expressions inside aggregate expressions. Convert to a
BETWEEN
predicate when possible.Removed the predicate integration tests, which tested round-tripping but didn’t ensure correct conversion and omitted many edge cases. Replaced the integration tests with systematic unit tests for converting in either direction, plus unit tests for round-tripping and symmetry.
The unit tests required the addition of
==
operators for all the classes associated withmbgl::style::Filter
.Fixes #7543, fixes #7544, fixes #7545, fixes #7547.
/cc @boundsj @jfirebaugh