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

Allow multiple time shifts #5067

Merged
merged 3 commits into from Jun 1, 2018

Conversation

betodealmeida
Copy link
Member

This PR changes the Advanced Analytics section to allow multiple time shifts, instead of only one:

multiple_time_shifts

@codecov-io
Copy link

codecov-io commented May 24, 2018

Codecov Report

Merging #5067 into master will decrease coverage by 0.01%.
The diff coverage is 50%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
superset/viz.py 81.32% <50%> (-0.1%) ⬇️

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 0511d1f...23ea649. Read the comment docs.

@@ -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.

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:
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.

@hughhhh hughhhh added the lyft Related to Lyft label May 31, 2018
@mistercrunch mistercrunch merged commit 1d3e96b into apache:master Jun 1, 2018
@mistercrunch mistercrunch deleted the DPTOOLS-445_multiple_time_shifts branch June 1, 2018 04:18
mistercrunch pushed a commit to lyft/incubator-superset that referenced this pull request Jun 4, 2018
* Allow multiple time shifts

* Handle old form data

(cherry picked from commit 1d3e96b)
@betodealmeida betodealmeida mentioned this pull request Jun 5, 2018
timifasubaa pushed a commit to airbnb/superset-fork that referenced this pull request Jul 25, 2018
* Allow multiple time shifts

* Handle old form data
graceguo-supercat pushed a commit to graceguo-supercat/superset that referenced this pull request Jul 26, 2018
* Allow multiple time shifts

* Handle old form data

(cherry picked from commit 1d3e96b)
wenchma pushed a commit to wenchma/incubator-superset that referenced this pull request Nov 16, 2018
* Allow multiple time shifts

* Handle old form data
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.26.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 lyft Related to Lyft 🚢 0.26.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants