Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support expressions in filters #5193

Closed
wants to merge 7 commits into from
Closed

Conversation

anandthakker
Copy link
Contributor

@anandthakker anandthakker commented Aug 26, 2017

Closes #4077
Closes #4410

cc @jfirebaugh @aparlato

@indus
Copy link
Contributor

indus commented Aug 28, 2017

Will there be an expression for "object.hasOwnProperty()" or "array.indexOf() != -1". I'm living with a patched version since #4490

@anandthakker
Copy link
Contributor Author

@indus Good question - there is a ["has", "propertyName"] expression (see #4777), but we should also add [ "contains" , ... ] for checking presence in an array.

@anandthakker
Copy link
Contributor Author

@aparlato @nickidlugash updated with:

  • support for [zoom]
  • [contains]

Still working on performance but it should be usable for developing styles.

@anandthakker anandthakker force-pushed the expressions-filter branch 2 times, most recently from 502dd15 to 9237f07 Compare August 31, 2017 19:09
@anandthakker anandthakker force-pushed the feature/expressions branch 2 times, most recently from d68949a to 9ee7362 Compare August 31, 2017 19:47
@anandthakker anandthakker changed the base branch from feature/expressions to master August 31, 2017 19:51
@anandthakker anandthakker force-pushed the expressions-filter branch 2 times, most recently from c658afb to e695aa5 Compare September 3, 2017 22:03
@anandthakker
Copy link
Contributor Author

Rebased onto master and reworked to take advantage of #5234 -- performance is much better, but evaluation performance is still about 8x compared to non-expressions-based filters:

screen shot 2017-09-28 at 3 52 25 pm

And creation performance is much slower:
screen shot 2017-09-28 at 3 52 10 pm

@anandthakker
Copy link
Contributor Author

Along the lines of #5379 , @mourner suggests leaving the existing filter implementation in place, and making expressions support additive / opt-in.

anandthakker pushed a commit that referenced this pull request Oct 5, 2017
anandthakker pushed a commit that referenced this pull request Oct 5, 2017
anandthakker pushed a commit that referenced this pull request Oct 5, 2017
anandthakker added a commit that referenced this pull request Oct 5, 2017
* Add heatmap-density expression (prepares for #5253)

* Accept options for context in which expression is used

Prepares for #5193

* Unify creatExpression and createExpressionWithErrorHandling

Closes #5409

* declaration => property
@anandthakker
Copy link
Contributor Author

Back to being a bit worse after rebasing onto the recent updates on master (no-eval, etc.):

Bench master expressions-filter gist
FilterCreate 0.303 ms 9.81 ms ( +9.51 ms / 15.1 std devs ) https://bl.ocks.org/anonymous/raw/f98f920d15e3df367eb5175987548939/
FilterEvaluate 0.136 ms 2.93 ms ( +2.79 ms / 37.3 std devs ) https://bl.ocks.org/anonymous/raw/b42d635f2eda75b0627f95801985d7ba/

@anandthakker
Copy link
Contributor Author

cc @mourner @jfirebaugh I was hoping the recent changes would mean we could stick with converting old-style filters, but looks like we should drop the conversion for now.

@jfirebaugh
Copy link
Contributor

https://github.com/mapbox/mapbox-gl-js/compare/expressions-filter-perf gets it down to 3.5-4x slower.

@jfirebaugh
Copy link
Contributor

I merged the alternative without conversion, #5434. We'll hopefully be resurrecting the conversion code from this PR at some point, but closing for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow null as a filter value Allow arbitrary expressions in functions and filters
3 participants