Skip to content
This repository has been archived by the owner on Dec 10, 2021. It is now read-only.

fix: revert default series limit and update eligible choices #1430

Merged
merged 3 commits into from
Oct 28, 2021

Conversation

john-bodley
Copy link
Contributor

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

🏠 Internal

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 #1033 (and more) to ensure there is no default if undefined given that:

  • It is not apparent from the UX when the current default is cleared, i.e., Select ... is rendered, that a default is set.
  • The only way to previously disable the joined subquery is to set the series limit to 0 which is misleading (and thus removed as an option) given that in actually it is akin to having no limits as the joined subquery logic is not invoked.

Furthermore said change was actually a breaking change but was not protected with a feature flag.

Related PRs:

@john-bodley john-bodley requested a review from a team as a code owner October 25, 2021 17:26
@vercel
Copy link

vercel bot commented Oct 25, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/superset/superset-ui/chL5Fk5MWQByT84S6d3sx64yBHCq
✅ Preview: https://superset-ui-git-revert-1033-etr2460-default-ser-121fb0-superset.vercel.app

@@ -330,7 +330,6 @@ const limit: SharedControlConfig<'SelectControl'> = {
freeForm: true,
label: t('Series limit'),
validators: [legacyValidateInteger],
default: 100,
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with removing the default, as the added overhead of the limit query is difficult to justify nowadays. Should we also add clearable: true here to make sure the value is clearable, as the "unset" does not exist in the SERIES_LIMIT constant?

@john-bodley john-bodley changed the title Revert "feat: add default series_limit" fix: default series limit and options Oct 26, 2021
@john-bodley john-bodley changed the title fix: default series limit and options fix: default series limit and choices Oct 26, 2021
@codecov
Copy link

codecov bot commented Oct 26, 2021

Codecov Report

Merging #1430 (1c1bcf1) into master (f279549) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1430   +/-   ##
=======================================
  Coverage   30.42%   30.42%           
=======================================
  Files         497      497           
  Lines       10003    10003           
  Branches     1689     1689           
=======================================
  Hits         3043     3043           
  Misses       6714     6714           
  Partials      246      246           
Impacted Files Coverage Δ
...et-ui-chart-controls/src/shared-controls/index.tsx 36.00% <100.00%> (ø)

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 f279549...1c1bcf1. Read the comment docs.

@john-bodley john-bodley force-pushed the revert-1033-etr2460--default-series-limit branch from d80c839 to 1c1bcf1 Compare October 26, 2021 19:00
@john-bodley john-bodley changed the title fix: default series limit and choices fix: revert default series limit and update eligible choices Oct 26, 2021
Copy link
Contributor

@villebro villebro left a comment

Choose a reason for hiding this comment

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

LGTM

@john-bodley john-bodley merged commit 4ac888e into master Oct 28, 2021
@delete-merged-branch delete-merged-branch bot deleted the revert-1033-etr2460--default-series-limit branch October 28, 2021 15:44
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants