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: revert default series sort-by metric #17236

Merged

Conversation

john-bodley
Copy link
Member

@john-bodley john-bodley commented Oct 26, 2021

SUMMARY

This PR is one of a slew to the apache/superset and apache-superset/superset-ui repos to revert pseudo recent changes to the series restrictions for high cardinality groupings. For more context please refer to this Slack thread in the #committers channel.

Specifically this PR reverts #15343 (and more), i.e., does not fallback to the first selected metric and requires the user to specify a sort-by metric for improved UX, i.e., it was not apparent to the user from the UI how the sorting was happening. Furthermore said change was actually a breaking change but was not protected with a feature flag.

Initially I added logic for an XOR check that required either neither or both the series sort-by and limit needed to be provided, but this would be a breaking change—in addition to thinking about how this should work with the table row limit—and thus I felt this would be better handled at a later stage.

Related PRs:

TESTING INSTRUCTIONS

CI.

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

@john-bodley john-bodley changed the title fix: Revert default series sort-by metric and enforce non-xor with series limit fix: revert default series sort-by metric and enforce non-xor with series limit Oct 26, 2021
@john-bodley john-bodley force-pushed the john-bodley--fix-series-sort-by-limit branch from 1a58a10 to 86ddaf8 Compare October 26, 2021 20:36
@pull-request-size pull-request-size bot added size/M and removed size/L labels Oct 26, 2021
@john-bodley john-bodley marked this pull request as ready for review October 26, 2021 20:36
@john-bodley john-bodley force-pushed the john-bodley--fix-series-sort-by-limit branch 2 times, most recently from f5d8a45 to 9652004 Compare October 26, 2021 21:33
@@ -92,7 +92,7 @@ export const D3_FORMAT_OPTIONS = [

const ROW_LIMIT_OPTIONS = [10, 50, 100, 250, 500, 1000, 5000, 10000, 50000];

const SERIES_LIMITS = [0, 5, 10, 25, 50, 100, 500];
const SERIES_LIMITS = [5, 10, 25, 50, 100, 500];
Copy link
Member Author

Choose a reason for hiding this comment

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

Consistent with apache-superset/superset-ui#1430. Note I'm unsure why things are defined in multiple places.

@codecov
Copy link

codecov bot commented Oct 26, 2021

Codecov Report

Merging #17236 (86a6612) into master (dd71035) will increase coverage by 0.00%.
The diff coverage is 93.75%.

❗ Current head 86a6612 differs from pull request most recent head 8fbdc32. Consider uploading reports for the commit 8fbdc32 to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##           master   #17236   +/-   ##
=======================================
  Coverage   76.95%   76.95%           
=======================================
  Files        1038     1037    -1     
  Lines       55612    55609    -3     
  Branches     7590     7588    -2     
=======================================
- Hits        42798    42796    -2     
+ Misses      12564    12563    -1     
  Partials      250      250           
Flag Coverage Δ
javascript 71.01% <100.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
superset/utils/core.py 90.14% <ø> (ø)
superset/connectors/sqla/models.py 87.48% <92.85%> (ø)
superset-frontend/src/explore/controls.jsx 29.72% <100.00%> (ø)
superset/viz.py 57.99% <100.00%> (ø)
...eFilters/FiltersConfigModal/FiltersConfigModal.tsx 87.13% <0.00%> (-0.59%) ⬇️
...perset-frontend/src/components/ChartIcon/index.tsx
superset/db_engine_specs/presto.py 90.37% <0.00%> (+0.41%) ⬆️
... and 1 more

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 dd71035...8fbdc32. Read the comment docs.

@john-bodley john-bodley force-pushed the john-bodley--fix-series-sort-by-limit branch from 9652004 to 8f659aa Compare October 26, 2021 23:13
@john-bodley john-bodley force-pushed the john-bodley--fix-series-sort-by-limit branch from 8f659aa to 8fbdc32 Compare October 26, 2021 23:20
@john-bodley john-bodley changed the title fix: revert default series sort-by metric and enforce non-xor with series limit fix: revert default series sort-by metric Oct 26, 2021
@john-bodley
Copy link
Member Author

john-bodley commented Nov 1, 2021

@zhaoyongjie any thoughts/comments regarding this in light of the comments I added here?

Copy link
Member

@etr2460 etr2460 left a comment

Choose a reason for hiding this comment

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

this makes sense. ideally we could export the SERIES_LIMITS constant from superset-ui and use that here, but it feels out of scope

@john-bodley john-bodley merged commit 1c12167 into apache:master Nov 3, 2021
@john-bodley john-bodley mentioned this pull request Nov 9, 2021
9 tasks
AAfghahi pushed a commit that referenced this pull request Jan 10, 2022
…ries limit (#17236)

Co-authored-by: John Bodley <john.bodley@airbnb.com>
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.5.0 labels Mar 13, 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 🚢 1.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants