codeant-ai-for-open-source[bot] commented on code in PR #41732:
URL: https://github.com/apache/superset/pull/41732#discussion_r3518100170
##########
superset-frontend/plugins/legacy-plugin-chart-horizon/src/buildQuery.ts:
##########
@@ -16,18 +16,28 @@
* specific language governing permissions and limitations
* under the License.
*/
-import { buildQueryContext, QueryFormData } from '@superset-ui/core';
+import {
+ buildQueryContext,
+ ensureIsArray,
+ QueryFormData,
+} from '@superset-ui/core';
/**
- * Mirrors the legacy HorizonViz query: a plain timeseries query grouped
- * by the dimensions. Series limiting (the `limit` control) and
- * order_desc are handled by buildQueryObject.
+ * Mirrors the legacy HorizonViz query (via NVD3TimeSeriesViz): a
+ * timeseries query grouped by the dimensions, ordered by the first
+ * metric ascending unless order_desc. Series limiting (the `limit`
+ * control) is handled by buildQueryObject.
*/
export default function buildQuery(formData: QueryFormData) {
- return buildQueryContext(formData, baseQueryObject => [
- {
- ...baseQueryObject,
- is_timeseries: true,
- },
- ]);
+ const { order_desc } = formData;
+ return buildQueryContext(formData, baseQueryObject => {
+ const firstMetric = ensureIsArray(baseQueryObject.metrics)[0];
+ return [
+ {
+ ...baseQueryObject,
+ is_timeseries: true,
+ orderby: firstMetric ? [[firstMetric, !order_desc]] : undefined,
Review Comment:
**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]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
❌ Horizon charts ignore configured series sort metric.
❌ Series limiting uses wrong metric, changing visible series.
⚠️ Legacy NVD3TimeSeries ordering parity is not preserved.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
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.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=1c0d16ea01864fe1831f1a19efc4da60&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=1c0d16ea01864fe1831f1a19efc4da60&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
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
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41732&comment_hash=6a8f56bf734bc936ed8a2eabb04220577982b56335a9ec62d25030a6da3cc585&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41732&comment_hash=6a8f56bf734bc936ed8a2eabb04220577982b56335a9ec62d25030a6da3cc585&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/legacy-plugin-chart-horizon/test/buildQuery.test.ts:
##########
@@ -37,4 +37,11 @@ test('builds a grouped timeseries query with a series
limit', () => {
expect(query.is_timeseries).toBe(true);
expect(query.series_limit).toEqual(25);
expect(query.granularity).toEqual('ds');
+ // legacy engine default: order by the first metric ascending
+ expect(query.orderby).toEqual([['sum__num', true]]);
Review Comment:
**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]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
⚠️ 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.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
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.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=ccf19a384baa4cf79f17a5dbdcb84c8f&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=ccf19a384baa4cf79f17a5dbdcb84c8f&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
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
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41732&comment_hash=4f631427de6f4590fde80717b5ddc62acaaba700411cf0ca240af4387310429b&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41732&comment_hash=4f631427de6f4590fde80717b5ddc62acaaba700411cf0ca240af4387310429b&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/legacy-plugin-chart-rose/test/buildQuery.test.ts:
##########
@@ -29,6 +29,13 @@ const formData: QueryFormData = {
metrics: ['sum__num'],
};
+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]]);
+});
Review Comment:
**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]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
⚠️ Rose buildQuery tests omit timeseries_limit_metric ordering case.
⚠️ Sort-metric regressions for rose charts go undetected.
⚠️ Tests only validate partial legacy ordering behavior.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
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.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=cd3df221fa374acca3df303f1e28d29e&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=cd3df221fa374acca3df303f1e28d29e&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
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
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41732&comment_hash=ad7614623c0503c7f0e175330ffeae2c30a94427389fb3b78d67406468b8e2b6&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41732&comment_hash=ad7614623c0503c7f0e175330ffeae2c30a94427389fb3b78d67406468b8e2b6&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/legacy-plugin-chart-rose/src/buildQuery.ts:
##########
@@ -57,9 +57,18 @@ export default function buildQuery(rawFormData:
QueryFormData) {
: rawFormData.comparison_type,
};
return buildQueryContext(formData, baseQueryObject => {
+ const firstMetric = ensureIsArray(baseQueryObject.metrics)[0];
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)
Review Comment:
**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]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
❌ 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.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
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.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=30d171e11cea4e0cb088813f776b6f07&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=30d171e11cea4e0cb088813f776b6f07&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
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
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41732&comment_hash=ceee09901df4d4b21d149cec9263225b43d7bb35c356a9befa693acf3981c678&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41732&comment_hash=ceee09901df4d4b21d149cec9263225b43d7bb35c356a9befa693acf3981c678&reaction=dislike'>👎</a>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
