codeant-ai-for-open-source[bot] commented on code in PR #41184:
URL: https://github.com/apache/superset/pull/41184#discussion_r3555863956
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/PivotTableChart.tsx:
##########
@@ -341,22 +296,43 @@ export default function PivotTableChart(props:
PivotTableProps) {
const unpivotedData = useMemo(
() =>
- data.reduce(
- (acc: Record<string, any>[], record: Record<string, any>) => [
- ...acc,
- ...metricNames
+ // `data` is now one entry per rollup level. Tag every record with the
+ // row/column dimension labels of the level that produced it (mirroring
+ // the METRIC_KEY injection used for the full rows/cols below) so
+ // PivotData can slot each pre-computed value without re-aggregating.
+ // buildGroupbyCombinations already applied transposePivot, so the
level's
+ // groupby is display-oriented and is not transposed again here.
+ data.flatMap((query: QueryData) => {
+ let levelRows = query.groupby.rows.map(getColumnLabel);
+ let levelCols = query.groupby.columns.map(getColumnLabel);
+ if (metricsLayout === MetricsLayoutEnum.ROWS) {
+ levelRows = combineMetric
+ ? [...levelRows, METRIC_KEY]
+ : [METRIC_KEY, ...levelRows];
+ } else {
+ levelCols = combineMetric
+ ? [...levelCols, METRIC_KEY]
+ : [METRIC_KEY, ...levelCols];
+ }
+ return query.data.flatMap((record: Record<string, any>) =>
+ metricNames
.map((name: string) => ({
...record,
[METRIC_KEY]: name,
value: record[name],
// Mark currency column for per-cell currency detection in
aggregators
__currencyColumn: currencyCodeColumn,
+ // The level this record belongs to (used by PivotData
placement).
+ rows: levelRows,
+ columns: levelCols,
+ // Identify the metric pseudo-dimension so PivotData can feed the
+ // metric-collapsed totals (the opposite "Total" axis + corner).
+ __metricKey: METRIC_KEY,
}))
- .filter(record => record.value !== null),
- ],
- [],
- ),
- [data, metricNames, currencyCodeColumn],
+ .filter(r => r.value !== null),
Review Comment:
**Suggestion:** Filtering out every metric row where the value is null
removes those dimension keys from PivotData entirely, so rows/columns that
legitimately have null metrics disappear instead of rendering as empty cells.
Keep the record and let the renderer show a blank/null value so table structure
remains correct. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Pivot table omits rows with only null metric values.
- ⚠️ Column headers misalign when null rows are pruned.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure a Pivot Table chart in the UI with a metric that can
legitimately return NULL
at some rollup level (e.g., an AVG over empty groups) and run the query,
producing
`queriesData[0].data` with null metric values, which is passed into
`transformProps()` at
`superset-frontend/plugins/plugin-chart-pivot-table/src/plugin/transformProps.ts:54`.
2. Inside `transformProps`, the `data: QueryData[]` structure is built
(additive or
grouping-sets path) at `transformProps.ts:105-144`, preserving rows where
certain metric
fields are `null` and associating each with its `groupby` level.
3. `PivotTableChart` is rendered with those `data` props at
`superset-frontend/plugins/plugin-chart-pivot-table/src/PivotTableChart.tsx:181-215`,
and
computes `unpivotedData` in the `useMemo` block at
`PivotTableChart.tsx:297-335` by
mapping each `record` and `metricNames` entry into `{ ...record,
[METRIC_KEY]: name,
value: record[name], rows, columns, __metricKey }`.
4. The final step in that `useMemo` applies `.filter(r => r.value !== null)`
at
`PivotTableChart.tsx:332`, dropping every record whose metric `value` is
`null`; when all
metrics for a given dimension combination are null at a rollup level, no
records remain
for that row/column in `unpivotedData`, so the `<PivotTable>` rendered at
`PivotTableChart.tsx:671-685` never sees that dimension key and the
corresponding
row/column disappears instead of showing an empty cell.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=25c7b0eaa9fc4fc2aa94a467c8208099&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=25c7b0eaa9fc4fc2aa94a467c8208099&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/PivotTableChart.tsx
**Line:** 332:332
**Comment:**
*Logic Error: Filtering out every metric row where the value is null
removes those dimension keys from PivotData entirely, so rows/columns that
legitimately have null metrics disappear instead of rendering as empty cells.
Keep the record and let the renderer show a blank/null value so table structure
remains correct.
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=a1621286e7161a3e15f5c24652ccc356b8029fba630f7d04678cc2b92a5f0e33&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41184&comment_hash=a1621286e7161a3e15f5c24652ccc356b8029fba630f7d04678cc2b92a5f0e33&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-pivot-table/test/plugin/buildQuery.test.ts:
##########
@@ -56,9 +55,41 @@ const formData: PivotTableQueryFormData = {
currencyFormat: { symbol: 'USD', symbolPosition: 'prefix' },
};
-test('should build groupby with series in form data', () => {
+test('additive metrics use the fast-path: a single full-detail query', () => {
+ const { queries } = buildQuery({
+ ...formData,
+ metrics: [
+ {
+ expressionType: 'SIMPLE',
+ aggregate: 'SUM',
+ column: { column_name: 'num' },
+ label: 'sum_num',
+ },
+ ] as any,
+ });
+ expect(queries).toHaveLength(1);
+ // The single leaf query carries all dimensions (2 rows + 2 cols).
+ expect(queries[0].columns).toHaveLength(4);
+});
+
+test('non-additive metrics emit a single GROUPING SETS query with all levels',
() => {
+ // 2 row dims x 2 col dims -> (2+1) x (2+1) = 9 rollup levels, carried as
+ // grouping_sets on a single query (saved-metric strings are non-additive).
const queryContext = buildQuery(formData);
+ expect(queryContext.queries).toHaveLength(1);
const [query] = queryContext.queries;
+ // The single query selects the full set of dimensions ...
+ expect(query.columns).toHaveLength(4);
+ // ... and requests every rollup level via grouping_sets (grand total = []).
+ expect((query as any).grouping_sets).toHaveLength(9);
+ expect((query as any).grouping_sets[0]).toEqual([]);
+ expect((query as any).grouping_sets[8]).toHaveLength(4);
+});
+
+test('should build groupby with series in form data', () => {
+ const queryContext = buildQuery(formData);
+ // Multi-query rollup: the full-detail level is the last query (grand total
is first).
+ const query = queryContext.queries[queryContext.queries.length - 1];
Review Comment:
**Suggestion:** The inline comment says this path is a “multi-query rollup,”
but the new behavior in this file asserts a single-query approach with grouping
sets; this contradiction makes the test misleading and can cause future
regressions to be misinterpreted. Update the comment to match the actual
single-query contract. [comment mismatch]
<details>
<summary><b>Severity Level:</b> Minor 🧹</summary>
```mdx
- ⚠️ Pivot table buildQuery tests misdocument single-query behavior.
- ⚠️ Future refactors may target non-existent multi-query rollup.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open
`superset-frontend/plugins/plugin-chart-pivot-table/test/plugin/buildQuery.test.ts`
and
locate the test `non-additive metrics emit a single GROUPING SETS query with
all levels`
at lines 75-87, which asserts `queryContext.queries` has length 1, i.e. the
pivot-table
buildQuery path produces a single query for non-additive metrics.
2. In the same file, locate the subsequent test `should build groupby with
series in form
data` at lines 89-105; inside it, lines 91-92 contain the comment `//
Multi-query rollup:
the full-detail level is the last query (grand total is first).` immediately
before `const
query = queryContext.queries[queryContext.queries.length - 1];`, implying
multiple queries
are expected.
3. Open
`superset-frontend/plugins/plugin-chart-pivot-table/src/plugin/buildQuery.ts`
and
examine `buildQuery` at lines 64-112: the function always calls
`buildQueryContext(formData, ...)` with a callback that returns a
single-element array `[
{ ...baseQueryObject, columns: fullColumns, ...(groupingSets ? {
grouping_sets:
groupingSets } : {}) } ]`, proving that the frontend now always emits
exactly one query
object, regardless of additive vs non-additive metrics.
4. Open `superset/common/query_context_processor.py` and inspect
`get_query_result` at
lines 19-27: even when `query_object.grouping_sets` is set and
`_supports_grouping_sets()`
is false, the multi-query fallback `_grouping_sets_fallback` at lines 33-64
is internal to
the processor; callers (including chart data and tests) still operate on a
single
`QueryObject` in `QueryContext.queries`, confirming the test comment’s
“Multi-query
rollup” wording is outdated and misleading relative to the current
single-query contract.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=00337b91326b4822b37d537c7ddad02b&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=00337b91326b4822b37d537c7ddad02b&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/test/plugin/buildQuery.test.ts
**Line:** 91:92
**Comment:**
*Comment Mismatch: The inline comment says this path is a “multi-query
rollup,” but the new behavior in this file asserts a single-query approach with
grouping sets; this contradiction makes the test misleading and can cause
future regressions to be misinterpreted. Update the comment to match the actual
single-query 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%2F41184&comment_hash=f15f963d14efd8f8587367812795cc0cebcec815da71f3548ee4713ce572b402&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41184&comment_hash=f15f963d14efd8f8587367812795cc0cebcec815da71f3548ee4713ce572b402&reaction=dislike'>👎</a>
##########
superset/charts/schemas.py:
##########
@@ -1358,6 +1358,17 @@ class Meta: # pylint: disable=too-few-public-methods
load_default=False,
allow_none=True,
)
+ grouping_sets = fields.List(
+ fields.List(fields.String()),
+ metadata={
+ "description": "Rollup levels for non-additive totals: each entry
is "
+ "the list of groupby columns to group at that level (e.g. the
empty "
+ "list is the grand total). When set and the engine supports it,
the "
+ "levels are computed in a single GROUPING SETS query.",
+ },
+ load_default=None,
+ allow_none=True,
+ )
Review Comment:
**Suggestion:** The new `grouping_sets` field is fully user-controlled and
has no size/shape validation. On engines without native `GROUPING SETS`, the
backend executes one query per requested level, so a large payload can amplify
into many DB queries and exhaust resources. Add validation to cap the number of
levels and ensure each level is a subset of declared groupby columns.
[performance]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Chart data API can run unbounded per-level DB queries.
- ⚠️ Slow pivot or table charts on unsupported GROUPING SETS engines.
- ⚠️ Potential database resource exhaustion via oversized grouping_sets
payloads.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open `superset/charts/schemas.py` and inspect
`ChartDataQueryObjectSchema` starting at
line 1 (offset 1227); at lines 135-145 the new field `grouping_sets =
fields.List(fields.List(fields.String()), load_default=None,
allow_none=True)` is defined
with only type information and description, but no `validate` constraints on
list length,
element count, or allowed values.
2. Observe that `ChartDataQueryContextSchema` at lines 6-15 (offset 1498)
has a `queries =
fields.List(fields.Nested(ChartDataQueryObjectSchema))` field and a
`@post_load` method
`make_query_context` at lines 29-33 that constructs a `QueryContext`; this
schema is used
wherever chart-style query contexts are created, including in
`superset/mcp_service/chart/tool/get_chart_data.py` at lines 5-7 and 612,
and in the
public `/api/v1/chart/data` endpoint documented in
`docs/developer_docs/api.mdx` lines
115-116.
3. Open `superset/common/query_object.py` and inspect `QueryObject.__init__`
at lines
17-42 (offset 120): it accepts `grouping_sets: list[list[str]] | None =
None` and stores
it as `self.grouping_sets = grouping_sets or []`, meaning any user-supplied
`grouping_sets` list from the schema becomes a concrete list of levels on
the query object
with no additional validation.
4. Open `superset/common/query_context_processor.py` and examine
`get_query_result` at
lines 19-27 and `_grouping_sets_fallback` at lines 33-64: when
`query_object.grouping_sets` is non-empty and `_supports_grouping_sets()`
(lines 29-31)
returns false for the datasource’s engine spec, the processor runs
`_grouping_sets_fallback`, which does `levels = query_object.grouping_sets`
and then loops
`for level in levels:` issuing
`self._qc_datasource.get_query_result(sub_query)` once per
level. Because `grouping_sets` is fully user-controlled via
`ChartDataQueryObjectSchema`
with no size limits, a client can send a `/api/v1/chart/data` request whose
`queries[0].grouping_sets` contains hundreds of levels, causing the fallback
path on
engines without native GROUPING SETS support to execute hundreds of backend
SQL queries
for a single chart data request, amplifying load without any server-side cap.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=45a425ef9115430c9aef055bde281392&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=45a425ef9115430c9aef055bde281392&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/charts/schemas.py
**Line:** 1361:1371
**Comment:**
*Performance: The new `grouping_sets` field is fully user-controlled
and has no size/shape validation. On engines without native `GROUPING SETS`,
the backend executes one query per requested level, so a large payload can
amplify into many DB queries and exhaust resources. Add validation to cap the
number of levels and ensure each level is a subset of declared groupby columns.
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=14dc8dae4a21eef37cc698c193df7f78cf6376e79fb6cb8deefa23fec520b930&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41184&comment_hash=14dc8dae4a21eef37cc698c193df7f78cf6376e79fb6cb8deefa23fec520b930&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]
