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

Pass granularity from backend to frontend as ISO duration #4755

Merged
merged 8 commits into from Apr 6, 2018

Conversation

Projects
None yet
6 participants
@betodealmeida
Contributor

betodealmeida commented Apr 4, 2018

Currently the backend will pass the granularity as a string that is engine dependent, e.g, "30 seconds". I changed the backend so that an ISO 8601 duration is passed instead. This allows the frontend to handle the granularity better, eg, for the play slider.

@codecov-io

This comment has been minimized.

codecov-io commented Apr 4, 2018

Codecov Report

Merging #4755 into master will decrease coverage by 0.04%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4755      +/-   ##
==========================================
- Coverage   72.38%   72.33%   -0.05%     
==========================================
  Files         205      205              
  Lines       15374    15394      +20     
  Branches     1182     1182              
==========================================
+ Hits        11128    11136       +8     
- Misses       4243     4255      +12     
  Partials        3        3
Impacted Files Coverage Δ
...set/assets/javascripts/explore/stores/controls.jsx 39.25% <ø> (ø) ⬆️
superset/db_engine_specs.py 51.89% <100%> (ø) ⬆️
superset/models/core.py 86.52% <100%> (ø) ⬆️
superset/connectors/sqla/models.py 77.08% <100%> (-0.21%) ⬇️
superset/dataframe.py 97.7% <0%> (-1.05%) ⬇️
superset/viz.py 79.1% <0%> (-0.53%) ⬇️
superset/utils.py 88.07% <0%> (+0.02%) ⬆️

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 7a497e2...9c08b66. Read the comment docs.

]),
choices: [
[null, 'all'],
['PT5S', '5 seconds'],

This comment has been minimized.

@mistercrunch

mistercrunch Apr 4, 2018

Contributor

If a chart has been saved with a string say 5 seconds as a value, did you confirm that this load up ok in here as a "free form" value? From my understanding it should.

This comment has been minimized.

@mistercrunch

mistercrunch Apr 6, 2018

Contributor

@betodealmeida ^ please confirm and I'll merge

This comment has been minimized.

@betodealmeida

betodealmeida Apr 6, 2018

Contributor

It does, it loads with the saved value as freeform:

screen shot 2018-04-06 at 11 16 44 am

@mistercrunch mistercrunch merged commit 426c34e into apache:master Apr 6, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@mistercrunch mistercrunch deleted the lyft:DPTOOLS-392_granularity_payload branch Apr 6, 2018

john-bodley pushed a commit to john-bodley/incubator-superset that referenced this pull request Apr 8, 2018

Pass granularity from backend to frontend as ISO duration (apache#4755)
* Add ISO duration to time grains

* Use ISO duration

* Remove debugging code

* Add module to yarn.lock

* Remove autolint

* Druid granularity as ISO

* Remove dangling comma
@NathanQuinn

This comment has been minimized.

NathanQuinn commented Apr 10, 2018

@betodealmeida @mistercrunch

I exported a postgres backed set of charts and dashboards to another postgres instance with this change included. It seems to work when viewing the actual chart, but when a time grain is persisted on a chart used in a dashboard I'm getting an error as such:

Traceback (most recent call last):
  File "/home/superset/superset/viz.py", line 362, in get_df_payload
    df = self.get_df(query_obj)
  File "/home/superset/superset/viz.py", line 160, in get_df
    self.results = self.datasource.query(query_obj)
  File "/home/superset/superset/connectors/sqla/models.py", line 705, in query
    sql = self.get_query_str(query_obj)
  File "/home/superset/superset/connectors/sqla/models.py", line 419, in get_query_str
    qry = self.get_sqla_query(**query_obj)
  File "/home/superset/superset/connectors/sqla/models.py", line 545, in get_sqla_query
    timestamp = dttm_col.get_timestamp_expression(time_grain)
  File "/home/superset/superset/connectors/sqla/models.py", line 134, in get_timestamp_expression
    expr = grain.function.format(col=expr)
AttributeError: 'str' object has no attribute 'function'

I was able to backfill values stored on slices.params.time_grain_sqla, to the ISO durations from here and this resolved the issue. Did I miss a migration someplace or should one be added for this?

@xrmx

This comment has been minimized.

Contributor

xrmx commented Apr 10, 2018

@NathanQuinn ~~i think the ".function" part should be removed~~~. It's the default of the get that is wrong, it shouldn't be a string but a Grain. And we are probably missing some tests :)

@mistercrunch

This comment has been minimized.

Contributor

mistercrunch commented Apr 10, 2018

ttannis added a commit to ttannis/incubator-superset that referenced this pull request Apr 27, 2018

Pass granularity from backend to frontend as ISO duration (apache#4755)
* Add ISO duration to time grains

* Use ISO duration

* Remove debugging code

* Add module to yarn.lock

* Remove autolint

* Druid granularity as ISO

* Remove dangling comma

michellethomas added a commit to michellethomas/panoramix that referenced this pull request May 24, 2018

Pass granularity from backend to frontend as ISO duration (apache#4755)
* Add ISO duration to time grains

* Use ISO duration

* Remove debugging code

* Add module to yarn.lock

* Remove autolint

* Druid granularity as ISO

* Remove dangling comma

timifasubaa added a commit to timifasubaa/incubator-superset that referenced this pull request May 31, 2018

Pass granularity from backend to frontend as ISO duration (apache#4755)
* Add ISO duration to time grains

* Use ISO duration

* Remove debugging code

* Add module to yarn.lock

* Remove autolint

* Druid granularity as ISO

* Remove dangling comma
@john-bodley

This comment has been minimized.

Contributor

john-bodley commented Jul 3, 2018

@betodealmeida should there be a DB migration associated with this PR to update slices which reference the deprecated time grains?

cc: @michellethomas

wenchma pushed a commit to wenchma/incubator-superset that referenced this pull request Nov 16, 2018

Pass granularity from backend to frontend as ISO duration (apache#4755)
* Add ISO duration to time grains

* Use ISO duration

* Remove debugging code

* Add module to yarn.lock

* Remove autolint

* Druid granularity as ISO

* Remove dangling comma
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment