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

fix: Set default series limit to zero #1428

Closed
wants to merge 1 commit into from

Conversation

john-bodley
Copy link
Contributor

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

💔 Breaking Changes
🏠 Internal

I'm not sure if the series limit logic changed as part of apache/superset#16660 (personally I couldn't find anything abnormal which gets invoked if the series limit is truthy, i.e., not None or 0 unless perviously there was also a requirement for a sort-by metric) but it seems like either a sub-query or two-phase query runs even if they user didn't explicitly define a series limit per the UI as it falls back to the default (100).

Per the attached screenshot, when clearing the limit there's no indication that a limit is being defined—I would speculate that the user would sense the limit is undefined, i.e., None (which is equivalent to 0):

Screen Shot 2021-10-25 at 8 43 19 AM

however the form-data falls back to the default, resulting in an unexpected sub-query or two-phase query,

SELECT year AS __timestamp,
               genre AS genre,
               COUNT(*) AS count
FROM video_game_sales
JOIN
  (SELECT genre AS genre__,
          COUNT(*) AS mme_inner__
   FROM video_game_sales
   GROUP BY genre
   ORDER BY mme_inner__ DESC
   LIMIT 100
   OFFSET 0) AS anon_1 ON genre = genre__
GROUP BY genre,
         year
ORDER BY count DESC
LIMIT 50000
OFFSET 0;

The TL;DR is I'm not sure if there's been a regression in the backend but it does feel like a UX issue. The "fix" is merely to set the default to 0 which is consistent with the Python logic.

The other change is merely to revert #1033, i.e., remove the default, as it seems like said change broke the UX.

Note @etr2460 and @villebro I'm not sure if this is considered a breaking change given it could impact some charts, i.e., in theory there were charts which previously showed 100 series due to the limit but now will show an unbounded number. It does seem we having been gating the Apache Superset releases with these types of breaking changes.

@john-bodley john-bodley requested a review from a team as a code owner October 25, 2021 15:53
@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/CgKe1gvh8USknjpXdjBTK4Sa4VXn
✅ Preview: Failed

@john-bodley
Copy link
Contributor Author

Closing in favor of #1430.

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.

1 participant