[core] Add expression filter support #11251

lucaswoj · 2018-02-20T00:54:43Z

Resolves #10720

GL JS equivalent mapbox/mapbox-gl-js#5193

Todo

Todo in Subsequent PRs

Write integration tests
Convert "legacy" filters directly into expressions
- Port the specialized filter-* expressions from GL JS, adding them to src/mbgl/style/expression/compound_expression.cpp.
- Update src/mbgl/style/conversion/filter.cpp to always convert incoming JSON values to an ExpressionFilter (using the filter-* types as needed for legacy syntax)
- Rewrite/simplify include/mbgl/style/filter.hpp and mbgl::style::FilterEvaluator, removing all the different *Filter types and the Filter variant and renaming ExpressionFilter to Filter.

anandthakker

👍 looking good to me!

anandthakker · 2018-02-20T14:56:44Z

include/mbgl/style/filter.hpp

+    std::shared_ptr<expression::Expression> expression;
+
+    friend bool operator==(const ExpressionFilter& lhs, const ExpressionFilter& rhs) {
+        return lhs.expression == rhs.expression;


Note that this is checking the pointers rather than the expression objects themselves. (*(lhs.expression) == *(rhs.expression) to do a deep equality check)

Are you sure? I can't find a definitive answer but Googling around seems to indicate C++ will use ExpressionFilter::operator== rather than an address check.

Pretty sure -- according to http://en.cppreference.com/w/cpp/memory/shared_ptr/operator_cmp, shared_ptr::operator== simply compares the wrapped Expression* pointers, as opposed to invoking Expression::operator==(). Confirmed here: http://cpp.sh/7yskf

Oh, doh, I was thinking about the ExpressionFilter&s, not the shared_ptr<Expression>s 😄

anandthakker · 2018-02-20T14:56:54Z

include/mbgl/style/filter.hpp

+
+class ExpressionFilter {
+public:
+    std::shared_ptr<expression::Expression> expression;


Could this be a unique_ptr?

I think so. Will try that.

Update: this won't work because ExpressionFilter needs to be copyable

Can it be std::shared_ptr<const expression::Expression>?

anandthakker · 2018-02-20T14:59:52Z

src/mbgl/style/conversion/stringify.hpp

@@ -225,6 +225,10 @@ class StringifyFilter {
    void operator()(const NotHasIdentifierFilter&) {
        stringifyUnaryFilter("!has", "$id");
    }
+
+    void operator()(const ExpressionFilter&) {
+        stringifyUnaryFilter("herp", "derp");


#11156 should make this fairly straightforward

anandthakker · 2018-02-20T15:06:08Z

include/mbgl/style/filter.hpp

-
-    template <class PropertyAccessor>
-    bool operator()(FeatureType type, optional<FeatureIdentifier> id, PropertyAccessor accessor) const;
+    bool operator()(expression::EvaluationContext context) const;


Should we remove the bool operator()(const GeometryTileFeature&) const overload?

Absolutely 👍

1ec5 · 2018-02-26T18:35:44Z

platform/darwin/src/NSPredicate+MGLAdditions.mm

@@ -194,6 +194,10 @@
    NSPredicate *operator()(mbgl::style::NotHasIdentifierFilter filter) {
        return [NSPredicate predicateWithFormat:@"%K == nil", @"$id"];
    }
+
+    NSPredicate *operator()(mbgl::style::ExpressionFilter filter) {
+        return nil;


Once #11254 lands, the implementation should look something like this:

id jsonObject = MGLJSONObjectFromMBGLExpression(filter.expression); return [NSPredicate mgl_predicateWithJSONObject:jsonObject];

lucaswoj · 2018-02-26T20:39:12Z

src/mbgl/geometry/feature_index.cpp

@@ -125,7 +125,7 @@ void FeatureIndex::addFeature(
            continue;
        }

-        if (options.filter && !(*options.filter)(*geometryTileFeature)) {
+        if (options.filter && !(*options.filter)(style::expression::EvaluationContext { geometryTileFeature.get() })) {


lucaswoj · 2018-02-26T20:39:20Z

src/mbgl/layout/symbol_layout.cpp

@@ -100,7 +100,7 @@ SymbolLayout::SymbolLayout(const BucketParameters& parameters,
    const size_t featureCount = sourceLayer->featureCount();
    for (size_t i = 0; i < featureCount; ++i) {
        auto feature = sourceLayer->getFeature(i);
-        if (!leader.filter(feature->getType(), feature->getID(), [&] (const auto& key) { return feature->getValue(key); }))
+        if (!leader.filter(expression::EvaluationContext { feature.get() }))


lucaswoj · 2018-02-26T20:39:29Z

src/mbgl/tile/custom_geometry_tile.cpp

@@ -79,7 +79,7 @@ void CustomGeometryTile::querySourceFeatures(
            auto feature = layer->getFeature(i);

            // Apply filter, if any
-            if (queryOptions.filter && !(*queryOptions.filter)(*feature)) {
+            if (queryOptions.filter && !(*queryOptions.filter)(style::expression::EvaluationContext { feature.get() })) {


lucaswoj · 2018-02-26T20:39:35Z

src/mbgl/tile/geojson_tile.cpp

@@ -30,7 +30,7 @@ void GeoJSONTile::querySourceFeatures(
                auto feature = layer->getFeature(i);

                // Apply filter, if any
-                if (options.filter && !(*options.filter)(*feature)) {
+                if (options.filter && !(*options.filter)(style::expression::EvaluationContext { feature.get() })) {


lucaswoj · 2018-02-26T20:39:41Z

src/mbgl/tile/geometry_tile.cpp

@@ -281,7 +281,7 @@ void GeometryTile::querySourceFeatures(
                auto feature = layer->getFeature(i);

                // Apply filter, if any
-                if (options.filter && !(*options.filter)(*feature)) {
+                if (options.filter && !(*options.filter)(style::expression::EvaluationContext { feature.get() })) {


lucaswoj · 2018-02-26T20:39:47Z

src/mbgl/tile/geometry_tile_worker.cpp

@@ -339,7 +339,7 @@ void GeometryTileWorker::redoLayout() {
            for (std::size_t i = 0; !obsolete && i < geometryLayer->featureCount(); i++) {
                std::unique_ptr<GeometryTileFeature> feature = geometryLayer->getFeature(i);

-                if (!filter(feature->getType(), feature->getID(), [&] (const auto& key) { return feature->getValue(key); }))
+                if (!filter(expression::EvaluationContext { feature.get() }))


jfirebaugh · 2018-03-02T19:45:52Z

Add a Layer::getFilter method per Add expression filter support #10720 (comment)

Background, raster, and custom layers do not have a filter, so we've avoided adding it to the base class. The appropriate concrete types already do have a getFilter accessor.

jfirebaugh

This looks good, nice work! Just minor points.

jfirebaugh · 2018-03-02T19:33:14Z

include/mbgl/style/filter.hpp

+
+class ExpressionFilter {
+public:
+    std::shared_ptr<expression::Expression> expression;


Can it be std::shared_ptr<const expression::Expression>?

jfirebaugh · 2018-03-02T19:34:41Z

include/mbgl/style/filter.hpp

-
-    template <class PropertyAccessor>
-    bool operator()(FeatureType type, optional<FeatureIdentifier> id, PropertyAccessor accessor) const;
+    bool operator()(expression::EvaluationContext context) const;


const expression::EvaluationContext& to avoid a copy.

jfirebaugh · 2018-03-02T19:36:53Z

src/mbgl/style/conversion/filter.cpp

+
+    optional<std::string> op = toString(arrayMember(filter, 0));
+
+    if (*op == "has") {


Needs a guard conditional for if the first member is not a string (the GL JS implementation handles this implicitly in the default case).

jfirebaugh · 2018-03-02T19:37:14Z

src/mbgl/style/conversion/filter.cpp

+
+    if (*op == "has") {
+        auto operand = toString(arrayMember(filter, 1));
+        return arrayLength(filter) >= 2 && operand && *operand != "$id" && *operand != "$type";


The arrayLength check needs to come before arrayMember. Also needs a guard check for string type.

👍

optional<std::string> operand = toString(arrayMember(filter, 1)); return operand && *operand != "$id" && *operand != "$type";

My understanding is that checking the truthiness of operand serves as a guard check for string type. Is that the case?

Oh, you're right, I missed that condition.

lucaswoj · 2018-03-06T20:20:51Z

@jfirebaugh @anandthakker Is this ready to 🚢 🍏 pending a final sanity test?

anandthakker

LGTM 🚀 🐑 !

jfirebaugh · 2018-03-05T22:48:57Z

include/mbgl/style/filter.hpp

@@ -274,7 +274,7 @@ using FilterBase = variant<
 class Filter : public FilterBase {
 public:
    using FilterBase::FilterBase;
-    bool operator()(expression::EvaluationContext context) const;
+    bool operator()(const expression::EvaluationContext &context) const;


Nit: const expression::EvaluationContext& context (spacing around & is not always consistent due to history, but this is our preferred style).

…lter and use it to decide in Converter<Filter>::operator() whether to parse the incoming JSON as an ExpressionFilter or one of the legacy filter types

…licable

…er)`

jfirebaugh · 2018-03-09T00:27:41Z

I missed that this was targeted at master. Can you please open a PR cherry-picking it to boba-release?

@anandthakker

* WIP * WIP * WIP * Remove Filter::operator()(const Feature&) * WIP * WIP * WIP * WIP * Hook up expression filter evaluator * Replace `shared_ptr` with &reference * Fill in implementation of `void operator()(const ExpressionFilter&)` * Fix failing tests * Switch back to a shared_ptr per chat with @anandthakker * Fix benchmark compilation * Shot in the dark to fix CI * Shot in the dark to fix CI (part 2) * Shot in the dark to fix CI (part 3) * In src/mbgl/style/conversion/filter.cpp, add a port of isExpressionFilter and use it to decide in Converter<Filter>::operator() whether to parse the incoming JSON as an ExpressionFilter or one of the legacy filter types * Remove bool Filter::operator()(const GeometryTileFeature&) const * Ensure the map zoom is passed into filtering operations wherever applicable * Add expression filter tests * Addressed PR feedback * Implement `NSPredicate *operator()(mbgl::style::ExpressionFilter filter)` * Fix formatting& nit

@anandthakker

* WIP * WIP * WIP * Remove Filter::operator()(const Feature&) * WIP * WIP * WIP * WIP * Hook up expression filter evaluator * Replace `shared_ptr` with &reference * Fill in implementation of `void operator()(const ExpressionFilter&)` * Fix failing tests * Switch back to a shared_ptr per chat with @anandthakker * Fix benchmark compilation * Shot in the dark to fix CI * Shot in the dark to fix CI (part 2) * Shot in the dark to fix CI (part 3) * In src/mbgl/style/conversion/filter.cpp, add a port of isExpressionFilter and use it to decide in Converter<Filter>::operator() whether to parse the incoming JSON as an ExpressionFilter or one of the legacy filter types * Remove bool Filter::operator()(const GeometryTileFeature&) const * Ensure the map zoom is passed into filtering operations wherever applicable * Add expression filter tests * Addressed PR feedback * Implement `NSPredicate *operator()(mbgl::style::ExpressionFilter filter)` * Fix formatting& nit

lucaswoj added Core The cross-platform C++ core, aka mbgl in progress labels Feb 20, 2018

anandthakker reviewed Feb 20, 2018

View reviewed changes

1ec5 reviewed Feb 26, 2018

View reviewed changes

lucaswoj force-pushed the expession-filter branch from 52e3666 to f1764dd Compare February 26, 2018 20:33

lucaswoj commented Feb 26, 2018

View reviewed changes

lucaswoj force-pushed the expession-filter branch from 4c59cdb to bf13252 Compare February 28, 2018 20:42

lucaswoj changed the title ~~Add expression filter support~~ [core] Add expression filter support Feb 28, 2018

jfirebaugh reviewed Mar 2, 2018

View reviewed changes

anandthakker approved these changes Mar 7, 2018

View reviewed changes

jfirebaugh approved these changes Mar 7, 2018

View reviewed changes

Lucas Wojciechowski added 12 commits March 7, 2018 11:46

WIP

68b25f2

WIP

aab8b15

WIP

bbdce80

Remove Filter::operator()(const Feature&)

c9618fd

WIP

9a51bc4

WIP

228ca2d

WIP

57e1df1

WIP

d4aac8e

Hook up expression filter evaluator

392fe11

Replace shared_ptr with &reference

4eae9bb

Fill in implementation of void operator()(const ExpressionFilter&)

9c55db8

Fix failing tests

bce45e8

Lucas Wojciechowski added 12 commits March 7, 2018 11:46

Switch back to a shared_ptr per chat with @anandthakker

f4eb02a

Fix benchmark compilation

bf59bc6

Shot in the dark to fix CI

6c1c93f

Shot in the dark to fix CI (part 2)

fa9ae2a

Shot in the dark to fix CI (part 3)

562d553

In src/mbgl/style/conversion/filter.cpp, add a port of isExpressionFi…

3246b7d

…lter and use it to decide in Converter<Filter>::operator() whether to parse the incoming JSON as an ExpressionFilter or one of the legacy filter types

Remove bool Filter::operator()(const GeometryTileFeature&) const

00b5c27

Ensure the map zoom is passed into filtering operations wherever app…

777dd63

…licable

Add expression filter tests

bd9e8b6

Addressed PR feedback

c3eafed

Implement `NSPredicate *operator()(mbgl::style::ExpressionFilter filt…

a50c9f8

…er)`

Fix formatting& nit

594bfd1

lucaswoj force-pushed the expession-filter branch from 45b0db2 to 594bfd1 Compare March 7, 2018 19:46

lucaswoj merged commit 5b3ebc4 into master Mar 8, 2018

lucaswoj deleted the expession-filter branch March 8, 2018 23:48

lucaswoj mentioned this pull request Mar 9, 2018

Add integration tests for legacy and expression filters mapbox/mapbox-gl-js#6304

Merged

1 task

tobrun mentioned this pull request Mar 9, 2018

Cherry pick expression filters #11428

Merged

tobrun mentioned this pull request Mar 9, 2018

Expression filters #11429

Merged

9 tasks

1ec5 mentioned this pull request Mar 30, 2018

Support expression operators in style layer predicates #11567

Closed

mollymerp mentioned this pull request Oct 15, 2018

Add sdk-support entry for filter mapbox/mapbox-gl-js#7374

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[core] Add expression filter support #11251

[core] Add expression filter support #11251

lucaswoj commented Feb 20, 2018 •

edited

Loading

anandthakker left a comment

anandthakker Feb 20, 2018

lucaswoj Feb 20, 2018

anandthakker Feb 20, 2018

lucaswoj Feb 20, 2018

anandthakker Feb 20, 2018

lucaswoj Feb 20, 2018

lucaswoj Feb 26, 2018

jfirebaugh Mar 2, 2018

anandthakker Feb 20, 2018

anandthakker Feb 20, 2018

lucaswoj Feb 20, 2018

1ec5 Feb 26, 2018

lucaswoj Feb 26, 2018 •

edited

Loading

lucaswoj Feb 26, 2018 •

edited

Loading

lucaswoj Feb 26, 2018 •

edited

Loading

lucaswoj Feb 26, 2018 •

edited

Loading

lucaswoj Feb 26, 2018 •

edited

Loading

lucaswoj Feb 26, 2018 •

edited

Loading

jfirebaugh commented Mar 2, 2018

jfirebaugh left a comment

jfirebaugh Mar 2, 2018

jfirebaugh Mar 2, 2018

jfirebaugh Mar 2, 2018

jfirebaugh Mar 2, 2018

lucaswoj Mar 2, 2018

jfirebaugh Mar 2, 2018

lucaswoj commented Mar 6, 2018

anandthakker left a comment •

edited

Loading

jfirebaugh Mar 5, 2018

jfirebaugh commented Mar 9, 2018


		optional<std::string> op = toString(arrayMember(filter, 0));

		if (*op == "has") {

[core] Add expression filter support #11251

[core] Add expression filter support #11251

Conversation

lucaswoj commented Feb 20, 2018 • edited Loading

Todo

Todo in Subsequent PRs

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucaswoj Feb 26, 2018 • edited Loading

Choose a reason for hiding this comment

lucaswoj Feb 26, 2018 • edited Loading

Choose a reason for hiding this comment

lucaswoj Feb 26, 2018 • edited Loading

Choose a reason for hiding this comment

lucaswoj Feb 26, 2018 • edited Loading

Choose a reason for hiding this comment

lucaswoj Feb 26, 2018 • edited Loading

Choose a reason for hiding this comment

lucaswoj Feb 26, 2018 • edited Loading

Choose a reason for hiding this comment

jfirebaugh commented Mar 2, 2018

jfirebaugh 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

lucaswoj commented Mar 6, 2018

anandthakker left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jfirebaugh commented Mar 9, 2018

lucaswoj commented Feb 20, 2018 •

edited

Loading

lucaswoj Feb 26, 2018 •

edited

Loading

lucaswoj Feb 26, 2018 •

edited

Loading

lucaswoj Feb 26, 2018 •

edited

Loading

lucaswoj Feb 26, 2018 •

edited

Loading

lucaswoj Feb 26, 2018 •

edited

Loading

lucaswoj Feb 26, 2018 •

edited

Loading

anandthakker left a comment •

edited

Loading