-
Notifications
You must be signed in to change notification settings - Fork 1.3k
[ios, macos] Add predicate like expressions to NSExpression #11632
Conversation
@@ -62,6 +62,7 @@ + (void)installFunctions { | |||
INSTALL_METHOD(mgl_step:from:stops:); | |||
INSTALL_METHOD(mgl_coalesce:); | |||
INSTALL_METHOD(mgl_does:have:); | |||
INSTALL_METHOD(mgl_expression:betweenLeftHandExpression:rightHandExpression:); |
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.
It’s unnecessary to provide a custom expression function for the BETWEEN
operator. This operator is specific to NSPredicate and can’t be used inside an NSExpression. There’s already a way to use the BETWEEN
operator without a format string:
[NSComparisonPredicate predicateWithLeftExpression:… rightExpression:… modifier:NSDirectPredicateModifier type:NSBetweenPredicateOperatorType options:0]
In general, we should be using NSPredicate itself, rather than NSExpression, to represent Boolean-typed mbgl expressions, which is why I didn’t bother to create an aftermarket function for converting to Boolean values in #11472.
|
||
|
||
return [NSExpression expressionForFunction:@"mgl_expression:betweenLeftHandExpression:rightHandExpression:" | ||
arguments:@[leftHandPredicate.leftExpression, leftHandPredicate.rightExpression, rightHandPredicate.rightExpression]]; |
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.
Create an NSComparisonPredicate with NSBetweenPredicateOperatorType
rather than an NSExpression.
@@ -900,6 +924,10 @@ - (id)mgl_jsonExpressionObject { | |||
} | |||
[NSException raise:NSInvalidArgumentException | |||
format:@"Casting expression to %@ not yet implemented.", type]; | |||
} else if ([function isEqualToString:@"mgl_expression:betweenLeftHandExpression:rightHandExpression:"] || | |||
[function isEqualToString:@"mgl_between:"]) { |
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.
Instead of adding a case here, add a case to -[NSComparisonPredicate(MGLAdditions) mgl_jsonExpressionObject]
for NSBetweenPredicateOperatorType
.
@@ -280,7 +280,7 @@ + (instancetype)mgl_predicateWithJSONObject:(id)object { | |||
} | |||
if ([op isEqualToString:@">="]) { | |||
NSArray *subexpressions = MGLSubexpressionsWithJSONObjects([objects subarrayWithRange:NSMakeRange(1, objects.count - 1)]); | |||
return [NSPredicate predicateWithFormat:@"%K >= %@" argumentArray:subexpressions]; | |||
return [NSPredicate predicateWithFormat:@"%@ >= %@" argumentArray:subexpressions]; |
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.
Good catch.
359d5d0
to
4b7d427
Compare
@@ -318,14 +318,32 @@ - (id)mgl_jsonExpressionObject { | |||
case NSNotEqualToPredicateOperatorType: | |||
op = @"!="; | |||
break; | |||
case NSBetweenPredicateOperatorType: { | |||
op = @"all"; | |||
NSArray *arguments = self.rightExpression.constantValue; |
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.
This variable name is misleading. The right expression contains the minimum and maximum bounds, so bounds
or limits
would be a better name.
@@ -318,14 +318,32 @@ - (id)mgl_jsonExpressionObject { | |||
case NSNotEqualToPredicateOperatorType: | |||
op = @"!="; | |||
break; | |||
case NSBetweenPredicateOperatorType: { | |||
op = @"all"; | |||
NSArray *arguments = self.rightExpression.constantValue; |
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.
BETWEEN
, CONTAINS
, and IN
are most commonly written with aggregate expressions inline, not with arrays as constant values. For example, in the format string a BETWEEN {1, 3}
, {1, 3}
is an aggregate expression whose collection is an array. It’s important to support aggregate expressions with these three operators, although supporting constant value arrays is also nice to have.
NSString *leftOperator = leftCondition.firstObject; | ||
NSString *rightOperator = rightCondition.firstObject; | ||
if ([leftOperator isEqualToString:@"<="] && [rightOperator isEqualToString:@"<="]) { | ||
return [NSPredicate predicateWithFormat:@"%@ BETWEEN %@", [NSExpression mgl_expressionWithJSONObject:leftCondition[2]], @[[NSExpression mgl_expressionWithJSONObject:leftCondition[1]], [NSExpression mgl_expressionWithJSONObject:rightCondition[2]]]]; |
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 following JSON object:
[
"all",
["<=", ["get", "a"], 3],
["<=", 1, ["get", "a"]]
]
should convert to a BETWEEN {1, 3}
. Instead, this code turns it into 3 BETWEEN {a, a}
.
if ([leftOperator isEqualToString:@"<="] && [rightOperator isEqualToString:@"<="]) { | ||
return [NSPredicate predicateWithFormat:@"%@ BETWEEN %@", [NSExpression mgl_expressionWithJSONObject:leftCondition[2]], @[[NSExpression mgl_expressionWithJSONObject:leftCondition[1]], [NSExpression mgl_expressionWithJSONObject:rightCondition[2]]]]; | ||
} else if([leftOperator isEqualToString:@">="] && [rightOperator isEqualToString:@"<="]) { | ||
return [NSPredicate predicateWithFormat:@"%@ BETWEEN %@", [NSExpression mgl_expressionWithJSONObject:leftCondition[1]], @[[NSExpression mgl_expressionWithJSONObject:leftCondition[2]], [NSExpression mgl_expressionWithJSONObject:rightCondition[2]]]]; |
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 following JSON object:
[
"all",
[">=", 3, ["get", "a"]],
["<=", 1, ["get", "a"]]
]
should convert to a BETWEEN {1, 3}
. Instead, this code turns it into 3 BETWEEN {a, a}
.
return [NSPredicate predicateWithFormat:@"%@ BETWEEN %@", [NSExpression mgl_expressionWithJSONObject:leftCondition[1]], @[[NSExpression mgl_expressionWithJSONObject:leftCondition[2]], [NSExpression mgl_expressionWithJSONObject:rightCondition[1]]]]; | ||
} | ||
} | ||
} |
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 the following cases (in pseudocode) should be translated to an expression with the format string a BETWEEN {1, 3}
or a BETWEEN {3, 1}
:
- a >= 1 AND a <= 3
- 1 <= a AND a <= 3
- 1 <= a AND 3 >= a
- a >= 1 AND 3 >= a
- 1 <= a AND a <= 3
- a <= 3 AND a >= 1
- 3 >= a AND a >= 1
- 3 >= a AND 1 <= a
- a <= 3 AND 1 <= a
- 3 >= a AND a >= 1
return [NSCompoundPredicate andPredicateWithSubpredicates:subpredicates]; | ||
} | ||
if ([op isEqualToString:@"any"]) { | ||
NSArray *subpredicates = MGLSubpredicatesWithJSONObjects([objects subarrayWithRange:NSMakeRange(1, objects.count - 1)]); | ||
return [NSCompoundPredicate orPredicateWithSubpredicates:subpredicates]; | ||
} | ||
if ([op isEqualToString:@"match"]) { | ||
NSArray *subpredicates = MGLSubexpressionsWithJSONObjects([objects subarrayWithRange:NSMakeRange(1, objects.count - 1)]); | ||
return [NSPredicate predicateWithFormat:@"%@ IN %@" argumentArray:subpredicates]; |
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 following JSON objects:
[
"match",
["get", "a"],
1, true,
2, true,
3, true,
4, true,
false
]
[
"match",
["get", "a"],
[1, 2, 3, 4], true,
false
]
should translate to an NSExpression with the format string a IN {1, 2, 3, 4}
. We don’t currently support translating an NSExpression to a match
that consolidates cases like that (#11539), but I think it’s possible for us to encounter such an expression here if the style JSON defines it that way.
Not all match
expressions can be represented by the IN
operator, only expressions that have two unique output values, true
and false
.
NSArray *subpredicates = MGLSubexpressionsWithJSONObjects([objects subarrayWithRange:NSMakeRange(1, objects.count - 1)]); | ||
return [NSPredicate predicateWithFormat:@"%@ IN %@" argumentArray:subpredicates]; | ||
} | ||
if ([op isEqualToString:@"has"]) { |
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.
A has
expression checks whether an object has a particular property; it doesn’t check whether an array contains a particular object. We need to convert a match
expression to either the CONTAINS
or IN
operator (pick one), but we shouldn’t convert a has
expression to either operator.
op = @"match"; | ||
break; | ||
case NSContainsPredicateOperatorType: | ||
op = @"has"; |
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.
A CONTAINS
predicate should be translated into a match
expression. For example, an NSExpression with the format string {1, 2, 3, 4} CONTAINS a
should produce the following JSON object:
[
"match",
["get", "a"],
[1, 2, 3, 4], true,
false
]
return @[op, leftHandPredicate.mgl_jsonExpressionObject, rightHandPredicate.mgl_jsonExpressionObject]; | ||
} | ||
case NSInPredicateOperatorType: | ||
op = @"match"; |
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.
A match
expression takes different arguments than an IN
predicate. An NSExpression with the format string a IN {1, 2, 3, 4}
should produce the following JSON object:
[
"match",
["get", "a"],
[1, 2, 3, 4], true,
false
]
4b7d427
to
534e281
Compare
@@ -318,14 +318,32 @@ - (id)mgl_jsonExpressionObject { | |||
case NSNotEqualToPredicateOperatorType: | |||
op = @"!="; | |||
break; | |||
case NSBetweenPredicateOperatorType: { | |||
op = @"all"; | |||
NSArray *limits = self.rightExpression.constantValue; |
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 right expression is more likely to be an aggregate expression than a constant value expression. An expression with the format a BETWEEN {1, 3}
will crash here.
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.
This works for aggregate expressions. In https://github.com/mapbox/mapbox-gl-native/pull/11632/files#diff-6753380c30f27dfd9377e67fca76d44cR581 I added the test.
Wether is a NSConstantValueExpressionType
— for this case — or NSAggregateExpressionType
the underlaying value is an `NSArray.
NSPredicate *leftHandPredicate = [NSComparisonPredicate predicateWithLeftExpression:limits[0] | ||
rightExpression:self.leftExpression | ||
modifier:NSAllPredicateModifier | ||
type:NSLessThanOrEqualToPredicateOperatorType |
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.
a BETWEEN {1, 3}
and a BETWEEN {3, 1}
are equivalent, but this code turns the latter into a >= 3 AND a <= 1
, which is always false. That said, I don’t think -mgl_filter
has ever handled this case correctly either – I just learned today that it’s possible to reverse the bounds this way – so it’d be fine to address the issue separately.
Musing a bit, I can think of two ways to approach the problem, both of them a bit uglier:
- Cover both scenarios explicitly:
(a >= 3 AND a <= 1) OR (a <= 3 AND a >= 1)
- Ensure the correct order:
a >= min({3, 1}) AND a <= max({3, 1})
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 think the reverse format we can follow up later. I was adding a predicate to ensure the correct order, but evaluating a >= min({3, 1}) AND a <= max({3, 1})
will not work all the time.
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.
Oh, I meant that we could translate a BETWEEN {b, c}
to a >= min({b, c}) AND a <= max({b, c})
then convert to JSON. I don’t see why that wouldn’t work, but it certainly would complicate any attempt to round-trip back to NSPredicate, if we decide later on that that would be desirable.
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.
What I meant is if you evaluate the predicate a >= min({3, 1}) AND a <= max({3, 1})
to ensure that BETWEEN
is in the correct order it won't work since evaluating to a keyPath a
does not contain any value at that moment. If we evaluate a constant value like 2 >= min({3, 1}) AND 2 <= max({3, 1})
then it will.
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.
Couldn’t we do something like this?
[NSPredicate predicateWithFormat:@"%@ >= min(%@) AND %@ <= max(%@)", self.leftExpression, self.rightExpression, self.leftExpression, self.rightExpression];
return [self mgl_jsonMatchObjectWithExpression:self.leftExpression options:self.rightExpression]; | ||
} | ||
case NSContainsPredicateOperatorType: { | ||
return [self mgl_jsonMatchObjectWithExpression:self.rightExpression options:self.leftExpression]; |
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.
It’s unclear to me why the first parameter is named operand
and the second is named options
. Based on how this method is being used, I think one parameter could be named inputExpression
and the other valuesExpression
.
Better yet, we can inline this function under the NSInPredicateOperatorType
case above. Then the case for NSContainsPredicateOperatorType
could recurse, similar to the following:
NSPredicate *inPredicate = [NSComparisonPredicate predicateWithLeftExpression:self.rightExpression
rightExpression:self.leftExpression
modifier:self.modifier
type:NSInPredicateOperatorType
options:self.options];
return inPredicate.mgl_jsonExpressionObject;
NSArray *limits = self.rightExpression.constantValue; | ||
NSPredicate *leftHandPredicate = [NSComparisonPredicate predicateWithLeftExpression:limits[0] | ||
rightExpression:self.leftExpression | ||
modifier:NSAllPredicateModifier |
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 an operator like >=
or <=
, the modifier should be NSDirectPredicateModifier
. This SDK’s NSPredicate implementation has so far never supported NSAllPredicateModifier
, which is for an expression like a > ALL {1, 2, 3}
, which means “a is greater than each of the elements in {1, 2, 3}”.
@@ -335,4 +353,18 @@ - (id)mgl_jsonExpressionObject { | |||
return nil; | |||
} | |||
|
|||
- (id)mgl_jsonMatchObjectWithExpression:(NSExpression *)operand options:(NSExpression *)options { | |||
NSMutableArray *elements = [NSMutableArray arrayWithObjects:@"match", operand.mgl_jsonExpressionObject, nil]; | |||
NSArray *optionsExpressions = options.constantValue; |
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 right side of an IN
predicate or the left side of a CONTAINS
predicate is more likely to be an aggregate expression than a constant array value.
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 know but applies the same as in #11632 (comment)
while (id object = optionsEnumerator.nextObject) { | ||
id option = ((NSExpression *)object).mgl_jsonExpressionObject; | ||
[elements addObject:option]; | ||
[elements addObject:[NSExpression expressionForConstantValue:@YES].mgl_jsonExpressionObject]; |
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.
This is correct, but I think [elements addObject:@YES]
would be clearer.
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.
This was changed to:
while (id object = optionsEnumerator.nextObject) {
id option = ((NSExpression *)object).mgl_jsonExpressionObject;
[elements addObject:option];
[elements addObject:@YES];
}
[elements addObject:@NO];
But for some reason is not refreshed in github.
leftConditionExpression = [NSExpression mgl_expressionWithJSONObject:leftCondition[1]]; | ||
} | ||
|
||
if (limits && limits) { |
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.
Did you mean to check whether leftConditionExpression
is non-nil?
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.
Yes leftConditionExpression
if (jsonObjects.count == 2) { | ||
// Determine if the expression is of BETWEEN type | ||
if ([jsonObjects[0] isKindOfClass:[NSArray class]] && | ||
[jsonObjects[1] isKindOfClass:[NSArray class]]) { |
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.
Starting from here, I think it would be more concise to work with the NSPredicates in subpredicates
, then only call +[NSExpression mgl_expressionWithJSONObject:]
at the end.
7886ef3
to
8a47d37
Compare
8a47d37
to
562f69c
Compare
NSMutableArray *elements = [NSMutableArray arrayWithObjects:@"match", self.leftExpression.mgl_jsonExpressionObject, nil]; | ||
NSArray *optionsExpressions = self.rightExpression.constantValue; | ||
NSEnumerator *optionsEnumerator = optionsExpressions.objectEnumerator; | ||
while (id object = optionsEnumerator.nextObject) { |
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.
A while
loop with an NSEnumerator is only useful when you need to also work with indices, which isn’t the case here. (And in that case, you could use the more modern -enumerateObjectsAtIndexes:options:usingBlock:
anyways.) Replace the while
loop with a for
-in
loop, which takes advantage of fast enumeration.
NSArray *subpredicates = MGLSubpredicatesWithJSONObjects([objects subarrayWithRange:NSMakeRange(1, objects.count - 1)]); | ||
NSArray *jsonObjects = [objects subarrayWithRange:NSMakeRange(1, objects.count - 1)]; | ||
NSArray<NSPredicate *> *subpredicates = MGLSubpredicatesWithJSONObjects(jsonObjects); | ||
if (jsonObjects.count == 2) { |
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.
From this line onward, there should be no references to jsonObjects
. Use subpredicates
instead.
/ref #11632 (comment)
if (jsonObjects.count == 2) { | ||
// Determine if the expression is of BETWEEN type | ||
if ([jsonObjects[0] isKindOfClass:[NSArray class]] && | ||
[jsonObjects[1] isKindOfClass:[NSArray class]]) { |
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.
Check whether the first and last subpredicates are both NSComparisonPredicates, then declare leftPredicate
and rightPredicate
variables of type NSComparisonPredicate.
NSArray *limits; | ||
NSExpression *leftConditionExpression; | ||
|
||
if([leftOperator isEqualToString:@">="] && [rightOperator isEqualToString:@"<="]) { |
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.
Compare leftPredicate.predicateOperatorType == NSGreaterThanOrEqualToPredicateOperatorType
instead of comparing strings. I don’t think there would be a significant performance difference either way, but we’d benefit from stronger type checking.
return [NSComparisonPredicate predicateWithLeftExpression:expression | ||
rightExpression:[NSExpression expressionForConstantValue:@YES] | ||
modifier:NSDirectPredicateModifier | ||
type:NSNotEqualToPredicateOperatorType |
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 think we want ["match", ["get", "a"], 1, true, 2, true, 3, true, 4, true, false]
to end up as MGL_MATCH(a, 1, true, 2, true, 3, true, false) == true
, so the type should be NSEqualToPredicateOperatorType
.
XCTAssertEqualObjects(predicate.mgl_jsonExpressionObject, expected); | ||
NSString *mglMatch = @"MGL_MATCH($mgl_featureIdentifier, 6, YES, 5, YES, 4, YES, 3, YES, NO)"; | ||
NSPredicate *predicateAfter = [self matchPredicateWithFormat:mglMatch]; | ||
XCTAssertEqualObjects([NSPredicate mgl_predicateWithJSONObject:expected], predicateAfter); |
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.
Add tests that convert match
expressions to NSPredicates.
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.
What is the purpose of this test? comparing directly match
expressions to match
expression predicates will not work because we add the condition ... == YES
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.
Right, what I mean is that we should test that behavior, to verify that #11632 (comment) is fixed.
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.
Looks like you already added these tests, never mind.
NSPredicate *predicate = [NSPredicate predicateWithFormat:@"{ 6, 5, 4, 3} CONTAINS x"]; | ||
XCTAssertEqualObjects(predicate.mgl_jsonExpressionObject, expected); | ||
NSString *mglMatch = @"MGL_MATCH(x, 6, YES, 5, YES, 4, YES, 3, YES, NO)"; | ||
NSPredicate *predicateAfter = [self matchPredicateWithFormat:mglMatch]; |
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.
A more concise way to write predicateAfter
would be [NSPredicate predicateWithFormat:@"MGL_MATCH(x, 6, YES, 5, YES, 4, YES, 3, YES, NO) == YES"]
, so a separate -matchPredicateWithFormat:
method would be unnecessary.
return [NSCompoundPredicate andPredicateWithSubpredicates:subpredicates]; | ||
} | ||
if ([op isEqualToString:@"any"]) { | ||
NSArray *subpredicates = MGLSubpredicatesWithJSONObjects([objects subarrayWithRange:NSMakeRange(1, objects.count - 1)]); | ||
return [NSCompoundPredicate orPredicateWithSubpredicates:subpredicates]; | ||
} | ||
if ([op isEqualToString:@"match"]) { |
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.
This case is written generically enough to be a catch-all for any operator not handled above. So we can have the code below run unconditionally and remove the assertion at the bottom of the method.
a171f1c
to
2a4f753
Compare
2a4f753
to
711338e
Compare
Fixes #11013
Add the following operators to
NSExpression
.BETWEEN
IN
CONTAINS