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 'to-json' expression #7796

Closed
wants to merge 4 commits into from
Closed

Conversation

stepankuzmin
Copy link
Contributor

@stepankuzmin stepankuzmin commented Jan 22, 2019

Hi there!

Sometimes there is a need to encode some structured data in a feature property. Current vector tiles spec does not support compound properties, such as arrays and objects. We can encode such properties as a stringified JSON.

This PR adds to-json expression, that allows parsing stringified JSON.

  • briefly describe the changes in this PR
  • write tests for all new functionality
  • document any changes to public APIs
  • manually test the debug page

Consider example:

If you have a features with stringified object or array properties, you can use the following expressions to obtain JSON values:

["object", ["to-json", ["get", "stringified-object-property"]]]
["array", ["to-json", ["get", "stringified-array-property"]]]

What do you think of this?

@asheemmamoowala
Copy link
Contributor

Thanks for this idea and implementation @stepankuzmin ! The native implementation of expressions does not have the same JSON support as provided by JSON.parse provides in the browser.
@brunoabinader @1ec5 @tobrun Would it be possible for the platform specific ConversionTraits to parse a stringified JSON object?

@1ec5
Copy link
Contributor

1ec5 commented Jan 23, 2019

That is possible in Objective-C, via the NSJSONSerialization class.

@kkaefer
Copy link
Contributor

kkaefer commented Jan 25, 2019

That is possible in Objective-C, via the NSJSONSerialization class.

Given that expression evaluation is happening in C++ code anyway, and we already have a JSON parser in there, we'd just parse the JSON code there.

Taking a step back though, I'm not convinced that adding the ability to parse JSON fosters good vector tile design, since it usually means that you haven't spent much time thinking about your data model. You're right in pointing out that vector tiles currently don't support nested properties and arrays, which make some data models complicated to implement in vector tiles. However, instead of JSON support, we should work towards mapbox/vector-tile-spec#75.

@nextstopsun
Copy link
Contributor

This is a long awaited one, it is often needed to include json structured data in feature properties. I.e. in my case I'm packing an array of numbers into a property, so parsing this encoded string to an object is a first step to working with arrays - values and indexes. In conjunction with this feature it becomes extremely useful. Surely there are more use cases for this.

Please merge for greater good.

@stepankuzmin
Copy link
Contributor Author

...adding the ability to parse JSON fosters good vector tile design, since it usually means that you haven't spent much time thinking about your data model

I agree with that, but sometimes it's better to encode attribute into an array instead of using a list of flat properties, which may lead to bloated tiles.

@nextstopsun
Copy link
Contributor

Any news here?
Waiting so much for this to land in master.

@stepankuzmin
Copy link
Contributor Author

Hi everyone! I've added tests for to-json expression, so this PR is no longer WIP.

@stepankuzmin stepankuzmin changed the title [WIP] Add 'to-json' expression Add 'to-json' expression Feb 18, 2019
@stdmn stdmn mentioned this pull request Feb 21, 2019
6 tasks
@stdmn
Copy link

stdmn commented Apr 15, 2019

Would love to see this get merged

@stdmn
Copy link

stdmn commented May 14, 2019

Bump to see this merged. Really important for a project I'm working on.

@nextstopsun
Copy link
Contributor

Still hoping to see this merged.
@stepankuzmin can you please resolve those conflicts?

@vakila
Copy link

vakila commented Nov 1, 2019

Hi @stepankuzmin thanks again for your suggestion of this feature & your very diligent & thorough work on this PR! And thanks everyone else for supporting this - We so appreciate your contributions to this project & discussions around its feature roadmap. We have discussed this suggestion at length as a team, so let me sum up our thoughts & conclusions here.

We hear all of your frustrations with the lack of structured data support in tile properties, and we hear your vocal support for this expression. But as @kkaefer explained above, adding JSON parsing would only be a workaround for the real problem. The best long-term solution will be actual support for structured data in the source itself, and work is already underway on other teams at Mapbox to solve that & related problems in a holistic way.

That said, even if a to-json expression would provide a workaround, we have several concerns about the consequences of adding this expression, which would necessitate more work than simply merging this change. JSON parsing can get tricky; a lot can go wrong, and it can be hard to debug exactly what the issue is. Additional safeguards, like limits on the number of bytes a JSON string may have, would need to be put in place. There's also a lack of additional syntax for querying JSON; we currently only have get, but a syntax like that of jq might need to be implemented next. And more generally, fully supporting this functionality would not stop with merging it into mapbox-gl-js; long-term, Mapbox would have to implement & support this throughout our entire ecosystem (Native SDKs, Studio, Tilesets API, etc.).

All of this brings us to the conclusion that a to-json expression opens a bit of a Pandora's box and could become a long-term source of more complexity than we are able to justify for this feature, so we won't accept it at this point. But once again, our sincere thanks for your work on this and for sparking & engaging in this discussion. ❤️

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

Successfully merging this pull request may close these issues.

7 participants