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

Add a filter option for GeoJSON sources #9864

Merged
merged 3 commits into from
Aug 5, 2020
Merged

Add a filter option for GeoJSON sources #9864

merged 3 commits into from
Aug 5, 2020

Conversation

mourner
Copy link
Member

@mourner mourner commented Jul 9, 2020

Introduces a filter option to GeoJSON sources that allows filtering out features prior to processing (e.g. before clustering) with an expression. Closes #2613.

Launch Checklist

  • briefly describe the changes in this PR
  • write tests for all new functionality
  • document any changes to public APIs
  • manually test the debug page
  • tagged @mapbox/map-design-team @mapbox/static-apis if this PR includes style spec API or visual changes (not sure if that's relevant to these teams since it only targets GeoJSON)
  • tagged @mapbox/gl-native (needs a native port)
  • apply changelog label ('bug', 'feature', 'docs', etc) or use the label 'skip changelog'
  • add an entry inside this element for inclusion in the mapbox-gl-js changelog: <changelog>Add a filter option to GeoJSON sources.</changelog>

@mourner mourner requested a review from asheemmamoowala July 9, 2020 16:32
@mourner mourner self-assigned this Jul 10, 2020
Copy link
Contributor

@asheemmamoowala asheemmamoowala left a comment

Choose a reason for hiding this comment

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

@mourner This works fine, but is missing the ability to update the filter dynamically. Without that, it becomes difficult to use it in many situations where data has to be filtered based on user input.

What do you think about adding a GeoJSONSource#setFilter as well? Since the filtering is applied on the worker, there's the question of whether the worker source also needs to hold an unfiltered copy of the source data.

For geojson-vt, the filtering could be applied when retrieving a tile - making filter updates quite cheap. For supercluster, Im not sure that options would work.

@mourner
Copy link
Member Author

mourner commented Jul 16, 2020

@asheemmamoowala I've touched on this in my comment here #2613 (comment). Basically we only need source-level filtering for clustering use cases, because we have to do it prior to clustering (which needs to run on all of the data), and other use cases such as geojson-vt are covered with layer-level filtering. We could add setFilter for convenience (and Supercluster already retains a reference to all the original points), but functionally it shouldn't be much different from removing the source and adding a new one with the same id.

@asheemmamoowala
Copy link
Contributor

we only need source-level filtering for clustering use cases, because we have to do it prior to clustering

Should this be restricted to clusterFilter then so that there isn't confusion over which to use for non-clustered GeoJSON sources.

#2613 (comment) suggests that:

, and the file by the URL is already cached by the browser.

But this would only work when using a geoJSON by URL, not GeoJSON data, where an application might also be updating the feature dynamically

We could add setFilter for convenience (and Supercluster already retains a reference to all the original points), but functionally it shouldn't be much different from removing the source and adding a new one with the same id.

This can be added in the future if necessary.

Copy link
Contributor

@asheemmamoowala asheemmamoowala left a comment

Choose a reason for hiding this comment

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

Good to go pending tests.

@ryanhamley
Copy link
Contributor

Reminder to @mourner that this PR just needs some tests

@mourner mourner merged commit ab7dae3 into main Aug 5, 2020
@mourner mourner deleted the geojson-filter branch August 5, 2020 17:37
@enersis-pst
Copy link
Contributor

enersis-pst commented Feb 25, 2021

like i understand there is no possibility to update the source filter, right? Only by removing and adding a new source is not a solution for me, because therefore i also need to remove an recreate all depening layers.

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.

Support preclustering filtering
4 participants