codeant-ai-for-open-source[bot] commented on code in PR #41184:
URL: https://github.com/apache/superset/pull/41184#discussion_r3565327481
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/plugin/buildQuery.ts:
##########
@@ -54,6 +59,39 @@ export default function buildQuery(formData:
PivotTableQueryFormData) {
}
return col;
});
+}
+
+export default function buildQuery(formData: PivotTableQueryFormData) {
+ const { extra_form_data } = formData;
+ const time_grain_sqla =
+ extra_form_data?.time_grain_sqla || formData.time_grain_sqla;
+
+ // The full set of dimensions (the leaf level) is always queried; the rollup
+ // levels are derived from it. Apply transposePivot here exactly as
+ // buildGroupbyCombinations does, so the column order matches the rollup
+ // levels. See SIP.md.
+ const [fullRows, fullColumnDims] = formData.transposePivot
+ ? [formData.groupbyColumns, formData.groupbyRows]
+ : [formData.groupbyRows, formData.groupbyColumns];
+ const fullGroupby: Groupby = {
+ rows: ensureIsArray<QueryFormColumn>(fullRows),
+ columns: ensureIsArray<QueryFormColumn>(fullColumnDims),
+ };
+ const fullColumns = getQueryColumns(fullGroupby, formData, time_grain_sqla);
+
+ // A single query always suffices:
+ // - additive metrics (SUM/COUNT/MIN/MAX): a full-detail query whose
+ // subtotals/grand totals transformProps derives by client-side reduction.
+ // - non-additive metrics: a GROUPING SETS query (one `grouping_sets` level
+ // per displayed total/subtotal) so the database computes every level; the
+ // backend falls back to per-level queries on engines without native
+ // support. transformProps splits the combined result by level.
+ const additive = allMetricsAdditive(ensureIsArray(formData.metrics));
+ const groupingSets = additive
+ ? undefined
+ : buildGroupbyCombinations(formData).map(level =>
+ [...level.rows, ...level.columns].map(getColumnLabel),
Review Comment:
**Suggestion:** The generated `grouping_sets` levels are not de-duplicated,
so if the same dimension is placed on both axes the same column label can
appear twice in one level. That propagates duplicate expressions into backend
GROUPING SETS generation and can produce invalid SQL or engine-specific
failures. De-duplicate labels per level before sending them. [logic error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
❌ Pivot Table queries can fail when axis dimensions overlap.
⚠️ Backend may generate invalid GROUPING SETS SQL.
⚠️ Non-additive rollups brittle for shared row/column dimensions.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Create a Pivot Table chart where the same dimension (e.g. `state`) is
assigned on both
axes so `formData.groupbyRows` and `formData.groupbyColumns` each contain an
identical
`QueryFormColumn` entry; these arrays are used by
`buildGroupbyCombinations(formData)` in
`superset-frontend/plugins/plugin-chart-pivot-table/src/plugin/utilities.ts:183-231`
without cross-axis deduplication.
2. With any totals/subtotals toggles enabled (`colTotals`, `rowTotals`,
`rowSubTotals`, or
`colSubTotals` true), `buildGroupbyCombinations` generates multiple rollup
levels; for
levels where the prefixes of `rows` and `columns` both include the shared
dimension, the
resulting `Groupby` combination has that dimension in both `rows` and
`columns`.
3. In `buildQuery` at
`superset-frontend/plugins/plugin-chart-pivot-table/src/plugin/buildQuery.ts:89-94`,
`groupingSets` is computed as `buildGroupbyCombinations(formData).map(level
=>
[...level.rows, ...level.columns].map(getColumnLabel))`. When a dimension
appears on both
axes, `[...]` concatenation produces per-level label lists containing the
same column
label multiple times (e.g. `['state', 'state', 'gender']`), and there is no
per-level
deduplication before attaching `grouping_sets` to the query object.
4. On the backend, `SqlaTable.query` in `superset/models/helpers.py:3550-45`
builds native
GROUPING SETS when `grouping_sets` is set and
`db_engine_spec.supports_grouping_sets`; it
translates each level label list into SQL columns via `gs_levels =
[[groupby_all_columns[col] for col in level if col in groupby_all_columns]
for level in
grouping_sets or []]` and then `qry =
qry.group_by(grouping_sets_clause(gs_levels))`.
Because per-level labels are not deduped, `grouping_sets_clause` in
`superset/common/grouping_sets.py:14-27` receives groups where the same
`ColumnElement`
appears multiple times in one tuple, yielding GROUPING SETS clauses with
repeated columns
that can be syntactically invalid or cause engine-specific failures when
executing the
chart query.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=8ed171bb7f93409e9aa72532241a757f&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=8ed171bb7f93409e9aa72532241a757f&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-pivot-table/src/plugin/buildQuery.ts
**Line:** 93:93
**Comment:**
*Logic Error: The generated `grouping_sets` levels are not
de-duplicated, so if the same dimension is placed on both axes the same column
label can appear twice in one level. That propagates duplicate expressions into
backend GROUPING SETS generation and can produce invalid SQL or engine-specific
failures. De-duplicate labels per level before sending them.
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%2F41184&comment_hash=0784cf33ea677289a4495e7e0e4ea0136f9101bc31ad790aa9a5064a6fff4fdd&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41184&comment_hash=0784cf33ea677289a4495e7e0e4ea0136f9101bc31ad790aa9a5064a6fff4fdd&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]
