codeant-ai-for-open-source[bot] commented on code in PR #41184:
URL: https://github.com/apache/superset/pull/41184#discussion_r3555865333
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/utilities.ts:
##########
@@ -1088,76 +1115,106 @@ class PivotData {
}
processRecord(record: PivotRecord): void {
- // this code is called in a tight loop
+ // this code is called in a tight loop.
+ // Each record is tagged (in PivotTableChart) with `rows`/`columns`: the
+ // dimension labels of the rollup level that produced it. The database has
+ // already aggregated that level, so we place the value into exactly one
+ // slot and store it verbatim (no re-aggregation). A key shorter than the
+ // full dimension list identifies a subtotal level. Records without tags
+ // (e.g. direct unit-test construction) fall back to the full dimension
+ // lists, i.e. they are treated as leaf rows.
+ const recordRows = (record.rows as unknown as string[]) ?? null;
+ const recordColumns = (record.columns as unknown as string[]) ?? null;
+ const levelRows = recordRows ?? (this.props.rows as string[]);
+ const levelColumns = recordColumns ?? (this.props.cols as string[]);
+
const colKey: string[] = [];
const rowKey: string[] = [];
- (this.props.cols as string[]).forEach((col: string) => {
+ levelColumns.forEach((col: string) => {
colKey.push(col in record ? String(record[col]) : 'null');
});
- (this.props.rows as string[]).forEach((row: string) => {
+ levelRows.forEach((row: string) => {
rowKey.push(row in record ? String(record[row]) : 'null');
});
- this.allTotal.push(record);
-
- const rowStart = this.subtotals.rowEnabled ? 1 : Math.max(1,
rowKey.length);
- const colStart = this.subtotals.colEnabled ? 1 : Math.max(1,
colKey.length);
-
- let isRowSubtotal;
- let isColSubtotal;
- for (let ri = rowStart; ri <= rowKey.length; ri += 1) {
- isRowSubtotal = ri < rowKey.length;
- const fRowKey = rowKey.slice(0, ri);
- const flatRowKey = flatKey(fRowKey);
- if (!this.rowTotals[flatRowKey]) {
- this.rowKeys.push(fRowKey);
- this.rowTotals[flatRowKey] = this.getFormattedAggregator(
- record,
- rowKey,
- )(this, fRowKey, []);
+ const flatRowKey = flatKey(rowKey);
+ const flatColKey = flatKey(colKey);
+
+ const isColSubtotal = colKey.length < (this.props.cols as string[]).length;
+ const isRowSubtotal = rowKey.length < (this.props.rows as string[]).length;
+
+ // Register/create the column-total slot the first time this colKey is
seen.
+ if (colKey.length > 0 && !this.colTotals[flatColKey]) {
+ if (!isColSubtotal || this.subtotals.colEnabled) {
+ this.colKeys.push(colKey);
}
- this.rowTotals[flatRowKey].push(record);
- this.rowTotals[flatRowKey].isSubtotal = isRowSubtotal;
+ this.colTotals[flatColKey] = this.getFormattedAggregator(record, colKey)(
+ this,
+ [],
+ colKey,
+ );
}
-
- for (let ci = colStart; ci <= colKey.length; ci += 1) {
- isColSubtotal = ci < colKey.length;
- const fColKey = colKey.slice(0, ci);
- const flatColKey = flatKey(fColKey);
- if (!this.colTotals[flatColKey]) {
- this.colKeys.push(fColKey);
- this.colTotals[flatColKey] = this.getFormattedAggregator(
- record,
+ // Register/create the row-total slot the first time this rowKey is seen.
+ if (rowKey.length > 0 && !this.rowTotals[flatRowKey]) {
+ if (!isRowSubtotal || this.subtotals.rowEnabled) {
+ this.rowKeys.push(rowKey);
+ }
+ this.rowTotals[flatRowKey] = this.getFormattedAggregator(record, rowKey)(
+ this,
+ rowKey,
+ [],
+ );
+ }
+ // Create the body-cell slot.
+ if (rowKey.length > 0 && colKey.length > 0) {
+ if (!this.tree[flatRowKey]) {
+ this.tree[flatRowKey] = {};
+ }
+ if (!this.tree[flatRowKey][flatColKey]) {
+ this.tree[flatRowKey][flatColKey] =
this.getFormattedAggregator(record)(
+ this,
+ rowKey,
colKey,
- )(this, [], fColKey);
+ );
}
+ }
+
+ // Place the value in exactly one slot, determined by the level.
+ if (rowKey.length === 0 && colKey.length === 0) {
+ this.allTotal.push(record);
+ } else if (rowKey.length === 0) {
this.colTotals[flatColKey].push(record);
this.colTotals[flatColKey].isSubtotal = isColSubtotal;
+ } else if (colKey.length === 0) {
+ this.rowTotals[flatRowKey].push(record);
+ this.rowTotals[flatRowKey].isSubtotal = isRowSubtotal;
+ } else {
+ this.tree[flatRowKey][flatColKey].push(record);
+ this.tree[flatRowKey][flatColKey].isRowSubtotal = isRowSubtotal;
+ this.tree[flatRowKey][flatColKey].isColSubtotal = isColSubtotal;
+ this.tree[flatRowKey][flatColKey].isSubtotal =
+ isRowSubtotal || isColSubtotal;
}
- // And now fill in for all the sub-cells.
- for (let ri = rowStart; ri <= rowKey.length; ri += 1) {
- isRowSubtotal = ri < rowKey.length;
- const fRowKey = rowKey.slice(0, ri);
- const flatRowKey = flatKey(fRowKey);
- if (!this.tree[flatRowKey]) {
- this.tree[flatRowKey] = {};
+ // Metric-collapse totals. The metric is a pseudo-dimension always present
on
+ // one axis, so no rollup level produces an empty key on that axis -- which
+ // would leave the opposite "Total" axis and the grand-total corner empty.
+ // When a record's axis holds only the metric (no real dims there), its
value
+ // is also the collapsed total for that axis, so mirror it into rowTotals /
+ // colTotals / allTotal. (For a single metric this equals the metric
column;
+ // for multiple metrics it is the last metric -- a cross-metric total is
not
+ // well defined and is left as future work.)
+ const metricKey = record.__metricKey as unknown as string | undefined;
+ if (metricKey) {
+ const realColCount = levelColumns.filter(c => c !== metricKey).length;
+ const realRowCount = levelRows.filter(r => r !== metricKey).length;
+ if (levelColumns.includes(metricKey) && realColCount === 0) {
+ if (rowKey.length === 0) this.allTotal.push(record);
+ else this.rowTotals[flatRowKey]?.push(record);
}
- for (let ci = colStart; ci <= colKey.length; ci += 1) {
- isColSubtotal = ci < colKey.length;
- const fColKey = colKey.slice(0, ci);
- const flatColKey = flatKey(fColKey);
- if (!this.tree[flatRowKey][flatColKey]) {
- this.tree[flatRowKey][flatColKey] = this.getFormattedAggregator(
- record,
- )(this, fRowKey, fColKey);
- }
- this.tree[flatRowKey][flatColKey].push(record);
-
- this.tree[flatRowKey][flatColKey].isRowSubtotal = isRowSubtotal;
- this.tree[flatRowKey][flatColKey].isColSubtotal = isColSubtotal;
- this.tree[flatRowKey][flatColKey].isSubtotal =
- isRowSubtotal || isColSubtotal;
+ if (levelRows.includes(metricKey) && realRowCount === 0) {
+ if (colKey.length === 0) this.allTotal.push(record);
+ else this.colTotals[flatColKey]?.push(record);
}
Review Comment:
**Suggestion:** The metric-collapse path pushes multiple metric records into
the same total aggregators, but the new passthrough aggregator stores only one
value per cell, so totals for multi-metric pivots are overwritten by whichever
metric is processed last. This produces incorrect row/column/grand totals
whenever metrics are collapsed on one axis. You need a deterministic
aggregation strategy for these totals (or explicitly suppress/disable them)
instead of repeatedly overwriting the same slot. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
❌ Multi-metric pivot totals display only last metric value.
⚠️ Users misinterpret collapsed totals on pivot table chart.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure a Pivot Table chart using `PivotTableChart`
(PivotTableChart.tsx lines
181–215) with at least two metrics in `metrics`, a layout that places the
metric
pseudo-dimension (`METRIC_KEY`) on one axis (`metricsLayout` and
`combineMetric`), and
enable row/column totals (`rowTotals`/`colTotals` true).
2. The backend returns `QueryData` objects; `unpivotedData` in
`PivotTableChart.tsx`
(lines 297–335) flattens them, producing per-metric records that include
`value` (the
metric value), `rows` and `columns` arrays describing the rollup level, and
`__metricKey:
METRIC_KEY` to mark the metric pseudo-dimension.
3. `TableRenderer` passes these records into `PivotData` (TableRenderers.tsx
lines
623–628), which constructs aggregators via the passthrough `cellValue`
template
(utilities.ts lines 393–416) and feeds each record into `processRecord`
(utilities.ts
lines 1117–1219). For rollup levels where the metric axis is the only
dimension, the
metric-collapse logic (utilities.ts lines 1199–1218) mirrors each metric
record into the
same total aggregators (`rowTotals[flatRowKey]`, `colTotals[flatColKey]`,
and `allTotal`)
for a given key.
4. The `cellValue` aggregator used for these totals stores a single `val`
and overwrites
it on every `push` (utilities.ts lines 397–400). When multiple metrics share
the same
collapsed-total slot, their records are pushed sequentially and the last
metric’s `value`
wins. Totals rows and the grand-total corner rendered by `renderTotalsRow` in
`TableRenderers.tsx` (lines 1444–1466) and `getAggregator([], [])`
(utilities.ts lines
1222–1244) therefore show only the last metric’s value instead of a
well-defined
cross-metric total whenever metrics are collapsed on one axis.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=cbfb266c90994384b284f0088f2cd8c4&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=cbfb266c90994384b284f0088f2cd8c4&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/react-pivottable/utilities.ts
**Line:** 1207:1218
**Comment:**
*Logic Error: The metric-collapse path pushes multiple metric records
into the same total aggregators, but the new passthrough aggregator stores only
one value per cell, so totals for multi-metric pivots are overwritten by
whichever metric is processed last. This produces incorrect row/column/grand
totals whenever metrics are collapsed on one axis. You need a deterministic
aggregation strategy for these totals (or explicitly suppress/disable them)
instead of repeatedly overwriting the same slot.
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=09b91a556e09436da6a025207aa9a384b21749cf95c33e9ff63790f1ed4db60a&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41184&comment_hash=09b91a556e09436da6a025207aa9a384b21749cf95c33e9ff63790f1ed4db60a&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]
