codeant-ai-for-open-source[bot] commented on code in PR #41184:
URL: https://github.com/apache/superset/pull/41184#discussion_r3565566152
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/plugin/transformProps.ts:
##########
@@ -88,20 +98,63 @@ export default function transformProps(chartProps:
ChartProps<QueryFormData>) {
emitCrossFilters,
theme,
} = chartProps;
- const {
- data,
- colnames,
- coltypes,
- detected_currency: detectedCurrency,
- } = queriesData[0];
+ const groupbyCombinations = buildGroupbyCombinations(
+ formData as PivotTableQueryFormData,
+ );
+ const metricsArr = ensureIsArray(formData.metrics);
+ let data: QueryData[];
+ if (allMetricsAdditive(metricsArr)) {
+ // Additive fast-path: a single full-detail query was issued; synthesize
+ // each rollup level by reducing the leaf rows on the client (see SIP.md).
+ const leafRows = queriesData[0].data;
+ const metricReducers: Record<string, RollupReducer> = {};
+ metricsArr.forEach(metric => {
+ metricReducers[getMetricLabel(metric)] = additiveReducerFor(metric);
+ });
+ const labelLevels = groupbyCombinations.map(combination => ({
+ rows: combination.rows.map(getColumnLabel),
+ columns: combination.columns.map(getColumnLabel),
+ }));
+ const synthesized = synthesizeAdditiveLevels(
+ leafRows,
+ labelLevels,
+ metricReducers,
+ );
Review Comment:
**Suggestion:** The new additive fast-path is enabled for MIN/MAX metrics
too, but the reducer pipeline it calls coerces metric values to numbers;
MIN/MAX over non-numeric data (for example strings or temporal values) will
therefore synthesize wrong/null totals. Restrict the fast-path to numeric-safe
aggregates or preserve native value types when reducing MIN/MAX. [type error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ MIN/MAX rollup totals null for temporal/string metrics.
- ⚠️ Pivot table diverges from backend MIN/MAX semantics.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Create a Pivot Table chart whose metrics include a MIN or MAX over a
non-numeric
column, for example `MIN(event_time)` on a temporal column; the chart form
data is sent to
the frontend as `formData.metrics` in `ChartProps` consumed by
`transformProps` at
`superset-frontend/plugins/plugin-chart-pivot-table/src/plugin/transformProps.ts:54-100`.
2. In `transformProps`, `metricsArr` is constructed with
`ensureIsArray(formData.metrics)`
at line 104, and `allMetricsAdditive(metricsArr)` at line 106 calls
`isAdditiveMetric` in
`superset-frontend/plugins/plugin-chart-pivot-table/src/plugin/utilities.ts:36-41`,
which
treats SIMPLE metrics with aggregate `MIN` or `MAX` as additive regardless
of the
underlying value type; thus `allMetricsAdditive` returns true when all
metrics are
SUM/COUNT/MIN/MAX, including MIN/MAX over temporal or string columns.
3. Because `allMetricsAdditive(metricsArr)` is true, the additive fast-path
block at
`transformProps.ts:106-123` runs: it builds `metricReducers` using
`additiveReducerFor(metric)` (utilities.ts:58-63, returning
`'min'`/`'max'`), constructs
`labelLevels`, and calls `synthesizeAdditiveLevels(leafRows, labelLevels,
metricReducers)`
(utilities.ts:83-123) to synthesize rollup rows from the full-detail
`queriesData[0].data`.
4. Inside `synthesizeAdditiveLevels` (`plugin/utilities.ts:111-119`), each
metric value is
converted with `Number(v)` and filtered so only entries where `Number(v)` is
not `NaN` are
kept; for MIN/MAX metrics over temporal or string values, every value fails
this numeric
check and `values` becomes empty, so `out[metricKey]` is set to `null` for
all
totals/subtotals. The resulting `data: QueryData[]` (built at
`transformProps.ts:123-126`)
is passed to `PivotTableChart`, which then renders rollup-level cells for
these MIN/MAX
metrics as null/blank, even though the backend could compute correct MIN/MAX
totals over
non-numeric types via GROUPING SETS.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=b9e3ec7e855b43e68f9fb8a941e696f6&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=b9e3ec7e855b43e68f9fb8a941e696f6&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/transformProps.ts
**Line:** 106:122
**Comment:**
*Type Error: The new additive fast-path is enabled for MIN/MAX metrics
too, but the reducer pipeline it calls coerces metric values to numbers;
MIN/MAX over non-numeric data (for example strings or temporal values) will
therefore synthesize wrong/null totals. Restrict the fast-path to numeric-safe
aggregates or preserve native value types when reducing MIN/MAX.
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=7c80a70dad5f4c0ba97ecb84eedf4ebf2638d53c7289ad60fad18c03a0afb521&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41184&comment_hash=7c80a70dad5f4c0ba97ecb84eedf4ebf2638d53c7289ad60fad18c03a0afb521&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/PivotTableChart.tsx:
##########
@@ -383,22 +338,45 @@ export default function PivotTableChart(props:
PivotTableProps) {
const unpivotedData = useMemo(
() =>
- data.reduce(
- (acc: DataRecord[], record: DataRecord) => [
- ...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: DataRecord) =>
+ 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).
+ // Namespaced with a `__` prefix (like `__metricKey` below) so it
+ // can't collide with a real dataset column named
`rows`/`columns`.
+ __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 records where metric value is null drops those
rows from pivot ingestion entirely, so dimension members with null metrics
disappear instead of rendering as empty cells. Keep null-valued records and let
the cell formatter render blank/null output so row/column headers and totals
remain complete. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Pivot table hides groups with all-null metric values.
- ⚠️ Totals/subtotals omit null-only dimension members.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure a Pivot Table chart in Superset with any groupby rows/columns
and a metric
that can legitimately evaluate to NULL for some dimension combinations (e.g.
a ratio
metric or a MIN/MAX over sparse data), then run the chart so the backend
returns grouped
data to the pivot plugin (handled by `transformProps` at
`superset-frontend/plugins/plugin-chart-pivot-table/src/plugin/transformProps.ts:54-144`).
2. On the client, `transformProps` builds the rollup-level `data:
QueryData[]` and passes
it as props into `PivotTableChart` (constructor at
`superset-frontend/plugins/plugin-chart-pivot-table/src/PivotTableChart.tsx:223-257`),
where each `QueryData` entry contains rows for one rollup level, including
rows whose
metric values are `null`.
3. Inside `PivotTableChart`, the `unpivotedData` memo at
`PivotTableChart.tsx:339-377`
expands each `QueryData` into per-metric records, assigning `value:
record[name]` and
tagging the rollup level with `__rows`/`__columns`/`__metricKey` (lines
360-374), then
applies `.filter(r => r.value !== null)` at line 376; any record where the
metric value is
`null` is removed entirely from `unpivotedData`.
4. The `PivotTable` component (from `./react-pivottable`) feeds
`unpivotedData` into
`PivotData.processRecord` (see
`superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/utilities.ts:1119-1223`),
which creates rowKeys/colKeys only for dimension combinations that have at
least one
record; combinations whose metric is `null` for all metrics now have no
records at all and
therefore no rowKey/colKey, so the corresponding row/column headers
disappear from the
rendered pivot instead of showing empty/null cells.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=812b9d9d27a04f2ab1e1c7a35848a18b&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=812b9d9d27a04f2ab1e1c7a35848a18b&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:** 376:376
**Comment:**
*Logic Error: Filtering out records where metric value is null drops
those rows from pivot ingestion entirely, so dimension members with null
metrics disappear instead of rendering as empty cells. Keep null-valued records
and let the cell formatter render blank/null output so row/column headers and
totals remain complete.
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=71f9ec7cb498c02e03979057750ff1c08d1028f181894d59a1f94be99031c6cb&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41184&comment_hash=71f9ec7cb498c02e03979057750ff1c08d1028f181894d59a1f94be99031c6cb&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]
