[core] Convert "legacy" filters directly into expressions #11610

lucaswoj · 2018-04-05T22:52:17Z

Follow up to #11429

Launch Checklist

lucaswoj · 2018-04-20T00:19:27Z

platform/android/src/style/layers/layer.cpp

+            mbgl::Value expressionValue = filter.expression.get()->serialize();
+            return gson::JsonElement::New(env, expressionValue);
+        } else {
+          return null;


Question for reviewers: Are we setting ourselves up for future NPEs by allowing filter.expression to be null? Is there a better solution?

lucaswoj · 2018-04-20T00:21:09Z

platform/glfw/glfw_view.cpp

+    args.push_back(std::make_unique<mbgl::style::expression::Literal>(mbgl::style::expression::Value(std::string("extrude"))));
+    args.push_back(std::make_unique<mbgl::style::expression::Literal>(mbgl::style::expression::Value(std::string("true"))));
+    mbgl::style::expression::ParsingContext parsingContext;
+    extrusionLayer->setFilter(mbgl::style::Filter { std::move(*mbgl::style::expression::createCompoundExpression("filter-==", std::move(args), parsingContext)) });


Question for future reviewer: should we create a more ergonomic syntax for instantiating expressions? Ideally something close to:

createExpression("filter-==", "extrude", "true")

I definitely agree that creating filters (and, more generally, expressions) is very cumbersome in mbgl core. I'm just not sure how important it is to streamline this internal API, since its primary consumer is really ParsingContext.

I ran into this while working on #11247 -- I wanted to completely remove the *Function constructors that take a Stops argument. But I gave up because it was sooo cumbersome to convert everything that used them to expressions. That included tests and a few other things that I can't recall at the moment.

So I think it would be worthwhile to provide a more convenient API at least for the subset of expression operators needed by legacy filters and tests. It will probably look similar to the convenience API that Android has.

Specifically, this constructor. There’s a similar thing on iOS/macOS with +[NSExpression expressionWithMGLJSONObject:] and/or the MGL_FUNCTION() format string syntax.

I added some convenience overloads to createCompoundExpression and added createLiteral function. These reduces the boilerplate significantly:

ParsingContext ctx; extrusionLayer->setFilter(Filter(createCompoundExpression("filter-==", createLiteral("extrude"), createLiteral("true"), ctx)));

lucaswoj · 2018-04-20T20:51:12Z

@anandthakker This ready for an initial review 😄

1ec5

Looks good on the iOS/macOS side.

1ec5 · 2018-04-20T23:18:51Z

platform/darwin/test/MGLPredicateTests.mm

-        }
-    }
-}
-
 #define MGLAssertEqualFilters(actual, expected, ...) \


This macro is unused.

anandthakker

@lucaswoj still in progress, but wanted to post these before I close my browser

anandthakker · 2018-04-24T08:57:42Z

include/mbgl/style/filter.hpp

