-
Notifications
You must be signed in to change notification settings - Fork 14.6k
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
Allow multiple time shifts #5067
Allow multiple time shifts #5067
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5067 +/- ##
==========================================
- Coverage 77.5% 77.48% -0.02%
==========================================
Files 44 44
Lines 8720 8723 +3
==========================================
+ Hits 6758 6759 +1
- Misses 1962 1964 +2
Continue to review full report at Codecov.
|
@@ -1575,11 +1575,20 @@ export const controls = { | |||
}, | |||
|
|||
time_compare: { | |||
type: 'TextControl', | |||
type: 'SelectControl', | |||
multi: true, |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
superset/viz.py
Outdated
time_compare = fd.get('time_compare') | ||
if time_compare: | ||
time_compare = fd.get('time_compare') or [] | ||
for option in time_compare: |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(y)
There was a problem hiding this comment.
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.
…TOOLS-445_multiple_time_shifts
* Allow multiple time shifts * Handle old form data (cherry picked from commit 1d3e96b)
* Allow multiple time shifts * Handle old form data
* Allow multiple time shifts * Handle old form data (cherry picked from commit 1d3e96b)
* Allow multiple time shifts * Handle old form data
This PR changes the Advanced Analytics section to allow multiple time shifts, instead of only one: