Skip to content

fix(mixed-chart): preserve order_desc and series_limit_metric in buildQuery#41401

Draft
sadpandajoe wants to merge 1 commit into
masterfrom
claude/strange-banzai-d82afd
Draft

fix(mixed-chart): preserve order_desc and series_limit_metric in buildQuery#41401
sadpandajoe wants to merge 1 commit into
masterfrom
claude/strange-banzai-d82afd

Conversation

@sadpandajoe

Copy link
Copy Markdown
Member

SUMMARY

In a Mixed Chart (mixed_timeseries), changing "Sort Query By" or toggling "Sort Descending" updated the displayed SQL ("View query") but did not change the rendered result.

Root cause is in the frontend query builder. MixedTimeseries/buildQuery.ts returned the full output of normalizeOrderBy(tmpQueryObject). normalizeOrderBy deletes order_desc and series_limit_metric from the object it returns, re-adding only orderby. The backend main query reads orderby (so the displayed SQL looks correct), but the series-limit subquery that decides which top-N series to show reads order_desc and series_limit_metric directly (superset/models/helpers.py: direction = sa.desc if order_desc else sa.asc, and _get_series_orderby reads series_limit_metric). With those two fields stripped, the backend QueryObject defaults order_desc to True, so the same top-N series were always selected → the result never changed.

The fix mirrors what the single-query Timeseries chart already does: spread the query object and override only orderby, preserving order_desc and series_limit_metric.

// MixedTimeseries/buildQuery.ts
return [
  {
    ...tmpQueryObject,
    orderby: normalizeOrderBy(tmpQueryObject).orderby,
  },
];

This applies symmetrically to Query A and Query B.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

N/A — no visual chrome change. The behavior change is that the rendered series now respond to the sort controls (see Testing Instructions).

TESTING INSTRUCTIONS

  1. Create a Mixed Chart (Mixed Timeseries) on a dataset with a dimension that produces more series than the series limit (e.g. group by a high-cardinality column with Series limit set to a small N).
  2. Set "Sort Query By" to a metric and toggle "Sort Descending" on, then off.
  3. Before this fix: the displayed SQL changes but the rendered top-N series stay the same.
  4. After this fix: the rendered top-N series change to honor the chosen sort metric and direction, for both Query A and Query B.

Automated coverage (plugin-chart-echarts/test/MixedTimeseries/buildQuery.test.ts):

  • Updated "should compile query object A/B" to assert order_desc and series_limit_metric are preserved.
  • Added a regression test, preserves order_desc and series_limit_metric for both queries, asserting the sc-107146 ascending-sort repro keeps order_desc: false + series_limit_metric for both queries. Reverting only the source fix fails this test.

Run: npm run test -- plugins/plugin-chart-echarts/test/MixedTimeseries/buildQuery.test.ts

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

Reviewer / QA note — expected behavior change on existing saved charts

Preserving series_limit_metric changes the series-limit subquery's ORDER BY column, not just its direction. Previously the field was stripped, so the backend fell back to ordering the top-N prequery by the main metric; with the fix it orders by the chosen "Sort Query By" metric (helpers.py _get_series_orderby). This is the intended correctness fix, but existing saved Mixed Charts may render a different (correct) top-N series set with no user action. Please don't mistake this for a regression during QA.

@codecov

codecov Bot commented Jun 24, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 64.37%. Comparing base (d6ede99) to head (8513f40).
⚠️ Report is 33 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #41401      +/-   ##
==========================================
- Coverage   64.37%   64.37%   -0.01%     
==========================================
  Files        2653     2653              
  Lines      145152   145152              
  Branches    33491    33491              
==========================================
- Hits        93446    93440       -6     
- Misses      50014    50020       +6     
  Partials     1692     1692              
Flag Coverage Δ
javascript 68.66% <100.00%> (-0.01%) ⬇️

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

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

…dQuery

MixedTimeseries buildQuery returned the full `normalizeOrderBy()` result,
which deletes `order_desc` and `series_limit_metric` from its returned
clone and re-adds only `orderby`. The backend main query reads `orderby`
(so "View query" SQL updated correctly), but the series-limit subquery that
selects which top-N series to display reads `order_desc` directly and the
sort metric via `_get_series_orderby`. With both stripped, the backend
QueryObject defaulted `order_desc` to True (descending), so toggling
"Sort Descending" or changing "Sort Query By" never changed the rendered
result.

Spread the query object and overwrite only `orderby`, mirroring the
single-query Timeseries chart's buildQuery. Adds a regression test that
fails without the fix.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Fixes Mixed Timeseries (mixed_timeseries) query construction so changing “Sort Query By” and “Sort Descending” affects not only the displayed SQL but also the rendered top‑N series selection. The frontend now preserves order_desc and series_limit_metric while still normalizing orderby, matching the established pattern used by the single-query Timeseries chart.

Changes:

  • Update MixedTimeseries buildQuery to spread the original query object and override only orderby using normalizeOrderBy(...).
  • Adjust existing unit tests to assert order_desc / series_limit_metric are preserved on compiled query objects.
  • Add a regression test covering the “Sort Descending off” repro for both Query A and Query B.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/buildQuery.ts Preserve order_desc and series_limit_metric while normalizing orderby so series-limit behavior matches user sort controls.
superset-frontend/plugins/plugin-chart-echarts/test/MixedTimeseries/buildQuery.test.ts Update expectations and add regression coverage ensuring both Mixed queries keep order_desc/series_limit_metric.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants