[core] Introduce dedicated filter types for $type and $id special cases

[jfirebaugh]

Refs #7552

[mention-bot]

@jfirebaugh, thanks for your PR! By analyzing this pull request, we identified @yhahn and @springmeyer to be potential reviewers.

[1ec5]

iOS and macOS tests are failing because FilterEvaluator in NSPredicate+MGLAdditions.mm has no operator () for the new type filter types:

▸ Compiling NSPredicate+MGLAdditions.mm

❌  /Users/vagrant/git/mason_packages/headers/variant/1.1.4/include/mapbox/variant.hpp:312:20: no matching function for call to object of type 'FilterEvaluator'

            return f(unwrapper<T>::apply(v.template get_unchecked<T>()));
                   ^



❌  /Users/vagrant/git/mason_packages/headers/variant/1.1.4/include/mapbox/variant.hpp:312:20: no matching function for call to object of type 'FilterEvaluator'

            return f(unwrapper<T>::apply(v.template get_unchecked<T>()));
                 ^


make: *** [run-test-*] Error 65

For now, let’s have the conversion match the existing documentation to avoid backwards compatibility concerns. For example:

[NSPredicate predicateWithFormatString:@"%@ = 'LineString'", @"$type"]
[NSPredicate predicateWithFormatString:@"%@ IN { 'LineString', 'Polygon' }", @"$type"]

If we’re considering cherry-picking this change into iOS SDK v3.4.2, then I’d recommend aliasing 1, 2, 3 to Point, LineString, and Polyline, respectively, when setting a layer’s predicate (but always producing the strings when getting a predicate). Otherwise, I think we should chalk the current behavior up to buggy behavior that isn’t worth preserving.

I’ve also identified #7298 as an alternative syntax that we should also support in a future release, but that doesn’t have to happen within the scope of this bug fix (and it also doesn’t have to affect the getter as long as we’re worried about backwards compatibility).

[jfirebaugh]

Feel free to push SDK changes to this PR.

@ivovandongen, can you comment on whether the Android SDK has a similar issue to #7552 and how this change would affect it?

[1ec5]

The style validator allows the has and !has operators with $id, right?

[ivovandongen]

@jfirebaugh The $type filter works correctly with string values on this branch, thanks. There is no accessor yet on Android, added #7978 for this.

[1ec5]

@ivovandongen, so if you set eq("$type", "Point") on Android, will you get a TypeEqualsFilter under the hood? Or will mbgl be smart enough to convert it from an EqualsFilter to a TypeEqualsFilter even when specified via runtime styling?

[jfirebaugh]

The style validator allows the has and !has operators with $id, right?

The validator allows them but they don't work as expected: mapbox/mapbox-gl-js#4235.

[1ec5]

b7fa2c0cf54f25249c1d24eb0df59c3a265536ec takes care of the changes needed for iOS and macOS.

[jfirebaugh]

Updated with ["has"/"!has", "$id"] support.

[1ec5]

8efe3b97102d753ca4c6015c4071c99ab88193b7 also takes care of $id = nil and $id != nil on iOS and macOS and adds more unit tests.

[1ec5]

failed regressions mapbox-gl-js#4235

[boundsj]

The iOS and macOS changes look good to me.

[1ec5]

Thanks! I think all that’s left is:

• Core regression test failure: [core] Introduce dedicated filter types for $type and $id special cases #7971 (comment) @jfirebaugh
• Does the eq() syntax produce the expected results on Android? [core] Introduce dedicated filter types for $type and $id special cases #7971 (comment) @ivovandongen

[ivovandongen]

Does the eq() syntax produce the expected results on Android

@1ec5 Yes (#7971 (comment))

... Android, will you get a TypeEqualsFilter under the hood

Yes, we use the conversion templates for filter conversion (and the rest of the style conversions) which will produce the TypeEqualsFilter in this case here.

Thanks for the heads up.