bito-code-review[bot] commented on code in PR #41184:
URL: https://github.com/apache/superset/pull/41184#discussion_r3565603918
##########
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;
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Dashboard time grain override not propagated</b></div>
<div id="fix">
The `time_grain_sqla` value resolved from `extra_form_data` (lines 66–67) is
used to build `getQueryColumns` columns but is never forwarded to
`extras.time_grain_sqla` on the query object returned at line 109.
Dashboard-level time grain overrides are therefore not applied by the database,
despite being extracted. The existing test at line 178
(`expect(query.extras?.time_grain_sqla).toEqual(...)`) will catch this
regression once the fix is in place.
</div>
</div>
<small><i>Code Review Run #90e9f1</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/plugins/plugin-chart-pivot-table/test/react-pivottable/tableRenders.test.tsx:
##########
@@ -149,18 +202,83 @@ test('TableRenderer renders col totals row when colTotals
is enabled', () => {
test('TableRenderer renders grand total when both totals are enabled', () => {
const props = buildDefaultProps({
+ data: TAGGED_COUNT_DATA,
+ vals: ['value'],
tableOptions: { rowTotals: true, colTotals: true },
});
renderWithTheme(<TableRenderer {...props} />);
- // The grand total cell should show "4" (total record count).
+ // The grand total cell shows the DB-computed "4" (total record count).
const grandTotalCells = screen
.getAllByRole('gridcell')
.filter(cell => cell.classList.contains('pvtGrandTotal'));
expect(grandTotalCells.length).toBe(1);
expect(grandTotalCells[0]).toHaveTextContent('4');
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Wrong assertion format</b></div>
<div id="fix">
The grand total assertion expects raw `'4'` but the formatter (default:
`numberFormat` with `digitsAfterDecimal: 2`) will render `4` as `'4.00'`. Other
tests in this diff correctly use `'1.00'` and `'2.00'`. This assertion will
fail at runtime.
</div>
</div>
<small><i>Code Review Run #90e9f1</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/TableRenderers.tsx:
##########
@@ -94,7 +94,6 @@ interface SubtotalOptions {
interface TableRendererProps {
cols: string[];
rows: string[];
- aggregatorName: string;
tableOptions?: TableOptions;
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>CWE-693: Protection Mechanism Bypassed</b></div>
<div id="fix">
Add `aggregatorName: string;` back to the TableRendererProps interface and
reintroduce `aggregatorName` in props destructuring so it can be used in total
headers. (See also: [CWE-693](https://cwe.mitre.org/data/definitions/693.html))
</div>
</div>
<small><i>Code Review Run #90e9f1</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/TableRenderers.tsx:
##########
@@ -1099,9 +1096,7 @@ export function TableRenderer(props: TableRendererProps) {
true,
)}
>
- {t('Total (%(aggregatorName)s)', {
- aggregatorName: t(aggregatorName),
- })}
+ {t('Total')}
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>CWE-693: Protection Mechanism Bypassed</b></div>
<div id="fix">
Restore the translation pattern `{t('Total (%(aggregatorName)s)', {
aggregatorName: t(aggregatorName) })}` for the column grand total header after
ensuring `aggregatorName` is available in props. (See also:
[CWE-693](https://cwe.mitre.org/data/definitions/693.html))
</div>
</div>
<small><i>Code Review Run #90e9f1</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/TableRenderers.tsx:
##########
@@ -1183,11 +1177,7 @@ export function TableRenderer(props: TableRendererProps)
{
true,
)}
>
- {settingsColAttrs.length === 0
- ? t('Total (%(aggregatorName)s)', {
- aggregatorName: t(aggregatorName),
- })
- : null}
+ {settingsColAttrs.length === 0 ? t('Total') : null}
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>CWE-693: Protection Mechanism Bypassed</b></div>
<div id="fix">
Restore the translation pattern `{settingsColAttrs.length === 0 ? t('Total
(%(aggregatorName)s)', { aggregatorName: t(aggregatorName) }) : null}` for the
row grand total header, after making `aggregatorName` available. (See also:
[CWE-693](https://cwe.mitre.org/data/definitions/693.html))
</div>
</div>
<small><i>Code Review Run #90e9f1</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/TableRenderers.tsx:
##########
@@ -1448,9 +1437,7 @@ export function TableRenderer(props: TableRendererProps) {
true,
)}
>
- {t('Total (%(aggregatorName)s)', {
- aggregatorName: t(aggregatorName),
- })}
+ {t('Total')}
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>CWE-693: Protection Mechanism Bypassed</b></div>
<div id="fix">
Restore `{t('Total (%(aggregatorName)s)', { aggregatorName:
t(aggregatorName) })}` for the row total column header after reintroducing
`aggregatorName` in props. (See also:
[CWE-693](https://cwe.mitre.org/data/definitions/693.html))
</div>
</div>
<small><i>Code Review Run #90e9f1</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/utilities.ts:
##########
@@ -1088,76 +1117,108 @@ 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. The `__` prefix
+ // keeps these rollup tags from colliding with a real dataset column
+ // named `rows`/`columns`.
+ 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:
<div>
<div id="suggestion">
<div id="issue"><b>Unguarded metric-collapse totals</b></div>
<div id="fix">
Metric-collapse logic (lines 1211-1222) runs unconditionally even when
subtotals are disabled, contradicting the subtotal-guard pattern established at
lines 1151-1160. Add this.subtotals row/colEnabled checks.
</div>
</div>
<small><i>Code Review Run #90e9f1</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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]
