Skip to content
This repository has been archived by the owner on Apr 10, 2018. It is now read-only.

Allow null as a filter operand #421

Closed
wants to merge 2 commits into from
Closed

Allow null as a filter operand #421

wants to merge 2 commits into from

Conversation

lucaswoj
Copy link

fixes mapbox/mapbox-gl-js#2162

cc @jfirebaugh @ansis @tmcw @mourner

Is this the right solution?

JSON doesn't allow undefined, only null.

@jfirebaugh
Copy link
Contributor

Does ["==", key, null] mean properties[key] === null or !(key in properties)? I think the latter is what's desired for the example in question.

The current vector tile specification appears to prohibit a null value, which might appear to make this distinction moot: if a null value is not possible, then properties.key === null is always false, and !(key in properties) seems like the only reasonable interpretation. However, I think prohibiting null is an oversight in the vector tile specification, and it intended to permit a distinction between "property isn't present" and "property is present, but null valued". And certainly GeoJSON has this distinction, and filters should support filtering GeoJSON sources.

Therefore we need unambiguous ways to ask for:

  • key in properties
  • !(key in properties)
  • properties[key] === null
  • properties[key] !== null

Let's reserve ["==", key, null] and ["!=", key, null] for the second two, and add "has" and "!has" operators for the first two as suggested in #364.

@jfirebaugh
Copy link
Contributor

@lucaswoj
Copy link
Author

Thank you @jfirebaugh.

If there is a chance that null values will be added to the vector tile spec in the future, then I agree that we should go the route of has/!has instead.

If there is little to no chance that null values will be added to the vector tile spec in the future, then sticking with this implementation is desirable because it works more elegantly with in/!in.

@jfirebaugh
Copy link
Contributor

We should both merge this PR (for null value support), and add has / !has. I don't think that answer depends on whether null values are added to the vector tile spec in the future: they are already possible via GeoJSON.

As far as this PR goes, the code changes look good. It should also update the documentation for filters, which list the supported types as "string, number, or boolean" (with broken links). Then we need changes and tests for feature-filter and the native implementation. And we should think about whether these additions are compatible with a non-major version style spec release.

@lucaswoj
Copy link
Author

Added has / !has operators to this PR. 👀 @jfirebaugh

I don't think that answer depends on whether null values are added to the vector tile spec in the future: they are already possible via GeoJSON.

What happens to null values in GeoJSON after they are passed through geojson-vt? Aren't they restricted in the same way as any other vector tile?

It should also update the documentation for filters, which list the supported types as "string, number, or boolean" (with broken links).

👍 Done

Then we need changes and tests for feature-filter and the native implementation.

Tracking in mapbox/DEPRECATED-mapbox-gl#10

And we should think about whether these additions are compatible with a non-major version style spec release.

I think a minor release is appropriate. We're adding a new feature, not breaking any existing functionality.

  • using a null operand before was previously an error, now it works
  • using a has / !has operator was previously an error, now it works

@jfirebaugh
Copy link
Contributor

What happens to null values in GeoJSON after they are passed through geojson-vt? Aren't they restricted in the same way as any other vector tile?

geojson-vt passes the properties value through unchanged to the output (though in the output it's called tags instead). So not only is null allowed, so are objects! But I'm drawing the line at null -- I don't think we should support filtering by sub-object.

Everything else looks good. :shipit:

@lucaswoj
Copy link
Author

lucaswoj commented Jun 9, 2016

Resolved in #451

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

Successfully merging this pull request may close these issues.

Check all examples for style validation errors
2 participants