-class Filter : public FilterBase {
-public:
-    using FilterBase::FilterBase;
+    Filter():expression({}) {}


Instead of using nullptr to model the absence of a filter, should we have expression be optional<std::shared_ptr<Expression>>?

Nit: surround the : with spaces (here and just below)

Is there an advantage to using optional instead of a nullptr? For better or for worse, unique_ptr has "optional" functionality built in and every callsite ought to handle it gracefully.

anandthakker · 2018-04-24T09:41:50Z

src/mbgl/style/conversion/filter.cpp


 namespace mbgl {
 namespace style {
 namespace conversion {

-using GeometryValue = mapbox::geometry::value;
+static bool isExpression(const Convertible& filter);


Is this static necessary?

anandthakker · 2018-04-24T09:42:02Z

src/mbgl/style/conversion/filter.cpp


 namespace mbgl {
 namespace style {
 namespace conversion {

-using GeometryValue = mapbox::geometry::value;
+static bool isExpression(const Convertible& filter);
+    std::unique_ptr<expression::Expression> convertLegacyFilter(const Convertible& values, Error& error);


Nit: unnecessary indentation

anandthakker · 2018-04-24T09:44:21Z

src/mbgl/style/conversion/filter.cpp

+            error = { parsingContext.getCombinedErrors() };
+            return {};
+        }
+        expression = std::move(*parseResult);


Can this just be return { Filter { std::move(*parseResult) } };, given the if(!parseResult) check above?

anandthakker · 2018-04-24T10:00:04Z

src/mbgl/style/conversion/filter.cpp

+
+std::unique_ptr<expression::Expression> convertLiteral(const Convertible& convertible) {
+    optional<mapbox::geometry::value> value = toValue(convertible);
+    expression::Value expressionValue = value ? expression::toExpressionValue(*value) : expression::Null;


It looks like this will effectively coerce array or object values to expression::Null, because toValue is supposed to return "none" if the value is not a boolean, number, or string.

I think this means that instead of getting a validation error for ["==", "key", {"x": 1}], we'd get the equivalent of ["==", "key", null].

Ah. Just seeing the Literal::parse method now. That looks like the right one to use.

anandthakker · 2018-04-24T10:09:43Z

src/mbgl/style/conversion/filter.cpp

+            *op == "none" ? createExpression("!", createExpression("any", convertLegacyFilterArray(values, error, 1), error), error) :
+            *op == "in" ? convertLegacyInFilter(values, error) :
+            *op == "!in" ? createExpression("!", convertLegacyInFilter(values, error), error) :
+            *op == "has" ? convertLegacyHasFilter(*toString(arrayMember(values, 1)), error) :


What if there's an invalid filter of the form ["has", 1]?

Then everything will 💥 ! Will fix shortly.

anandthakker · 2018-04-25T09:07:44Z

Is there an advantage to using optional instead of a nullptr? For better or for worse, unique_ptr has "optional" functionality built in and every callsite ought to handle it gracefully.

🤷‍♂️ I can see it both ways. IMO, the benefit of consistently modeling maybe-an-expression with optional<std::unique_ptr<Expression>> // pointer is never null is that we then have a way of saying "definitely an expression", using std::unique_ptr<Expression>. I guess what we really want is optional<non_nullable_unique_ptr<Expression>>

anandthakker · 2018-04-25T09:23:28Z

src/mbgl/style/conversion/filter.cpp

-        return { IdentifierFilterType {} };
+std::unique_ptr<expression::Expression> convertLiteral(const Convertible& convertible, Error& error) {
+    expression::ParsingContext parsingContext;
+    expression::ParseResult parseResult = expression::Literal::parse(convertible, parsingContext);


I think this still does less type validation than was previously happening. For instance, Literal::parse() will successfully parse any string, number, boolean, or null value -- but previously, a legacy filter like ["==", "$type", null] would have caused the error, "value for $type filter must be Point, LineString, or Polygon"; and ones like ["==", "property_key", null] would have caused "filter expression value must be a boolean, number, or string".

anandthakker · 2018-04-25T09:35:06Z

src/mbgl/style/expression/compound_expression.cpp

+    define("filter-id->=", [](const EvaluationContext& params, double lhs) -> Result<bool> {
+        auto rhs = featureIdAsDouble(params);
+        return rhs ? rhs >= lhs : false;
+    });


We also need string overloads of these comparison operators

anandthakker · 2018-05-04T20:03:03Z

test/style/filter.test.cpp

+    ASSERT_TRUE(filter("[\"<=\", \"two\", \"2\"]", {{"two", std::string("2")}}));
+    ASSERT_FALSE(filter("[\"<\", \"two\", \"1\"]", {{"two", std::string("2")}}));
+    ASSERT_FALSE(filter("[\"==\", \"two\", 4]", {{"two", std::string("2")}}));
+}


GL JS has tests for the expression version of any and all (to make sure that we're correctly parsing them as expressions as opposed to legacy filters): https://github.com/mapbox/mapbox-gl-js/blob/95bb4c0bcbf2da406da28795baf6a325c189cc04/test/unit/style-spec/feature_filter.test.js#L27-L37 -- would it make sense to add those?

Addressed at: 01de0a9#diff-4574a618412773ee8823c6637c5221a2

anandthakker · 2018-05-04T20:05:12Z

include/mbgl/style/expression/compound_expression.hpp

@@ -141,6 +141,18 @@ ParseResult createCompoundExpression(const CompoundExpressionRegistry::Definitio
 ParseResult createCompoundExpression(const std::string& name,
                                     std::vector<std::unique_ptr<Expression>> args,
                                     ParsingContext& ctx);
+
+ParseResult createCompoundExpression(const std::string& name, ParsingContext& ctx);


Could we stick a quick comment here indicating that these overloads are here to avoid inconvenience of having to create the args vector before calling?

anandthakker · 2018-05-04T20:35:40Z

src/mbgl/style/conversion/filter.cpp

-        return { IdentifierFilterType {} };
+std::unique_ptr<Expression> convertLiteral(const Convertible& convertible, Error& error) {
+    ParsingContext parsingContext;
+    ParseResult parseResult = Literal::parse(convertible, parsingContext);


I think we've still got the issue described in #11610 (comment) : I think right now, this would allow ["==", "$type", null] or ["==", "property_key", null], whereas previously they would have caused errors.

Looking into the fix for $type, but the null property value case seems to be supported in gl-js: https://github.com/mapbox/mapbox-gl-js/blob/95bb4c0bcbf2da406da28795baf6a325c189cc04/test/unit/style-spec/feature_filter.test.js#L83-L84

Expression syntax also allows Null as a comparable type :

mapbox-gl-native/src/mbgl/style/expression/equals.cpp

Lines 44 to 49 in 542713f

static bool isComparableType(const type::Type& type) {

return type == type::String ||

type == type::Number ||

type == type::Boolean ||

type == type::Null;

}

I added some test to verify this issue, and there is a change in the error returned, but there is still protection from invalid filter structure. Even when the Literal is parsed, the filter expression will not find a matching definition in the expression registry and complain about incorrect type or number of arguments.

["==", "$type"] //"Expected 1 arguments, but found 0 instead." ["==", "$type", null] //"[1]: Expected string but found null instead." ["==", "$type, "Point", 1] //"Expected 1 arguments, but found 2 instead."

Ahhh, gotcha. I mistakenly thought that the filter-... expressions were accepting a Value argument, but yeah, as long as we're accepting/rejecting the same filters, I don't think it matters if the specific error message changed. 👍

anandthakker · 2018-05-04T20:46:13Z

src/mbgl/style/expression/compound_expression.cpp

    define("!", [](bool e) -> Result<bool> { return !e; });
-    
+


Nit: revert these whitespace changes -- I think we normally use Xcode's default, which to include indentation for blank lines.

As far as I know, the preference in mbgl is still to avoid introducing indented blank lines, while the preference in platform/{darwin,ios,macos}/ and Objective-C code elsewhere is to follow the Xcode default of indenting blank lines. It isn’t really worth fighting over invisible characters, but that goes both ways: not really worth stripping them out, and not really worth putting them back in. 🤷‍♂️

It isn’t really worth fighting over invisible characters, but that goes both ways: not really worth stripping them out, and not really worth putting them back in

Yeah, that's basically my stance, too: the particular preference(s) matter less than avoiding whitespace-only changes (like these ^) in diffs.

anandthakker

One nit in the unit test, but otherwise LGTM!

anandthakker · 2018-05-08T18:26:59Z

test/style/filter.test.cpp

+    ASSERT_TRUE(filter("[\"any\", true]"));
+    ASSERT_TRUE(filter("[\"any\",true, false]"));
+    ASSERT_TRUE(filter("[\"any\", true, true]"));
+}


I think these will still end up testing the non-expression path -- how about just using the ones from the test immediately above, but replacing "foo" with ["get", "foo"]?

…src/mbgl/style/expression/compound_expression.cpp

Fix android compillation

… need an arg vector

…use expression syntax, and port Fiter.EqualsNull test to match gl-js

…egory.

friedbunny · 2018-05-10T19:40:40Z

Does this need changelog entries?

1ec5 · 2018-05-11T01:04:23Z

Not sure. Is the change transparent to developers using the runtime styling API?

lucaswoj · 2018-05-11T01:36:34Z

Ideally. We are replacing all the filter evaluation logic so there’s a small chance that it’ll behave differently in some poorly defined edge cases. And the serialized output may be different. (Sent from a remote ridge in Kings Canyon National Park)

anandthakker · 2018-05-11T01:44:05Z

I concur with the man on the ridge (ahoy @lucaswoj!): it should be transparent (and if it isn’t then it’s a bug). On Thu, May 10, 2018 at 9:36 PM Lucas Wojciechowski < notifications@github.com> wrote:

…

Ideally. We are replacing all the filter evaluation logic so there’s a small chance that it’ll behave differently in some poorly defined edge cases. And the serialized output may be different. (Sent from a remote ridge in Kings Canyon National Park) > On May 10, 2018, at 6:04 PM, Minh Nguyễn ***@***.***> wrote: > > Not sure. Is the change transparent to developers using the runtime styling API? > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub < #11610 (comment)>, or mute the thread < https://github.com/notifications/unsubscribe-auth/AARK2q-xdi8HkcI4CrF5WPh7q62XxRYHks5txOOfgaJpZM4TJRiX >. > — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#11610 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEvmRxELDOp3ZlyGE9Rt6rpO1UNqksquks5txOsmgaJpZM4TJRiX> .

lucaswoj force-pushed the expression-filters-2 branch 2 times, most recently from 0b2cce0 to 4678133 Compare April 6, 2018 19:25

1ec5 mentioned this pull request Apr 11, 2018

[ios, macos] NSPredicate expression filters. #11587

Merged

4 tasks

lucaswoj force-pushed the expression-filters-2 branch 3 times, most recently from 4842bbd to ecf41db Compare April 17, 2018 23:01

lucaswoj force-pushed the expression-filters-2 branch 2 times, most recently from 621a177 to daaefc1 Compare April 19, 2018 23:37

lucaswoj commented Apr 20, 2018

View reviewed changes

lucaswoj force-pushed the expression-filters-2 branch from 2c8d523 to d588405 Compare April 20, 2018 19:25

1ec5 added this to the ios-v4.0.1 milestone Apr 20, 2018

lucaswoj force-pushed the expression-filters-2 branch from ab8cd73 to 4a4b44f Compare April 20, 2018 20:36

lucaswoj requested a review from anandthakker April 20, 2018 20:50

1ec5 mentioned this pull request Apr 20, 2018

Publicly expose NSPredicate-JSON conversion #11751

Closed

1ec5 approved these changes Apr 20, 2018

View reviewed changes

anandthakker reviewed Apr 24, 2018

View reviewed changes

anandthakker reviewed Apr 25, 2018

View reviewed changes

lucaswoj force-pushed the expression-filters-2 branch from d6dddde to d94cec4 Compare April 25, 2018 18:46

fabian-guerra assigned asheemmamoowala May 1, 2018

fabian-guerra mentioned this pull request May 1, 2018

[ios, macos] Make NSPredicate+MGLAdditions public. #11810

Merged

anandthakker suggested changes May 4, 2018

View reviewed changes

anandthakker approved these changes May 8, 2018

View reviewed changes

asheemmamoowala force-pushed the expression-filters-2 branch from 2ffbb69 to 364e9e3 Compare May 9, 2018 21:24

Lucas Wojciechowski added 5 commits May 9, 2018 16:06

Port the specialized filter-* expressions from GL JS, adding them to …

14a8427

…src/mbgl/style/expression/compound_expression.cpp

WIP

5dc0e0e

Make it compile!

d8a57ec

Make filter tests pass!

2652219

Fix render tests

aca5a8f

Lucas Wojciechowski and others added 22 commits May 9, 2018 16:08

Try to fix linux-gcc4.9-debug

720df3d

Fix Android CI

d2a003e

Fix android compillation

Misc code cleanup

f407f50

Revive Stringify.Filter test

b339ef0

Create convieneice Filter constructor

d043b22

Add convienience overload of createCompoundExpression which doesn't…

8dd21f0

… need an arg vector

Add createLiteral convenience method

5f69284

Use convenience constructors in glfw_view.cpp

f88c306

Revive all tests

41fcbe5

Fix CI

a9ab81a

Remove unnused macro

f42bfca

Fix GLFW app

5634dcd

Made suggested formatting tweaks

2436fa0

Use Literal::parse

d080248

Ensure property is a string in legacy has filters

9c96d8a

Refactor Filter to use an optional

9efdc55

Fix Darwin apps

953b755

Fix Android compillation

eef6f54

Add string overrides for <, >, <=, >= filter operators.

f2fde3c

Fix Android build

5cd6b47

Adress review: Comments + Tests

45afc8f

Fix Filter.AnyExpression and Filter.AllExpression tests to correctly …

0359c4c

…use expression syntax, and port Fiter.EqualsNull test to match gl-js

asheemmamoowala force-pushed the expression-filters-2 branch from 364e9e3 to 0359c4c Compare May 9, 2018 23:09

[ios, macos] Fix rebase conflict, use the appropriate NSPredicate cat…

fea129c

…egory.

asheemmamoowala merged commit a4e2c1a into release-boba May 10, 2018

asheemmamoowala deleted the expression-filters-2 branch May 10, 2018 19:39

	static bool isComparableType(const type::Type& type) {
	return type == type::String \|\|
	type == type::Number \|\|
	type == type::Boolean \|\|
	type == type::Null;
	}

[core] Convert "legacy" filters directly into expressions #11610

[core] Convert "legacy" filters directly into expressions #11610

Conversation

lucaswoj commented Apr 5, 2018 • edited Loading

Launch Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jfirebaugh Apr 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucaswoj commented Apr 20, 2018

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

anandthakker 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

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

anandthakker commented Apr 25, 2018

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

asheemmamoowala May 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1ec5 May 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anandthakker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

friedbunny commented May 10, 2018

1ec5 commented May 11, 2018

lucaswoj commented May 11, 2018 via email • edited Loading

anandthakker commented May 11, 2018 via email

lucaswoj commented Apr 5, 2018 •

edited

Loading

jfirebaugh Apr 23, 2018 •

edited

Loading

asheemmamoowala May 5, 2018 •

edited

Loading

1ec5 May 5, 2018 •

edited

Loading

lucaswoj commented May 11, 2018 via email •

edited

Loading