fix(legacy-viz): restore line-engine default ordering for horizon and rose by rusackas · Pull Request #41732 · apache/superset

rusackas · 2026-07-03T07:01:23Z

SUMMARY

Follow-up to #41725/#41726 (targets remove-legacy-viz-pipeline): a bot review on the partition PR pointed out that the legacy line engine (NVD3TimeSeriesViz.query_obj) always ordered by the sort metric or the first selected metric, ascending unless order_desc. The horizon and rose buildQueries had dropped that default ordering, which changes which rows survive row_limit truncation. Restored, with tests pinning both directions.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

N/A — query ordering parity only.

TESTING INSTRUCTIONS

npm run test -- plugins/legacy-plugin-chart-horizon plugins/legacy-plugin-chart-rose — 18 tests pass.

ADDITIONAL INFORMATION

Has associated issue:
Required feature flags:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
Introduces new feature or API
Removes existing feature or API

🤖 Generated with Claude Code

… rose The NVD3TimeSeriesViz query_obj both charts rode on always ordered by the sort metric or the first selected metric, ascending unless order_desc. Their new buildQueries dropped that, which changes which rows survive the row limit. Restored with tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

bito-code-review · 2026-07-03T07:01:33Z

Bito Automatic Review Skipped - Branch Excluded

Bito didn't auto-review because the source or target branch is excluded from automatic reviews.
No action is needed if you didn't intend for the agent to review it. Otherwise, to manually trigger a review, type /review in a comment and save.
You can change the branch exclusion settings here, or contact your Bito workspace admin at evan@preset.io.

codeant-ai-for-open-source · 2026-07-03T07:10:34Z

