codeant-ai-for-open-source[bot] commented on code in PR #42477:
URL: https://github.com/apache/superset/pull/42477#discussion_r3657026184
##########
superset-frontend/src/dashboard/util/charts/getFormDataWithExtraFilters.ts:
##########
@@ -246,13 +246,10 @@ function applyChartSpecificGroupBy(
groupByFormData.target = limitedColumns[1];
}
} else if (chartType === 'sankey_v2') {
- const { limitedColumns } = limitColumnsForChartType(
- chartType,
- groupByColumns,
- );
- groupByFormData.source = limitedColumns[0];
- if (limitedColumns.length > 1) {
- groupByFormData.target = limitedColumns[1];
+ groupByFormData.source = groupByColumns[0];
+ if (groupByColumns.length > 1) {
+ groupByFormData.target = groupByColumns[groupByColumns.length - 1];
+ groupByFormData.intermediate_levels = groupByColumns.slice(1, -1);
}
Review Comment:
**Suggestion:** When the dynamic group-by contains only one column, this
branch updates `source` but leaves any previously configured `target` and
`intermediate_levels` values untouched. Reusing a chart with a one-column
override can therefore issue a Sankey query using stale levels instead of the
requested grouping. Clear the dependent fields, or explicitly handle the
one-column case. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Sankey dashboard overrides retain stale target columns.
- ⚠️ Sankey queries can include unintended intermediate levels.
- ❌ Resulting chart data may not match the selected grouping.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure a `sankey_v2` chart with an existing source, target, and
optional
`intermediate_levels` in its form data.
2. Apply a dashboard or chart group-by override that produces exactly one
column in
`groupByColumns`, causing `applyChartSpecificGroupBy()` in
`superset-frontend/src/dashboard/util/charts/getFormDataWithExtraFilters.ts:248`
to enter
the `sankey_v2` branch.
3. The branch assigns the override column to `groupByFormData.source` at
line 249, but the
`groupByColumns.length > 1` condition at line 250 is false, so lines 251-252
do not
replace or clear `target` and `intermediate_levels`.
4. The resulting form data still contains the previous Sankey target and
intermediate
levels, and the Sankey query/transform path uses those stale fields instead
of
representing the requested single-column grouping.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=60d90275d8784444a183843b778cb84c&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=60d90275d8784444a183843b778cb84c&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/src/dashboard/util/charts/getFormDataWithExtraFilters.ts
**Line:** 249:253
**Comment:**
*Logic Error: When the dynamic group-by contains only one column, this
branch updates `source` but leaves any previously configured `target` and
`intermediate_levels` values untouched. Reusing a chart with a one-column
override can therefore issue a Sankey query using stale levels instead of the
requested grouping. Clear the dependent fields, or explicitly handle the
one-column case.
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%2F42477&comment_hash=153b656d1c8e0698dfaae52ca8a29dd542e93c9dc765e6765f0b965532f8d0ea&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42477&comment_hash=153b656d1c8e0698dfaae52ca8a29dd542e93c9dc765e6765f0b965532f8d0ea&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-echarts/test/Sankey/buildQuery.test.ts:
##########
@@ -0,0 +1,79 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import buildQuery from '../../src/Sankey/buildQuery';
+import { SankeyFormData } from '../../src/Sankey/types';
+
+const baseFormData: SankeyFormData = {
+ colorScheme: 'supersetColors',
+ datasource: '1__table',
+ metric: 'count',
+ source: 'source_col',
+ target: 'target_col',
+ viz_type: 'sankey_v2',
+};
+
+test('two-column form data builds a single source/target groupby', () => {
+ const [query] = buildQuery(baseFormData).queries;
+ expect(query.groupby).toEqual(['source_col', 'target_col']);
+});
+
+test('intermediate levels are included in order between source and target', ()
=> {
+ const [query] = buildQuery({
+ ...baseFormData,
+ intermediate_levels: ['level_1', 'level_2'],
+ }).queries;
+ expect(query.groupby).toEqual([
+ 'source_col',
+ 'level_1',
+ 'level_2',
+ 'target_col',
+ ]);
Review Comment:
**Suggestion:** The PR adds the `intermediate_levels` control and tests that
it expands the query grouping, but no corresponding `buildQuery.ts` change is
included. The production query will continue grouping only by source and
target, so intermediate-level values will not be returned and the new feature
will not work. Update `buildQuery` to include the intermediate columns between
source and target, including the empty-array case. [logic error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Multi-level Sankey queries omit intermediate columns.
- ❌ Intermediate-level flows cannot be rendered from query results.
- ⚠️ New build-query regression tests fail against production code.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run the Sankey build-query tests in
`superset-frontend/plugins/plugin-chart-echarts/test/Sankey/buildQuery.test.ts`;
the test
imports the production function from `src/Sankey/buildQuery.ts` at line 19.
2. The test at lines 36-46 calls `buildQuery()` with `intermediate_levels:
['level_1',
'level_2']`.
3. The current production implementation in
`superset-frontend/plugins/plugin-chart-echarts/src/Sankey/buildQuery.ts`
still constructs
the grouping from only the source and target fields, because no
corresponding production
change is present in the supplied PR diff.
4. The query therefore returns `['source_col', 'target_col']` instead of the
expected
`['source_col', 'level_1', 'level_2', 'target_col']`, causing the new test
to fail and
preventing the transformer from receiving intermediate-level columns in real
Sankey
queries.
5. The empty-array test at lines 49-55 should continue returning the
two-column grouping
after the production implementation is updated.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=02e003b0054c4b99987af477c664ad7a&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=02e003b0054c4b99987af477c664ad7a&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/plugin-chart-echarts/test/Sankey/buildQuery.test.ts
**Line:** 37:46
**Comment:**
*Logic Error: The PR adds the `intermediate_levels` control and tests
that it expands the query grouping, but no corresponding `buildQuery.ts` change
is included. The production query will continue grouping only by source and
target, so intermediate-level values will not be returned and the new feature
will not work. Update `buildQuery` to include the intermediate columns between
source and target, including the empty-array case.
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%2F42477&comment_hash=f4f86f426ac32e2430b0b76de9ceced4b1623b5f49af319bee9fdba1c304ed2e&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42477&comment_hash=f4f86f426ac32e2430b0b76de9ceced4b1623b5f49af319bee9fdba1c304ed2e&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-echarts/test/Sankey/transformProps.test.ts:
##########
@@ -0,0 +1,176 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { ChartProps, DataRecord } from '@superset-ui/core';
+import { supersetTheme } from '@apache-superset/core/theme';
+import type { SankeySeriesOption } from 'echarts/charts';
+import transformProps from '../../src/Sankey/transformProps';
+import { SankeyChartProps } from '../../src/Sankey/types';
+
+type SankeyNode = { name: string; depth?: number };
+type SankeyLink = { source: string; target: string; value: number };
+
+const getSeries = (props: ChartProps): SankeySeriesOption => {
+ const { echartOptions } = transformProps(props as SankeyChartProps);
+ return (echartOptions as { series: SankeySeriesOption }).series;
+};
+
+const getNodes = (props: ChartProps) => getSeries(props).data as SankeyNode[];
+
+const getLinks = (props: ChartProps) => getSeries(props).links as SankeyLink[];
+
+const makeProps = (
+ formDataOverrides: Record<string, unknown>,
+ data: DataRecord[],
+) =>
+ new ChartProps({
+ formData: {
+ colorScheme: 'supersetColors',
+ datasource: '1__table',
+ metric: 'count',
+ source: 'source_col',
+ target: 'target_col',
+ vizType: 'sankey_v2',
+ ...formDataOverrides,
+ },
+ width: 800,
+ height: 600,
+ queriesData: [{ data }],
+ theme: supersetTheme,
+ });
+
+test('two-column mode emits raw pairwise links and node names', () => {
+ const props = makeProps({}, [
+ { source_col: 'a', target_col: 'b', count: 10 },
+ { source_col: 'b', target_col: 'c', count: 5 },
+ ]);
+ expect(getLinks(props)).toEqual([
+ { source: 'a', target: 'b', value: 10 },
+ { source: 'b', target: 'c', value: 5 },
+ ]);
+ const nodes = getNodes(props);
+ expect(nodes.map(node => node.name).sort()).toEqual(['a', 'b', 'c']);
+ // no level prefixes and no depth pinning: cross-row chaining (a→b→c)
+ // must keep working for edge-list datasets
+ nodes.forEach(node => expect(node.depth).toBeUndefined());
+});
+
+test('three-column mode chains adjacent pairs with level-prefixed names', ()
=> {
+ const props = makeProps({ intermediateLevels: ['mid_col'] }, [
+ { source_col: 'a', mid_col: 'm', target_col: 'z', count: 10 },
+ ]);
+ expect(getLinks(props)).toEqual([
+ { source: '0\0a', target: '1\0m', value: 10 },
+ { source: '1\0m', target: '2\0z', value: 10 },
+ ]);
Review Comment:
**Suggestion:** The new tests require multi-level links with level-specific
node identities, but no corresponding `transformProps.ts` change is included in
the PR. The existing transformer will not emit the expected prefixed adjacent
links or depth values, causing these tests to fail and allowing values from
different levels to be merged into the same Sankey node. Implement the
multi-level transformation before enabling this control. [logic error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Multi-level Sankey rendering does not produce adjacent links.
- ❌ Reused values can merge nodes across levels.
- ⚠️ ECharts may reject resulting cyclic Sankey graphs.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run the Sankey transform tests in
`superset-frontend/plugins/plugin-chart-echarts/test/Sankey/transformProps.test.ts`;
the
test helper at lines 37-55 constructs `ChartProps` and invokes the
production transformer
at line 29.
2. The test at lines 73-86 supplies `intermediateLevels: ['mid_col']` and
one row
containing `source_col`, `mid_col`, `target_col`, and `count`.
3. `transformProps` from
`superset-frontend/plugins/plugin-chart-echarts/src/Sankey/transformProps.ts`
receives
those props, but the supplied PR diff contains no production transformer
change
implementing adjacent-pair links, level-prefixed node names, or depth values.
4. The current transformer therefore cannot produce the expected links `0\0a
→ 1\0m` and
`1\0m → 2\0z` asserted at lines 77-80; the test fails when the production
code is
executed.
5. With actual multi-level query data, values reused across levels remain
represented by
the same raw node identity unless the transformer adds level prefixes,
allowing unintended
node merging and potentially producing invalid Sankey cycles.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=7f267ef803fe4b8f9d76ebc84b1550fb&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=7f267ef803fe4b8f9d76ebc84b1550fb&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/plugin-chart-echarts/test/Sankey/transformProps.test.ts
**Line:** 74:80
**Comment:**
*Logic Error: The new tests require multi-level links with
level-specific node identities, but no corresponding `transformProps.ts` change
is included in the PR. The existing transformer will not emit the expected
prefixed adjacent links or depth values, causing these tests to fail and
allowing values from different levels to be merged into the same Sankey node.
Implement the multi-level transformation before enabling this control.
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%2F42477&comment_hash=fdc354c0a42bb2fd15ad9550de0d7f6329c265330d37ad828be5c49e6573ad6b&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42477&comment_hash=fdc354c0a42bb2fd15ad9550de0d7f6329c265330d37ad828be5c49e6573ad6b&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]
