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

fix(drillby): Enable DrillBy in charts w/o filters (dimensions) #27941

Merged
merged 3 commits into from
Apr 11, 2024

Conversation

sowo
Copy link
Contributor

@sowo sowo commented Apr 8, 2024

SUMMARY

This fix resolves #27940 by enabling "Drll by" for Timeseries and Mixed Timeseries charts that do not have dimensions configured.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before:
Screenshot from 2024-04-08 15-30-47

After

image

image

TESTING INSTRUCTIONS

Open any dashboard that includes either Timeseries or Mixed Timeseries charts without dimensions and right-click on any data point in those charts. The "Drill by" should be available and open the modal for the selected dimension.

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@sowo sowo marked this pull request as draft April 8, 2024 16:26
@rusackas
Copy link
Member

rusackas commented Apr 8, 2024

Thanks @sowo this looks promising! Curious what you see left to do here, but we look forward to reviewing it when it's ready. So far, the code looks good to me!

@sowo
Copy link
Contributor Author

sowo commented Apr 8, 2024 via email

@john-bodley john-bodley added review:checkpoint Last PR reviewed during the daily review standup review:draft labels Apr 8, 2024
@rusackas
Copy link
Member

rusackas commented Apr 8, 2024

/testenv up

@rusackas
Copy link
Member

rusackas commented Apr 8, 2024

Maybe if @sfirke's hypothesis is right, we'll get lucky and this will help with #25334 as well... but maybe not. Just something to test when the ephemeral env spins up.

@pull-request-size pull-request-size bot added size/S and removed size/M labels Apr 8, 2024
@sowo
Copy link
Contributor Author

sowo commented Apr 8, 2024

Seems the breadcrumbs and drillByConfigs need to be in sync to make breadcrumb navigation work. So rather than filtering out the first breadcrumb item when it is empty (for charts without dimensions), I decided to just hide the first breadcrumb. This is somewhat ugly as it leaves the separator (/) visible.

image

Let me know if anyone has a better idea on how to non-intrusively fix this.

@pull-request-size pull-request-size bot added size/M and removed size/S labels Apr 9, 2024
@kgabryje
Copy link
Member

kgabryje commented Apr 9, 2024

/testenv up

Copy link
Contributor

github-actions bot commented Apr 9, 2024

@kgabryje Ephemeral environment spinning up at http://35.89.213.150:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

@sowo sowo marked this pull request as ready for review April 9, 2024 10:15
@john-bodley john-bodley removed review:checkpoint Last PR reviewed during the daily review standup review:draft labels Apr 9, 2024
Copy link
Member

@kgabryje kgabryje left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution @sowo! The code looks good to me. Feature wise, I think it would be great if we could have a drill filter based on X axis group, but that needs to be thought through first, so I'm fine with going with your change for now 🙂

@rusackas
Copy link
Member

Hmm... closing/reopening to see if that kicks CI into gear.

@rusackas rusackas closed this Apr 11, 2024
@rusackas rusackas reopened this Apr 11, 2024
Copy link
Contributor

Ephemeral environment shutdown and build artifacts deleted.

@rusackas rusackas merged commit 35c8b7a into apache:master Apr 11, 2024
82 of 83 checks passed
@sowo
Copy link
Contributor Author

sowo commented Apr 11, 2024

@kgabryje initially, I used the X axis value plus the selected dimension for the drill by chart. But this created a time series chart with just one bar broken down by the selected dimension. For this to really be useful, the chart type would need to be changed - e.g. pie chart. Is there any plan to make the chart type selectable as one drills down?

Thanks

@michael-s-molina michael-s-molina added the v4.0 Label added by the release manager to track PRs to be included in the 4.0 branch label Apr 12, 2024
EnxDev pushed a commit to EnxDev/superset that referenced this pull request Apr 15, 2024
michael-s-molina pushed a commit that referenced this pull request Apr 16, 2024
michael-s-molina pushed a commit that referenced this pull request Apr 16, 2024
michael-s-molina pushed a commit that referenced this pull request Apr 16, 2024
qleroy pushed a commit to qleroy/superset that referenced this pull request Apr 28, 2024
@sfirke
Copy link
Member

sfirke commented May 3, 2024

Maybe if @sfirke's hypothesis is right, we'll get lucky and this will help with #25334 as well... but maybe not. Just something to test when the ephemeral env spins up.

Alas this did not, just confirmed while testing 4.0.1rc1 which contains this PR. This fix works through and is great!

jzhao62 pushed a commit to jzhao62/superset that referenced this pull request May 16, 2024
@mistercrunch mistercrunch added 🍒 4.0.1 🍒 4.0.2 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels labels Jul 24, 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 size/M v4.0 Label added by the release manager to track PRs to be included in the 4.0 branch 🍒 4.0.1 🍒 4.0.2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Drill by is disabled for Timeseries and Mixed Timeseries charts without dimensions
7 participants