[ios, macos] NSPredicate expression filters. #11587

fabian-guerra · 2018-04-03T22:20:58Z

Add support for predicate expression filters.
Add NSPredicate<->ExpressionFilter converter.
Update tests.
Update documentation.

1ec5 · 2018-04-09T19:05:12Z

platform/darwin/src/NSComparisonPredicate+MGLAdditions.mm

+        case NSLessThanPredicateOperatorType:
+        case NSLessThanOrEqualToPredicateOperatorType:
+        case NSInPredicateOperatorType:
+        case NSContainsPredicateOperatorType:
        case NSBetweenPredicateOperatorType: {


We don’t really need a switch statement here anymore. -mgl_jsonExpressionObject can be responsible for raising an exception when the operator type is unsupported.

1ec5 · 2018-04-09T19:12:52Z

platform/darwin/src/NSExpression+MGLAdditions.mm

+        } else if ([op isEqualToString:@"number"] ||
+                   [op isEqualToString:@"string"] ||
+                   [op isEqualToString:@"boolean"]) {
+            return [NSExpression mgl_expressionWithJSONObject:argumentObjects.firstObject];


I’m not sure it’s a good idea to make the conversion from JSON to NSExpression lossy like this. If these calls to number etc. are failing the round-tripping tests, feel free to modify the tests to start out with these calls so that they pass without needing to remove the type assertions.

1ec5 · 2018-04-09T19:14:18Z

platform/darwin/test/MGLPredicateTests.mm

+    {
+        NSPredicate *predicate = [NSPredicate predicateWithFormat:@"x == YES"];
+        NSArray *jsonExpression = @[@"==", @[@"get", @"x"], @YES];
+        XCTAssertEqualObjects(predicate.mgl_jsonExpressionObject, jsonExpression);


Also test that reversing the order of the operands produces the same kind of expression.

1ec5 · 2018-04-09T19:16:30Z

platform/darwin/src/NSComparisonPredicate+MGLAdditions.mm

+    }
+
+    if (type) {
+        return @[type, expressionElement.mgl_jsonExpressionObject];


It shouldn’t be the job of the platform-specific code to insert type assertions based on the data type of the constant value.

I agree with that. But we will have to deal with this type assertion until mapbox/mapbox-gl-js#6459 is done. Expression filter comparisons relay on the data type.

/cc @anandthakker

Unfortunately, the code in this PR is only capable of inserting type assertions for the most basic expressions, but not for something more complex like CAST(ele, 'NSNumber') * 0.3048 > 1000. To handle the whole gamut of expressions that could require type assertions, we’d have to implement static type analysis in Objective-C.

Since it’s probably too late for mapbox/mapbox-gl-js#6459 to make it into the v4.0.0 release, we’ll have to settle for expecting the developer to include type assertions (or, more likely, CAST()) in their predicates. As long as the type checking in this PR isn’t incorrect, I suppose we can keep it, but it would complicate the developer’s mental model of when they’d need to add type assertions themselves.

mapbox/mapbox-gl-js#6459 would not take very long to implement, once we have consensus on the design. Is this something we should try to expedite to get into v4.0.0?

@anandthakker if it's possible to get it done for 4.0.0 would be good. Otherwise I think we can live with this for now and in a patch release remove it.

/cc @1ec5

Getting mapbox/mapbox-gl-js#6459 in would be nice, but I’m wary of further delaying the v4.0.0 for it. Either way, the SDK code should not elide type assertions during conversion to NSPredicate. I guess we can live with introducing type assertions during conversion to JSON, with the understanding that it’s already quite limited and will be removed shortly.

1ec5 · 2018-04-11T23:45:09Z

platform/darwin/src/NSComparisonPredicate+MGLAdditions.mm

 #import "NSPredicate+MGLAdditions.h"
 #import "NSExpression+MGLPrivateAdditions.h"

 @implementation NSComparisonPredicate (MGLAdditions)

- (mbgl::style::Filter)mgl_filter {


We probably shouldn’t drop this conversion code until #11610 lands.

/cc @lucaswoj

My mistake: we only need to keep FilterEvaluator around, since that’s responsible for converting mbgl::Filters to NSPredicates as part of the predicate property’s getter. -mgl_filter, meanwhile, is part of the setter, which is already dead code, so we can remove it.

1ec5 · 2018-04-11T23:48:04Z

platform/darwin/src/NSComparisonPredicate+MGLAdditions.mm

+            case NSLessThanPredicateOperatorType:
+            case NSLessThanOrEqualToPredicateOperatorType:
+            case NSGreaterThanPredicateOperatorType:
+            case NSGreaterThanOrEqualToPredicateOperatorType: {


NSEqualToPredicateOperatorType and NSNotEqualToPredicateOperatorType as well.

This condition applies only for this operators since are the only ones required to pass the comparator type.

1ec5 · 2018-04-11T23:50:34Z

platform/darwin/src/NSComparisonPredicate+MGLAdditions.mm

    }
    return nil;
 }

+- (id)mgl_expressionWithExpressionType:(NSExpression *)expressionElement fallbackExpression:(NSExpression *)fallbackExpression {


It’s unclear to me why the first parameter is labeled expressionType and named expressionElement, and the name fallbackExpression implies that this expression is substituted for expressionElement if the latter is missing. Finally, this method returns a JSON object rather than an NSExpression.

If I understand the purpose of this method, a clearer name would be -mgl_typeCheckedExpressionMatchingExpression:. Instead of expressionElement, use self; instead of fallbackExpression, use otherExpression. Instead of returning a JSON expression object, return an NSExpression that calls MGL_FUNCTION(… 'boolean/number/string').

1ec5 · 2018-04-11T23:56:12Z

platform/darwin/src/NSPredicate+MGLAdditions.mm

@@ -259,28 +271,33 @@ + (instancetype)mgl_predicateWithJSONObject:(id)object {
    NSString *op = objects.firstObject;

    if ([op isEqualToString:@"=="]) {
-        NSArray *subexpressions = MGLSubexpressionsWithJSONObjects([objects subarrayWithRange:NSMakeRange(1, objects.count - 1)]);
+        NSArray *sanitizedObjects = MGLSanitizeJSONObjects([objects subarrayWithRange:NSMakeRange(1, objects.count - 1)]);


Per #11587 (comment), revert these changes and remove MGLSanitizeJSONObjects().

I removed the code you mentioned in #11587 (comment), but this code is required for comparison predicates to work as filters. MGLSanitizeJSONObjects() removes the data types before converting them.

Hmm. I think a problem here is that this is unconditionally removing all "number", "string", "boolean" assertions. Removing the ones that were automatically inferred is fine, because they can be inferred again if the expression is sent back into mbgl to be parsed. But it's possible to have explicitly-added assertions that would not be automatically inferred if they were stripped. For example, ["==", ["number", ["id"]], ["get", "x"]] is an expression that specifically asserts that a feature's id is a number and compares it to property with key "x". If the "number" wrapper were stripped here, the resulting expression would fail to parse. (And even if we relaxed the typing requirements so that it did parse, its behavior would be different than that of the original.)

This applies only to >, >=, <, <= this type of evaluation is not possible using NSExpression directly.

Ah, okay. Some expressions might still fail to roundtrip -- e.g., [">", ["string", ["get", "x"]], ["string", ["get", "y"]]] would result in [">", ["get", "x"], ["get", "y"]] -- but I suppose that's a known/acceptable issue?

@anandthakker @1ec5 seems like the best solution is require that the developers add CAST to filter predicates. Something like:
[NSPredicate predicateWithFormat:@"CAST(x, 'NSNumber') < 5"];
This way we avoid all the problems mentioned above.

The code here converts from JSON to NSPredicate. As long as an expression in JSON already has type assertions, those type assertions should be reflected in the NSPredicate. The fact that this prevents literal round-tripping of some predicates is unfortunate but not a big deal compared to potentially preventing an expression from working correctly.

1ec5 · 2018-04-11T23:57:58Z

platform/darwin/test/MGLExpressionTests.mm

@@ -394,28 +394,28 @@ - (void)testArithmeticExpressionObject {
    {
        NSExpression *expression = [NSExpression expressionForFunction:@"ceiling:" arguments:@[MGLConstantExpression(@1.5)]];
        NSArray *jsonTruncation = @[@"-", @1.5, @[@"%", @1.5, @1]];
-        NSArray *jsonExpression = @[@"+", jsonTruncation, @[@"case", @[@">", @[@"%", @1.5, @1], @0], @1, @0]];
+        NSArray *jsonExpression = @[@"+", jsonTruncation, @[@"case", @[@">", @[@"number", @[@"%", @1.5, @1]], @[@"number", @0]], @1, @0]];


#11653 significantly simplified these conversions from NSExpression to JSON. It’s no longer necessary to insert these type assertions.

1ec5 · 2018-04-11T23:58:37Z

platform/darwin/test/MGLPredicateTests.mm

@@ -23,558 +23,404 @@ @interface MGLPredicateTests : XCTestCase

 @implementation MGLPredicateTests

- (void)testFilterization {


Restore these tests until #11610 lands.

Filter test were not added per #11587 (comment).

1ec5 · 2018-04-12T00:04:19Z

platform/darwin/test/MGLPredicateTests.mm

-            },
-        };
-        MGLAssertEqualFilters(actual, expected);
+        NSPredicate *predicate = [NSPredicate predicateWithFormat:@"a != nil"];


This used to be converted into a NotHasFilter. Ideally, we’d convert it to ["!", ["has", ["get", "a"]]]. Are we sure that ["!=", ["get", "a"], null] has the desired effect?

["==", ["get", "a"], null] should be equivalent to ["!", ["has", "a"]], because get returns null for a missing key

That said, the latter is probably a more direct translation..

1ec5 · 2018-04-12T00:06:28Z

platform/darwin/test/MGLPredicateTests.mm

-        };
-        XCTAssertEqualObjects([NSPredicate mgl_predicateWithFilter:filter], [NSPredicate predicateWithFormat:@"NOT(a == 'b' OR c == 'd')"]);
+        NSPredicate *predicate = [NSPredicate predicateWithFormat:@"NOT %@ CONTAINS a", @[@"b", @"c"]];
+        NSArray *jsonExpression = @[@"!", @[@"has", @[@"literal", @[@"b", @"c"]], @[@"get", @"a"]]];


#11632 will correct this to use a match expression.

1ec5 · 2018-04-12T00:06:59Z

platform/darwin/test/MGLPredicateTests.mm

-            },
-        };
-        XCTAssertEqualObjects([NSPredicate mgl_predicateWithFilter:filter], [NSPredicate predicateWithFormat:@"NOT(a == 'b' OR c == 'd')"]);
+        NSPredicate *predicate = [NSPredicate predicateWithFormat:@"NOT %@ CONTAINS a", @[@"b", @"c"]];


#11632 will also test using an aggregate expression instead of a constant array value.

1ec5 · 2018-04-13T20:59:40Z

platform/darwin/src/NSComparisonPredicate+MGLAdditions.mm

+ Infers the expressionObject type if it can not then tries to infer the type using fallbackExpression.
+ ExpressionFilters of type >, >=, <, <= requires that each compared value provides its type.
+ */
+- (id)mgl_typedJSONWithExpressionObject:(NSExpression *)expressionObject fallbackExpression:(NSExpression *)fallbackExpression {


In general, any manipulation we do of expressions at the SDK level should be done in terms of NSPredicate or NSExpression APIs instead of raw Foundation types. That way, we aren’t as tightly coupled to the JSON expression syntax all over the place.

I think this method should have the following signature:

- (NSExpression *)mgl_typeCheckedExpressionMatchingExpression:(NSExpression *)otherExpression;

The following two comments explain how. The main thing is using self, since this is an instance method, though I do think avoiding JSON inside this method would be preferable.

/ref #11587 (comment)

That method is generic. I was passing both sides of the comparison expression. So calling self didn't make sense.

I removed this code since it's purpose to aid the developers in the migration is not working as intended and is causing more problems per mi comment #11587 (comment).

For future reference, it would’ve been possible to call, e.g., [leftExpression mgl_typeCheckedExpressionMatchingExpression:rightExpression], in which case self inside the method would’ve been equivalent to leftExpression at the call site.

1ec5 · 2018-04-13T21:01:34Z

platform/darwin/src/NSComparisonPredicate+MGLAdditions.mm

+ ExpressionFilters of type >, >=, <, <= requires that each compared value provides its type.
+ */
+- (id)mgl_typedJSONWithExpressionObject:(NSExpression *)expressionObject fallbackExpression:(NSExpression *)fallbackExpression {
+    NSExpression *expression = expressionObject.expressionType == NSConstantValueExpressionType ? expressionObject : fallbackExpression;


Replace expressionObject with self and fallbackExpression with otherExpression.

1ec5 · 2018-04-13T21:03:58Z

platform/darwin/src/NSComparisonPredicate+MGLAdditions.mm

+    }
+
+    if (type) {
+        return @[type, expressionObject.mgl_jsonExpressionObject];


If self isn’t already a function expression that casts or type-asserts, then wrap self in a new MGL_FUNCTION expression and return that expression.

1ec5 · 2018-04-13T21:10:54Z

platform/darwin/src/NSExpression+MGLAdditions.mm

@@ -551,7 +551,7 @@ + (instancetype)expressionWithMGLJSONObject:(id)object {
                return [NSExpression expressionForAggregate:MGLSubexpressionsWithJSONObjects(argumentObjects.firstObject)];
            }
            return [NSExpression expressionWithMGLJSONObject:argumentObjects.firstObject];
-        } else if ([op isEqualToString:@"to-boolean"]) {
+        } else if ([op isEqualToString:@"to-boolean"] || [op isEqualToString:@"boolean"]) {


boolValue was only ever meant to coerce a value to a Boolean, not assert that it is a Boolean. Previously, to-boolean would be translated to MGL_FUNCTION('to-boolean', …). Is there any potential for data loss this way?

This was not meant to make it here. Removed

1ec5

This PR is almost ready to go, but the “Predicates and Expressions” guide needs to be updated to remove the requirement that key paths appear on the left side of any comparison and also emphasize that CAST() should be used to coerce certain key paths to numbers for inequality comparisons.

1ec5 · 2018-04-14T02:06:43Z

platform/darwin/src/MGLVectorStyleLayer.h

@@ -47,7 +47,7 @@ MGL_EXPORT
 ```swift
 let layer = MGLLineStyleLayer(identifier: "contour", source: terrain)
 layer.sourceLayerIdentifier = "contours"
- layer.predicate = NSPredicate(format: "(index == 5 || index == 10) && ele >= 1500.0")
+ layer.predicate = NSPredicate(format: "(index == 5 || index == 10) && CAST(ele, 'NSNumber') >= 1500.0")


This illustrates the fact that many existing uses of predicates in v3.x will need to be modified slightly when migrating to v4.0.0. On balance, I think this is necessary until mapbox/mapbox-gl-js#6459, but it does mean we’ll need to be clear about this requirement in the documentation, particularly the function-to-expression migration guide and the “Predicates and Expressions” guide.

Also, the iOS and macOS changelogs should have a blurb noting that:

Predicate format strings can contain arithmetic and calls to built-in NSExpression functions.

Key paths may need to be cast to NSString or NSNumber in some cases.

1ec5 · 2018-04-16T17:06:21Z

platform/darwin/docs/guides/Predicates and Expressions.md

@@ -19,6 +19,17 @@ based on the feature’s attributes. Use the `MGLVectorStyleLayer.predicate`
 property to include only the features in the source layer that satisfy a
 condition that you define.

+When a predicate is used for filtering vector data the key should be cast
+explicitly into the key's type. A number based key will become: `CAST(key, 'NSNumber')`


Filtering vector data is the subject of this section, so it’s a bit redundant to say so.

1ec5 · 2018-04-16T17:07:39Z

platform/darwin/docs/guides/Predicates and Expressions.md

+`NSGreaterThanOrEqualToPredicateOperatorType` | `CAST(key, 'valueType') >= value`<br />`CAST(key, 'valueType') => value`
+`NSLessThanOrEqualToPredicateOperatorType`    | `CAST(key, 'valueType') <= value`<br />`CAST(key, 'valueType') =< value`
+`NSGreaterThanPredicateOperatorType`          | `CAST(key, 'valueType') > value`
+`NSLessThanPredicateOperatorType`             | `CAST(key, 'valueType') < value`


This table is redundant to the table below it, which introduces these operators. Instead, add a paragraph below that table mentioning that key may need to be cast to a number or string using the CAST() operator and the type NSNumber or NSString. (Mention these types explicitly, because Swift developers may be inclined to use Float or String instead.)

1ec5 · 2018-04-16T17:08:58Z

platform/darwin/docs/guides/Predicates and Expressions.md

+`NSGreaterThanPredicateOperatorType`          | `CAST(key, 'valueType') > value`
+`NSLessThanPredicateOperatorType`             | `CAST(key, 'valueType') < value`
+
+


Also reword the paragraph below that starts with “ The predicate's left-hand expression must be…”, because feature attributes or these special attributes don’t need to be on the left side anymore.

1ec5

Good to go as soon as the documentation feedback above is addressed.

jmkiley

Thank you!

fabian-guerra added iOS Mapbox Maps SDK for iOS macOS Mapbox Maps SDK for macOS release blocker Blocks the next final release labels Apr 3, 2018

fabian-guerra self-assigned this Apr 3, 2018

fabian-guerra requested a review from 1ec5 April 3, 2018 22:20

1ec5 reviewed Apr 9, 2018

View reviewed changes

fabian-guerra force-pushed the fabian-expression-filters-11567 branch 2 times, most recently from ba9115c to f36376c Compare April 11, 2018 18:12

1ec5 reviewed Apr 12, 2018

View reviewed changes

fabian-guerra requested a review from anandthakker April 13, 2018 16:55

fabian-guerra force-pushed the fabian-expression-filters-11567 branch 2 times, most recently from fe68238 to 44f8507 Compare April 13, 2018 20:22

1ec5 reviewed Apr 13, 2018

View reviewed changes

fabian-guerra force-pushed the fabian-expression-filters-11567 branch from 0f31a2a to 232bfd4 Compare April 13, 2018 23:30

1ec5 reviewed Apr 14, 2018

View reviewed changes

1ec5 reviewed Apr 16, 2018

View reviewed changes

1ec5 approved these changes Apr 16, 2018

View reviewed changes

fabian-guerra added 13 commits April 16, 2018 13:40

[ios, macos] Add NSAndPredicateType predicate expression filter.

3ecbc71

[ios, macos] Add type to NSPredicate expressions.

d6a8e13

[ios, macos] Add Expression filter support.

038e5cb

[ios, macos] Update NSPredicate expression based tests.

1ef2486

[ios, macos] Refactor ExpressionFilters.

870e226

[ios, macos] Add symmetric test to ExpressionFilters.

c7256af

[ios, macos] Update NSPredicate test to ExpressionFilters.

7bc47ae

[ios, macos] Re-introduce Filter tests.

bce8c17

[ios, macos] Remove typed NSComparisonPredicate's comparable values.

e0dde38

[ios, macos] Update style layers predicate tests.

027fd6f

[ios, macos] Remove unused predicate conversion code.

e6f945f

[ios, macos] Update documentation exaple's test.

d2ab1dd

[ios, macos] Update Predicate and Expressions guide..

a550621

fabian-guerra added 3 commits April 16, 2018 13:57

[ios, macos] Remove mgl_ prefix from variable expressions.

40fda88

[ios, macos] Update predicates and expressions documentation.

0dbfcd8

[ios, macos] Update changelogs.

59665b0

fabian-guerra force-pushed the fabian-expression-filters-11567 branch from e0f4395 to 59665b0 Compare April 16, 2018 19:13

fabian-guerra requested a review from jmkiley April 16, 2018 19:13

jmkiley approved these changes Apr 16, 2018

View reviewed changes

fabian-guerra merged commit 62dd097 into release-boba Apr 16, 2018

fabian-guerra deleted the fabian-expression-filters-11567 branch April 16, 2018 20:52

fabian-guerra mentioned this pull request Apr 16, 2018

Support expression operators in style layer predicates #11567

Closed

		@@ -23,558 +23,404 @@ @interface MGLPredicateTests : XCTestCase

		@implementation MGLPredicateTests

		- (void)testFilterization {

		`NSGreaterThanPredicateOperatorType` \| `CAST(key, 'valueType') > value`
		`NSLessThanPredicateOperatorType` \| `CAST(key, 'valueType') < value`

[ios, macos] NSPredicate expression filters. #11587

[ios, macos] NSPredicate expression filters. #11587

Conversation

fabian-guerra commented Apr 3, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1ec5 Apr 13, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1ec5 Apr 12, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1ec5 Apr 13, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1ec5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1ec5 left a comment

Choose a reason for hiding this comment

jmkiley left a comment

Choose a reason for hiding this comment

fabian-guerra commented Apr 3, 2018 •

edited

1ec5 Apr 13, 2018 •

edited

1ec5 Apr 12, 2018 •

edited

1ec5 Apr 13, 2018 •

edited