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

Pass granularity from backend to frontend as ISO duration #4755

Merged
merged 8 commits into from
Apr 6, 2018

Conversation

betodealmeida
Copy link
Member

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
Copy link

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'],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@betodealmeida ^ please confirm and I'll merge

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
@mistercrunch mistercrunch deleted the DPTOOLS-392_granularity_payload branch April 6, 2018 23:19
@nathan-quinn
Copy link

@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
Copy link
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
Copy link
Member

@betodealmeida ^

michellethomas pushed a commit to michellethomas/panoramix that referenced this pull request May 24, 2018
* 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 pushed a commit to timifasubaa/incubator-superset that referenced this pull request May 31, 2018
* 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
Copy link
Member

@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
* 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
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.25.0 labels Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.25.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants