rusackas commented on code in PR #41184:
URL: https://github.com/apache/superset/pull/41184#discussion_r3565629696
##########
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:
That is not right, `buildQueryContext` already populates
`extras.time_grain_sqla` from `formData.time_grain_sqla` by default. The
`time_grain_sqla` here is only used to tag the BASE_AXIS column, and there is
already a passing test (`should not omit extras.time_grain_sqla from
queryContext so dashboards apply them`) covering the override.
##########
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:
`toHaveTextContent` does a substring match by default, so `4.00` satisfies
`toHaveTextContent(4)`. Test passes as written.
##########
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:
Different toggle: `subtotals.colEnabled`/`rowEnabled` gate intermediate
subtotal levels, not the fully-collapsed totals level this mirrors into. When
`colTotals`/`rowTotals` are off, `buildGroupbyCombinations` never even requests
that level, so `processRecord` never sees these records to begin with.
--
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]
