bito-code-review[bot] commented on code in PR #41184:
URL: https://github.com/apache/superset/pull/41184#discussion_r3565629800
##########
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:
<!-- Bito Reply -->
The suggestion to forward `time_grain_sqla` to `extras.time_grain_sqla` is
correct. While `extra_form_data` is extracted, it must be explicitly included
in the `extras` object returned by `buildQuery` for the database engine to
receive and apply the dashboard-level override. The existing test confirms this
requirement, and the current implementation is missing this assignment.
**superset-frontend/plugins/plugin-chart-pivot-table/src/plugin/buildQuery.ts**
```
const { extra_form_data } = formData;
const time_grain_sqla =
extra_form_data?.time_grain_sqla || formData.time_grain_sqla;
// ...
return {
...query,
extras: {
...query.extras,
time_grain_sqla,
},
};
```
##########
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:
<!-- Bito Reply -->
The assertion `expect(grandTotalCells[0]).toHaveTextContent('4')` in the
test file
`superset-frontend/plugins/plugin-chart-pivot-table/test/react-pivottable/tableRenders.test.tsx`
does not perform a substring match by default. The `toHaveTextContent` matcher
in Jest/Testing Library checks if the element's text content contains the
provided string, but it is sensitive to the exact string representation. If the
formatter renders the value as '4.00' instead of '4', the assertion will fail
because '4' is not equal to '4.00'.
**superset-frontend/plugins/plugin-chart-pivot-table/test/react-pivottable/tableRenders.test.tsx**
```
// 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');
```
##########
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:
<!-- Bito Reply -->
The suggestion to add `this.subtotals.rowEnabled` and
`this.subtotals.colEnabled` checks to the metric-collapse logic is valid. This
ensures that the metric-collapse totals are only mirrored when the
corresponding subtotals are enabled, maintaining consistency with the guard
pattern used elsewhere in the `processRecord` method.
--
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]
