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

Use Rison for the explore URL #5152

Closed
wants to merge 5 commits into from
Closed

Conversation

betodealmeida
Copy link
Member

Rison is a data serialization format optimized for compactness in URIs. Rison is a slight variation of JSON that looks vastly superior after URI encoding. Rison still expresses exactly the same set of data structures as JSON, so data can be translated back and forth without loss or guesswork.

The original website proposing Rison is dead, but there's an NPM module and I published prison (a Python encoder/decoder) to PyPI today.

The Rison-encoded explore URL is smaller and easier to read. Before:

http://0.0.0.0:8080/superset/explore/?form_data=%7B%22datasource%22%3A%2214__table%22%2C%22viz_type%22%3A%22table%22%2C%22granularity_sqla%22%3A%22__time%22%2C%22time_grain_sqla%22%3Anull%2C%22since%22%3A%227+days+ago%22%2C%22until%22%3A%22now%22%2C%22groupby%22%3A%5B%5D%2C%22metrics%22%3A%5B%22count%22%5D%2C%22percent_metrics%22%3A%5B%22count%22%5D%2C%22timeseries_limit_metric%22%3Anull%2C%22row_limit%22%3A10000%2C%22include_time%22%3Afalse%2C%22order_desc%22%3Atrue%2C%22all_columns%22%3A%5B%5D%2C%22order_by_cols%22%3A%5B%5D%2C%22table_timestamp_format%22%3A%22%25Y-%25m-%25d+%25H%3A%25M%3A%25S%22%2C%22page_length%22%3A0%2C%22include_search%22%3Afalse%2C%22table_filter%22%3Afalse%2C%22align_pn%22%3Afalse%2C%22color_pn%22%3Atrue%2C%22where%22%3A%22%22%2C%22having%22%3A%22%22%2C%22filters%22%3A%5B%5D%2C%22url_params%22%3A%7B%7D%7D

After:

http://0.0.0.0:8080/superset/explore/?form_data=align_pn:!f,all_columns:!(),color_pn:!t,datasource:%2714__table%27,filters:!(),granularity_sqla:__time,groupby:!(),having:%27%27,include_search:!f,include_time:!f,metrics:!(count),order_by_cols:!(),order_desc:!t,page_length:0,percent_metrics:!(count),row_limit:10000,since:%277%20days%20ago%27,table_filter:!f,table_timestamp_format:%27%Y-%m-%d%20%H:%M:%S%27,time_grain_sqla:!n,timeseries_limit_metric:!n,until:now,url_params:(),viz_type:table,where:%27%27

@codecov-io
Copy link

codecov-io commented Jun 7, 2018

Codecov Report

Merging #5152 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5152      +/-   ##
==========================================
- Coverage   77.48%   77.48%   -0.01%     
==========================================
  Files          44       44              
  Lines        8737     8735       -2     
==========================================
- Hits         6770     6768       -2     
  Misses       1967     1967
Impacted Files Coverage Δ
superset/views/core.py 74.64% <100%> (ø) ⬆️
superset/utils.py 88.66% <100%> (+0.13%) ⬆️
superset/viz.py 81.29% <0%> (-0.09%) ⬇️
superset/connectors/druid/models.py 80.49% <0%> (-0.05%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0545d11...9ce73a5. Read the comment docs.

@mistercrunch
Copy link
Member

READABLE URLS!

@mistercrunch
Copy link
Member

Did you have to modify anything from python-rison? We could ask the maintainer for write-rights and pypi access on rison too.

@hughhhh
Copy link
Member

hughhhh commented Jun 10, 2018

@betodealmeida thank you blessing us with this. CC.geoviz definitely needs it :P

@mistercrunch
Copy link
Member

This LGTM. It could be good to add an entry in the FAQ ("How are the URL formatted?"), or mentioning Rison somewhere else in the docs.

We should also do this in the dashboard app for consistency (doesn't have to be this PR)

@mistercrunch
Copy link
Member

Only thing is this may have some intricacies that are hard to predict (maybe around null, NaN, infinity handling). The assumption that this is a drop-in replacement may not be 100% right, but there's no way to know for sure but to test it extensively in a production-like env.

Tagging some of the Airbnb folks to make sure that they are on board and giving a heads up on this new URL format.
@graceguo-supercat @williaster @michellethomas @john-bodley

@hughhhh
Copy link
Member

hughhhh commented Jun 10, 2018

@betodealmeida @mistercrunch Couldn't we just host this under a v2 namespace for the api and port of over the code accordingly?

@graceguo-supercat
Copy link

graceguo-supercat commented Jun 11, 2018

screen shot 2018-06-11 at 12 15 30 pm

for url in iframe, i think it should keep the original version, so that the user (they don't have rison support) can still use as it.

somehow if i generate /r/3 type of url, i got ValueError: No JSON object could be decoded.

@williaster
Copy link
Contributor

Can you just elaborate on the pain points you're trying to solve with this? Is it for users or for devs? wasn't grace's PR for moving toward POST requests meant to address some of these?

It seems like there should be some strong reasons to introduce a 3rd system for encoding urls that will have to be supported indefinitely, particularly if there are risks of bugs with nulls etc and differences in handling these between the different formats. also do you think we should we be cautious in picking up a project that is dead/deprecated? presumably they stopped supporting it for some reason? (do you know why?)

@williaster
Copy link
Contributor

adding a third way of encoding urls seems particularly risky given our conversation from the last meetup that there are already multiple (fragile, complicated) places in the code that have to encode + decode form data, URLs, and query parameters.

@john-bodley
Copy link
Member

@betodealmeida/@mistercruch my concern with Rison is possibly migrating to a system which is potentially unsupported and has little traction, i.e., the last commit was almost one year ago and there's only ~ 100 stars. Sometimes a known, stable and well supported entity (even with its drawbacks) is more desirable than something more experimental.

@mistercrunch
Copy link
Member

@williaster the main driver is mostly readable, and maybe hackable urls. I really wish Rison was more popular / supported. Note that we could support it (@betodealmeida forked the repo and created a new Pypi entry)

@betodealmeida if we want this feature we should cherry-pick it in our production and run it for a while and come back on this PR to comment on it saying something like "This has been extensively tested in our production for a long time and all is ok, and we are committed to maintain Prison in the long term"

@betodealmeida
Copy link
Member Author

I'm committed to maintain the Python version. Last week I added support for Python 3, published it to PyPI with the name "prison", and I've requested ownership of the old package name, "rison" (pypi/warehouse#4140). The current code has some unit tests, and I'm planning to add the full suite from cPython.

The NPM version seems pretty popular, with ~5k weekly downloads. The last release was 4 years ago, but it's a fairly simple project that doesn't need updates. There are also Go, Erlang, PHP, .Net and Ruby versions.

@mistercrunch
Copy link
Member

How about bringing it into our staging and then production for a while and revisiting?

@stale
Copy link

stale bot commented Apr 10, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the inactive Inactive for >= 30 days label Apr 10, 2019
@stale stale bot closed this Apr 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
inactive Inactive for >= 30 days
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants