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.

Sign up for GitHub

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

Allow multiple time shifts #5067

Merged
merged 3 commits into from
Jun 1, 2018
Merged

Allow multiple time shifts #5067

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
72b0d9e
Allow multiple time shifts
betodealmeida May 24, 2018
545dd38
Merge branch 'master' of github.com:apache/incubator-superset into DP…
betodealmeida May 29, 2018
23ea649
Handle old form data
betodealmeida May 29, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 13 additions & 4 deletions superset/assets/src/explore/controls.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1575,11 +1575,20 @@ export const controls = {
},

time_compare: {
type: 'TextControl',
type: 'SelectControl',
multi: true,
Copy link
Member

Choose a reason for hiding this comment

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

I think that there's an open PR that makes this backward compatible. Here: #5057 , otherwise I think it would fail for previously saved charts

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I remembered that as soon as I created the PR. Cool that there's a PR addressing it.

Copy link
Member

Choose a reason for hiding this comment

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

Merged it, so we're good

Copy link
Member

Choose a reason for hiding this comment

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

Did another comment on that PR, I think the PR fixes the issue only for the explore view, not for the dashboard view. I think it requires a bit more thinking.

freeForm: true,
label: t('Time Shift'),
default: null,
description: t('Overlay a timeseries from a ' +
'relative time period. Expects relative time delta ' +
default: [],
choices: formatSelectOptions([
'1 day',
'1 week',
'28 days',
'30 days',
'1 year',
]),
description: t('Overlay one or more timeseries from a ' +
'relative time period. Expects relative time deltas ' +
'in natural language (example: 24 hours, 7 days, ' +
'56 weeks, 365 days)'),
},
Expand Down
2 changes: 1 addition & 1 deletion superset/assets/src/explore/visTypes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ export const sections = {
'of query results'),
controlSetRows: [
['rolling_type', 'rolling_periods', 'min_periods'],
['time_compare', null],
['time_compare'],
['num_period_compare', 'period_ratio_type'],
['resample_how', 'resample_rule', 'resample_fillmethod'],
],
Expand Down
13 changes: 7 additions & 6 deletions superset/viz.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ def __init__(self, datasource, form_data, force=False):
self._some_from_cache = False
self._any_cache_key = None
self._any_cached_dttm = None
self._extra_chart_data = None
self._extra_chart_data = []

self.process_metrics()

Expand Down Expand Up @@ -1199,10 +1199,10 @@ def process_data(self, df, aggregate=False):

def run_extra_queries(self):
fd = self.form_data
time_compare = fd.get('time_compare')
if time_compare:
time_compare = fd.get('time_compare') or []
for option in time_compare:
Copy link
Member

Choose a reason for hiding this comment

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

Eventually (out of scope for this PR) we could have parallelization mechanism for this kind of things, making timeouts less likely.

Copy link
Member Author

Choose a reason for hiding this comment

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

(y)

Copy link
Member Author

Choose a reason for hiding this comment

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

I looked into parallelizing this, but it's non trivial: we can't just wrap the calls here in threads or processes because the connection is already established at this point, and for some backends like SQLite it can't be shared between threads/processes.

I agree that we should probably do this in a separate PR, making the logic of run_extra_queries more robust.

query_object = self.query_obj()
delta = utils.parse_human_timedelta(time_compare)
delta = utils.parse_human_timedelta(option)
query_object['inner_from_dttm'] = query_object['from_dttm']
query_object['inner_to_dttm'] = query_object['to_dttm']

Expand All @@ -1215,10 +1215,11 @@ def run_extra_queries(self):

df2 = self.get_df_payload(query_object).get('df')
if df2 is not None:
label = '{} offset'. format(option)
df2[DTTM_ALIAS] += delta
df2 = self.process_data(df2)
self._extra_chart_data = self.to_series(
df2, classed='superset', title_suffix='---')
self._extra_chart_data.extend(self.to_series(
df2, classed='superset', title_suffix=label))

def get_data(self, df):
df = self.process_data(df)
Expand Down