+    const firstMetric = ensureIsArray(baseQueryObject.metrics)[0];
+    return [
+      {
+        ...baseQueryObject,
+        is_timeseries: true,
+        orderby: firstMetric ? [[firstMetric, !order_desc]] : undefined,


Suggestion: This implementation always orders by the first selected metric and ignores timeseries_limit_metric, which breaks legacy NVD3 parity and changes which series survive truncation when users pick a dedicated sort metric. Build the sort target from timeseries_limit_metric first (fallback to first metric), and mirror legacy behavior by ensuring that sort metric is included in metrics before applying orderby. [api mismatch]

Severity Level: Major ⚠️

❌ Horizon charts ignore configured series sort metric. ❌ Series limiting uses wrong metric, changing visible series. ⚠️ Legacy NVD3TimeSeries ordering parity is not preserved.

Steps of Reproduction ✅

1. Note the legacy line engine behavior in `superset/viz.py:31-42` where `NVD3TimeSeriesViz.query_obj()` computes `sort_by = self.form_data.get("timeseries_limit_metric") or utils.get_first_metric_name(query_obj.get("metrics") or [])`, appends `sort_by` into `query_obj["metrics"]` if missing, and then sets `query_obj["orderby"] = [(sort_by, is_asc)]`. 2. Observe that the Horizon chart exposes a `timeseries_limit_metric` control: `superset-frontend/plugins/legacy-plugin-chart-horizon/src/controlPanel.ts:38-41` defines the Query section rows as `['limit', 'timeseries_limit_metric']`, so users can configure a dedicated sort metric for series limiting. 3. In the new v1 pipeline, the Horizon plugin’s query is built by `buildQuery()` in `superset-frontend/plugins/legacy-plugin-chart-horizon/src/buildQuery.ts:31-39`, which unconditionally sets `const firstMetric = ensureIsArray(baseQueryObject.metrics)[0];` and `orderby: firstMetric ? [[firstMetric, !order_desc]] : undefined` without ever reading `formData.timeseries_limit_metric` or `baseQueryObject.series_limit_metric`. 4. Create or edit a Horizon chart in Explore using the Horizon plugin (registered in `superset-frontend/src/visualizations/presets/MainPreset.ts:28-33,133`) with `metrics = ['sum__num']`, `limit > 0`, and `timeseries_limit_metric` set to a different metric (e.g. `avg__num`); when the chart loads, the generated query’s `orderby` will still be `[['sum__num', !order_desc]]` instead of the configured `timeseries_limit_metric`, so the database row limiting and series truncation are performed using the wrong metric and the sort metric is never guaranteed to be present in `metrics`, diverging from the legacy `NVD3TimeSeriesViz` behavior.

(Use Cmd/Ctrl + Click for best experience)

Prompt for AI Agent 🤖

This is a comment left during a code review. **Path:** superset-frontend/plugins/legacy-plugin-chart-horizon/src/buildQuery.ts **Line:** 34:39 **Comment:** *Api Mismatch: This implementation always orders by the first selected metric and ignores `timeseries_limit_metric`, which breaks legacy NVD3 parity and changes which series survive truncation when users pick a dedicated sort metric. Build the sort target from `timeseries_limit_metric` first (fallback to first metric), and mirror legacy behavior by ensuring that sort metric is included in `metrics` before applying `orderby`. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix

👍 | 👎

codeant-ai-for-open-source · 2026-07-03T07:10:34Z

+  // legacy engine default: order by the first metric ascending
+  expect(query.orderby).toEqual([['sum__num', true]]);


Suggestion: This test locks in the wrong contract by asserting first-metric ordering only, so it will pass even when timeseries_limit_metric behavior regresses. Add coverage for the sort-metric path and expected fallback order so the test validates the real legacy ordering contract. [logic error]

Severity Level: Major ⚠️

⚠️ Horizon buildQuery tests miss sort-metric ordering branch. ⚠️ Regressions in timeseries_limit_metric handling go undetected. ⚠️ Legacy ordering contract is not fully specified in tests.

Steps of Reproduction ✅

1. Inspect `superset-frontend/plugins/legacy-plugin-chart-horizon/test/buildQuery.test.ts:22-31`, where `formData` for the Horizon chart includes metrics and limit but does not set any `timeseries_limit_metric`, and the first test asserts basic grouped-timeseries behavior. 2. See the added assertions at lines 40-41 and 44-46: the tests only verify that `orderby` is `[['sum__num', true]]` for the default case and flips to `[['sum__num', false]]` when `order_desc: true`, never exercising a `timeseries_limit_metric` value. 3. Compare this with the legacy behavior in `superset/viz.py:31-42`, where `NVD3TimeSeriesViz.query_obj()` prefers `timeseries_limit_metric` over the first metric, and with the Horizon control panel (`superset-frontend/plugins/legacy-plugin-chart-horizon/src/controlPanel.ts:38-41`) which exposes `timeseries_limit_metric` to users. 4. Because the tests never configure `timeseries_limit_metric`, the current regression in `superset-frontend/plugins/legacy-plugin-chart-horizon/src/buildQuery.ts:31-39`—which ignores `timeseries_limit_metric` and always orders by the first metric—passes all tests, and any future fix or regression in sort-metric handling will likewise not be covered until an explicit `timeseries_limit_metric` test case is added.

(Use Cmd/Ctrl + Click for best experience)

Prompt for AI Agent 🤖

This is a comment left during a code review. **Path:** superset-frontend/plugins/legacy-plugin-chart-horizon/test/buildQuery.test.ts **Line:** 40:41 **Comment:** *Logic Error: This test locks in the wrong contract by asserting first-metric ordering only, so it will pass even when `timeseries_limit_metric` behavior regresses. Add coverage for the sort-metric path and expected fallback order so the test validates the real legacy ordering contract. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix

👍 | 👎

codeant-ai-for-open-source · 2026-07-03T07:10:34Z

    const queryObject = {
      ...baseQueryObject,
      is_timeseries: true,
+      // the legacy engine ordered by the first metric, ascending unless
+      // order_desc
+      orderby: firstMetric
+        ? ([[firstMetric, !formData.order_desc]] as [
+            typeof firstMetric,
+            boolean,
+          ][])
+        : undefined,
      time_offsets: isTimeComparison(formData, baseQueryObject)


Suggestion: The new ordering logic hardcodes the first selected metric and does not honor timeseries_limit_metric, so rose queries no longer match legacy line-engine ordering when a separate sort metric is configured. Use the same legacy rule (sort metric if provided, otherwise first metric) and include the chosen sort metric in metrics if it is not already present. [api mismatch]

Severity Level: Major ⚠️

❌ Rose charts ignore configured timeseries_limit_metric for ordering. ❌ Limited series chosen by wrong metric, skewing results. ⚠️ Legacy line-engine semantics not preserved for rose chart.

Steps of Reproduction ✅

1. Confirm the legacy line-engine behavior for Rose charts: `RoseViz` extends `NVD3TimeSeriesViz` in `superset/viz.py:9-13,40-55`, and `NVD3TimeSeriesViz.query_obj()` at `superset/viz.py:31-42` uses `timeseries_limit_metric` as `sort_by` when present, appends that metric into `query_obj["metrics"]` if missing, and sets `query_obj["orderby"] = [(sort_by, is_asc)]`. 2. Note that the Rose chart’s Query controls in `superset-frontend/plugins/legacy-plugin-chart-rose/src/controlPanel.tsx:35-42` explicitly expose both `limit` and `timeseries_limit_metric` (`['limit', 'timeseries_limit_metric']`), allowing users to choose a dedicated sort metric. 3. In the new v1 Rose buildQuery implementation at `superset-frontend/plugins/legacy-plugin-chart-rose/src/buildQuery.ts:49-75`, the code computes `const firstMetric = ensureIsArray(baseQueryObject.metrics)[0];` and unconditionally sets `orderby: firstMetric ? ([[firstMetric, !formData.order_desc]] as [typeof firstMetric, boolean][]) : undefined`, never reading `formData.timeseries_limit_metric` or `baseQueryObject.series_limit_metric`, and never appending the sort metric into `metrics`. 4. Create a Rose chart in Explore (plugin registered in `superset-frontend/src/visualizations/presets/MainPreset.ts:28-33,142`) with `metrics = ['sum__num']`, `limit > 0`, and `timeseries_limit_metric` set to a different metric like `count`, then run the chart: the resulting query `orderby` will remain `[['sum__num', !order_desc]]` instead of `[['count', !order_desc]]`, so database row/series limiting is driven by the wrong metric and the configured sort metric may not appear in `metrics`, differing from the legacy `NVD3TimeSeriesViz` contract.

(Use Cmd/Ctrl + Click for best experience)

Prompt for AI Agent 🤖

This is a comment left during a code review. **Path:** superset-frontend/plugins/legacy-plugin-chart-rose/src/buildQuery.ts **Line:** 61:72 **Comment:** *Api Mismatch: The new ordering logic hardcodes the first selected metric and does not honor `timeseries_limit_metric`, so rose queries no longer match legacy line-engine ordering when a separate sort metric is configured. Use the same legacy rule (sort metric if provided, otherwise first metric) and include the chosen sort metric in `metrics` if it is not already present. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix

👍 | 👎

codeant-ai-for-open-source · 2026-07-03T07:10:34Z

+test('orders by the first metric like the legacy engine', () => {
+  const [query] = buildQuery(formData).queries;
+  expect(query.orderby).toEqual([['sum__num', true]]);
+  const [descQuery] = buildQuery({ ...formData, order_desc: true }).queries;
+  expect(descQuery.orderby).toEqual([['sum__num', false]]);
+});


Suggestion: This new test validates only first-metric ordering and omits the timeseries_limit_metric case, so it enforces an incomplete behavior model and misses the key legacy branch. Extend the test to assert ordering by the configured sort metric (with fallback to first metric) in both sort directions. [logic error]

Severity Level: Major ⚠️

⚠️ Rose buildQuery tests omit timeseries_limit_metric ordering case. ⚠️ Sort-metric regressions for rose charts go undetected. ⚠️ Tests only validate partial legacy ordering behavior.

Steps of Reproduction ✅

1. Review `superset-frontend/plugins/legacy-plugin-chart-rose/test/buildQuery.test.ts:22-30`, where `formData` defines a simple Rose chart with `metrics: ['sum__num']` but no `timeseries_limit_metric`, matching only the default first-metric ordering case. 2. The added test at lines 32-37 asserts that `buildQuery(formData)` yields `query.orderby = [['sum__num', true]]` and flips to `[['sum__num', false]]` when `order_desc: true`, but never configures or inspects a `timeseries_limit_metric` value. 3. Compare this with the Rose Query controls in `superset-frontend/plugins/legacy-plugin-chart-rose/src/controlPanel.tsx:35-42`, which include `['limit', 'timeseries_limit_metric']`, and with the legacy `NVD3TimeSeriesViz.query_obj()` behavior in `superset/viz.py:31-42` that prefers `timeseries_limit_metric` over the first metric and ensures it is present in `metrics`. 4. Because no test exercises the `timeseries_limit_metric` path, the current regression in `superset-frontend/plugins/legacy-plugin-chart-rose/src/buildQuery.ts:59-72`—which always orders by `firstMetric` and ignores `timeseries_limit_metric`—passes all tests; similarly, any future attempts to fix or refactor this behavior could accidentally reintroduce the bug without being caught unless tests are extended to assert ordering by the configured sort metric with fallback to the first metric.

(Use Cmd/Ctrl + Click for best experience)

Prompt for AI Agent 🤖

This is a comment left during a code review. **Path:** superset-frontend/plugins/legacy-plugin-chart-rose/test/buildQuery.test.ts **Line:** 32:37 **Comment:** *Logic Error: This new test validates only first-metric ordering and omits the `timeseries_limit_metric` case, so it enforces an incomplete behavior model and misses the key legacy branch. Extend the test to assert ordering by the configured sort metric (with fallback to first metric) in both sort directions. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix

👍 | 👎

codecov · 2026-07-03T07:11:15Z

Codecov Report

❌ Patch coverage is 87.50000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 64.73%. Comparing base (2a73fbd) to head (bf01764).
⚠️ Report is 1 commits behind head on remove-legacy-viz-pipeline.

Files with missing lines	Patch %	Lines
...plugins/legacy-plugin-chart-rose/src/buildQuery.ts	66.66%	1 Missing ⚠️

Additional details and impacted files

@@                     Coverage Diff                     @@
##           remove-legacy-viz-pipeline   #41732   +/-   ##
===========================================================
  Coverage                       64.73%   64.73%           
===========================================================
  Files                            2704     2704           
  Lines                          148947   148954    +7     
  Branches                        34412    34415    +3     
===========================================================
+ Hits                            96421    96427    +6     
- Misses                          50764    50765    +1     
  Partials                         1762     1762

Flag	Coverage Δ
javascript	`69.57% <87.50%> (+<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.

… rose (#41732) Co-authored-by: Claude Code <noreply@anthropic.com>

pull-request-size Bot added the size/M label Jul 3, 2026

dosubot Bot added viz:charts:horizon Related to the Horizon chart viz:nvd3 labels Jul 3, 2026

github-actions Bot added the plugins label Jul 3, 2026

codeant-ai-for-open-source Bot reviewed Jul 3, 2026

View reviewed changes

rusackas merged commit d4d3e83 into remove-legacy-viz-pipeline Jul 3, 2026
62 checks passed

rusackas deleted the legacy-viz-fix-line-ordering branch July 3, 2026 07:16

rusackas mentioned this pull request Jul 3, 2026

chore(viz): remove legacy explore_json + viz.py pipeline #41714

Open

9 tasks

rusackas added a commit that referenced this pull request Jul 7, 2026

fix(legacy-viz): restore line-engine default ordering for horizon and…

b55ffa0

… rose (#41732) Co-authored-by: Claude Code <noreply@anthropic.com>

rusackas added a commit that referenced this pull request Jul 7, 2026

fix(legacy-viz): restore line-engine default ordering for horizon and…

752825e

… rose (#41732) Co-authored-by: Claude Code <noreply@anthropic.com>

rusackas added a commit that referenced this pull request Jul 8, 2026

fix(legacy-viz): restore line-engine default ordering for horizon and…

c1b2e2f

… rose (#41732) Co-authored-by: Claude Code <noreply@anthropic.com>

rusackas added a commit that referenced this pull request Jul 10, 2026

fix(legacy-viz): restore line-engine default ordering for horizon and…

d39a588

… rose (#41732) Co-authored-by: Claude Code <noreply@anthropic.com>

rusackas added a commit that referenced this pull request Jul 13, 2026

fix(legacy-viz): restore line-engine default ordering for horizon and…

be4663b

… rose (#41732) Co-authored-by: Claude Code <noreply@anthropic.com>

rusackas added a commit that referenced this pull request Jul 14, 2026

fix(legacy-viz): restore line-engine default ordering for horizon and…

cd75bf6

… rose (#41732) Co-authored-by: Claude Code <noreply@anthropic.com>

rusackas added a commit that referenced this pull request Jul 15, 2026

fix(legacy-viz): restore line-engine default ordering for horizon and…

6a3060b

… rose (#41732) Co-authored-by: Claude Code <noreply@anthropic.com>

rusackas added a commit that referenced this pull request Jul 15, 2026

fix(legacy-viz): restore line-engine default ordering for horizon and…

9c40e10

… rose (#41732) Co-authored-by: Claude Code <noreply@anthropic.com>

rusackas added a commit that referenced this pull request Jul 16, 2026

fix(legacy-viz): restore line-engine default ordering for horizon and…

f2ee0de

… rose (#41732) Co-authored-by: Claude Code <noreply@anthropic.com>

rusackas added a commit that referenced this pull request Jul 17, 2026

fix(legacy-viz): restore line-engine default ordering for horizon and…

570b9a1

… rose (#41732) Co-authored-by: Claude Code <noreply@anthropic.com>

rusackas added a commit that referenced this pull request Jul 20, 2026

fix(legacy-viz): restore line-engine default ordering for horizon and…

3eb9684

… rose (#41732) Co-authored-by: Claude Code <noreply@anthropic.com>

rusackas added a commit that referenced this pull request Jul 20, 2026

fix(legacy-viz): restore line-engine default ordering for horizon and…

7e1c30c

… rose (#41732) Co-authored-by: Claude Code <noreply@anthropic.com>

rusackas added a commit that referenced this pull request Jul 20, 2026

fix(legacy-viz): restore line-engine default ordering for horizon and…

db233ac

… rose (#41732) Co-authored-by: Claude Code <noreply@anthropic.com>

rusackas added a commit that referenced this pull request Jul 21, 2026

fix(legacy-viz): restore line-engine default ordering for horizon and…

945d499

… rose (#41732) Co-authored-by: Claude Code <noreply@anthropic.com>

rusackas added a commit that referenced this pull request Jul 21, 2026

fix(legacy-viz): restore line-engine default ordering for horizon and…

8feed98

… rose (#41732) Co-authored-by: Claude Code <noreply@anthropic.com>

rusackas added a commit that referenced this pull request Jul 23, 2026

fix(legacy-viz): restore line-engine default ordering for horizon and…

2a55ad7

… rose (#41732) Co-authored-by: Claude Code <noreply@anthropic.com>

rusackas added a commit that referenced this pull request Jul 27, 2026

fix(legacy-viz): restore line-engine default ordering for horizon and…

8948b5a

… rose (#41732) Co-authored-by: Claude Code <noreply@anthropic.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(legacy-viz): restore line-engine default ordering for horizon and rose - #41732

fix(legacy-viz): restore line-engine default ordering for horizon and rose#41732
rusackas merged 1 commit into
remove-legacy-viz-pipelinefrom
legacy-viz-fix-line-ordering

rusackas commented Jul 3, 2026

Uh oh!

bito-code-review Bot commented Jul 3, 2026 •

edited

Loading

Uh oh!

codeant-ai-for-open-source Bot Jul 3, 2026

Uh oh!

codeant-ai-for-open-source Bot Jul 3, 2026

Uh oh!

codeant-ai-for-open-source Bot Jul 3, 2026

Uh oh!

codeant-ai-for-open-source Bot Jul 3, 2026

Uh oh!

codecov Bot commented Jul 3, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		// legacy engine default: order by the first metric ascending
		expect(query.orderby).toEqual([['sum__num', true]]);

Uh oh!

Conversation

rusackas commented Jul 3, 2026

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

Uh oh!

bito-code-review Bot commented Jul 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codeant-ai-for-open-source Bot Jul 3, 2026

Choose a reason for hiding this comment

Uh oh!

codeant-ai-for-open-source Bot Jul 3, 2026

Choose a reason for hiding this comment

Uh oh!

codeant-ai-for-open-source Bot Jul 3, 2026

Choose a reason for hiding this comment

Uh oh!

codeant-ai-for-open-source Bot Jul 3, 2026

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Jul 3, 2026

Codecov Report

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

bito-code-review Bot commented Jul 3, 2026 •

edited

Loading