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

Null values should be allowed #62

Open
jfirebaugh opened this issue Feb 22, 2016 · 6 comments
Open

Null values should be allowed #62

jfirebaugh opened this issue Feb 22, 2016 · 6 comments

Comments

@jfirebaugh
Copy link
Contributor

2.1 specifies:

In order to support values of varying string, boolean, integer, and floating point types, the protobuf encoding of the value field consists of a set of optional fields. A value MUST contain exactly one of these optional fields.

The text "MUST contain exactly one" effectively prohibits null values. I think this was unintended, and the text should read "MUST contain either zero of these optional fields, in which case the value shall be interpreted as an untyped "null" value, or exactly one of them."

@flippmoke
Copy link
Member

@jfirebaugh I think this would be acceptable within a minor bump, but I do have some reserves about doing so. Currently mapnik-vector-tile is the only encoder/decoder that has become v2 compatible I think it would okay to update this as a minor bump, but I don't want to be changing too much as times goes on like this as it could break backwards compatibility!

@lucaswoj
Copy link

What is the semantic difference between a null value and a missing key in a vector tile feature?

@flippmoke
Copy link
Member

@lucaswoj it would really depend on what someone was trying to represent, it might have a different meaning. I do not see any harm in allowing this as an option.

@springmeyer
Copy link

@jfirebaugh the original design intention was that null values would not be encoded to gain the space savings. So, a missing key is intended to be a substitute for a null value.

@jfirebaugh
Copy link
Contributor Author

Thanks for clarifying the design intent @springmeyer.

To me, the main motivation for supporting null values is one less GeoJSON mismatch. I.e., in a GeoJSON Feature you can have:

"properties": {}

and

"properties": {
  "foo": null
}

The current VT spec cannot distinguish these two, or round trip the latter.

(Non-scalar GeoJSON property values also can't be losslessly VT-encoded, but supporting null would be a lot lower cost for the specification and implementations than supporting non-scalar values.)

@fosskers
Copy link

Must compatibility with GeoJSON be maintained? null gets a lot of bad press, and for good reason. I'd prefer the idea of null being left out.

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

No branches or pull requests

5 participants