Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

[core] Fix using in expression with filter #16272

Merged
merged 3 commits into from
Mar 11, 2020
Merged

[core] Fix using in expression with filter #16272

merged 3 commits into from
Mar 11, 2020

Conversation

Chaoba
Copy link
Contributor

@Chaoba Chaoba commented Mar 5, 2020

Resolves #16262
mbgl::style::conversion::isExpression() always returns false for expression in, this PR fix this issue.

The test in added in android test app in this pr: mapbox/mapbox-gl-native-android#218

I'm not sure where should add test cases for this change, @alexshalamov any suggestions for it?

@Chaoba Chaoba requested review from 1ec5 and alexshalamov March 5, 2020 03:44
@chloekraw chloekraw changed the title [core] Not filter express in in filter. [core] Fix using in expression with filter Mar 5, 2020
Copy link
Contributor

@1ec5 1ec5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested changelog entry:

  • Fixed an issue where in expressions were interpreted as legacy filters.

src/mbgl/style/conversion/filter.cpp Show resolved Hide resolved
@1ec5
Copy link
Contributor

1ec5 commented Mar 5, 2020

I'm not sure where should add test cases for this change

mapbox/mapbox-gl-js#9374 adds relevant test cases to the style specification unit tests. Would it be feasible to update the submodule to include that change?

@alexshalamov
Copy link
Contributor

@Chaoba PR looks good, could you add render test for that in gl-js? Something similar to an example in #16262

@1ec5
Copy link
Contributor

1ec5 commented Mar 5, 2020

Unfortunately, rendering tests are now failing due to updating the GL JS submodule, which brought in a test case for mapbox/mapbox-gl-js#9198 along with the test case for mapbox/mapbox-gl-js#9374 that we wanted.

@alexshalamov
Copy link
Contributor

Unfortunately, rendering tests are now failing due to updating the GL JS submodule, which brought in a test case for mapbox/mapbox-gl-js#9198

@Chaoba could you please add failing tests related to mapbox/mapbox-gl-js#9198 test/integration/query-tests/evaluated/* to ignore list https://github.com/mapbox/mapbox-gl-native/blob/master/metrics/ignores/platform-all.json

@Chaoba
Copy link
Contributor Author

Chaoba commented Mar 6, 2020

@Chaoba PR looks good, could you add render test for that in gl-js? Something similar to an example in #16262

@alexshalamov Created a ticket in gl-js to track it.

@Chaoba
Copy link
Contributor Author

Chaoba commented Mar 10, 2020

@alexshalamov I didn't change these files, why sanity-checks failed?

@Chaoba
Copy link
Contributor Author

Chaoba commented Mar 11, 2020

The issue in this pr is that:

  1. If we update gl-js, the nitpick will fail.
  2. If we apply nitpick patch, then the test case will fail
2: [ RUN      ] Annotations.DebugSparse
2: ../test/src/mbgl/test/util.cpp:49: Failure
2: Expected: (pixels / (expected.size.width * expected.size.height)) <= (imageThreshold), actual: 0.015564 vs 0.0002
2: [  FAILED  ] Annotations.DebugSparse (103 ms)

@tmpsantos What should we do to let this pr merged?

Copy link
Contributor

@alexshalamov alexshalamov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, please remove gl-js bump commit and rebase to master

@1ec5
Copy link
Contributor

1ec5 commented Mar 11, 2020

The failing test came in through mapbox/mapbox-gl-js#9397 as a result of #16293, which has landed. It should pass once you rebase this branch onto master.

@chloekraw
Copy link
Contributor

chloekraw commented Mar 11, 2020

@tmpsantos thanks for rebasing! CI is passing, good to merge?

@tmpsantos tmpsantos merged commit 639434b into master Mar 11, 2020
@tmpsantos tmpsantos deleted the kl-filter-in branch March 11, 2020 19:53
@chloekraw chloekraw added the needs changelog Indicates PR needs a changelog entry prior to merging. label Mar 11, 2020
@chloekraw
Copy link
Contributor

Ack, forgot the changelog. @Chaoba would you mind adding an entry in a separate pr?

@pozdnyakov
Copy link
Contributor

Change log entry proposal

### Bugs

Fixed using of the `in` expression as a layer filter ([#16272](https://github.com/mapbox/mapbox-gl-native/pull/16272))

The bug was caused by `mbgl::style::conversion::isExpression()` always returning `false` for the `in` expression.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
needs changelog Indicates PR needs a changelog entry prior to merging.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to use in expression as layer filter
6 participants