rusackas commented on code in PR #41184:
URL: https://github.com/apache/superset/pull/41184#discussion_r3557746062
##########
superset-frontend/plugins/plugin-chart-pivot-table/test/react-pivottable/tableRenders.test.tsx:
##########
@@ -103,35 +150,41 @@ test('TableRenderer renders row headers from pivot data',
() => {
});
test('TableRenderer renders aggregated cell values', () => {
- const props = buildDefaultProps();
+ const props = buildDefaultProps({
+ data: TAGGED_COUNT_DATA,
+ vals: ['value'],
+ });
renderWithTheme(<TableRenderer {...props} />);
- // With "Count" aggregator, each cell (row x col intersection) should
- // contain "1" because each combination appears exactly once.
+ // Each leaf cell (row x col intersection) holds the DB-computed value "1".
const cells = screen.getAllByRole('gridcell');
const cellTexts = cells.map(cell => cell.textContent);
- // There should be cell values of "1" for each of the four intersections
- // (blue+circle, blue+square, red+circle, red+square).
- const onesCount = cellTexts.filter(text => text === '1').length;
+ // There should be a "1" leaf cell for each of the four intersections
+ // (blue+circle, blue+square, red+circle, red+square). The default formatter
+ // renders with two decimals in this test (production applies the metric's
+ // own format).
+ const onesCount = cellTexts.filter(text => text === '1.00').length;
Review Comment:
These cells render through the `cellValue` passthrough (pre-computed DB
values) with the default 2-decimal formatter, not `count()`, so `'1.00'` is
what comes out. Tests pass as-is.
##########
superset/charts/schemas.py:
##########
@@ -1358,6 +1358,17 @@ class Meta: # pylint: disable=too-few-public-methods
load_default=False,
allow_none=True,
)
+ grouping_sets = fields.List(
+ fields.List(fields.String()),
+ metadata={
+ "description": "Rollup levels for non-additive totals: each entry
is "
+ "the list of groupby columns to group at that level (e.g. the
empty "
+ "list is the grand total). When set and the engine supports it,
the "
+ "levels are computed in a single GROUPING SETS query.",
+ },
+ load_default=None,
+ allow_none=True,
+ )
Review Comment:
Fair concern. For this POC the levels are generated from the chart's own
groupby combinations; bounding and subset-validating the raw API field is
hardening I want to land with the SIP-216 work rather than here.
##########
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:
Right, and the comment right above calls it out: a cross-metric total isn't
well-defined, so it's intentionally the last metric for the POC and left for
the SIP.
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/PivotTableChart.tsx:
##########
@@ -341,22 +296,43 @@ export default function PivotTableChart(props:
PivotTableProps) {
const unpivotedData = useMemo(
() =>
- data.reduce(
- (acc: Record<string, any>[], record: Record<string, any>) => [
- ...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: Record<string, any>) =>
+ 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).
+ 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:
The filter drops the null-metric rows the rollup/GROUPING SETS split emits
per level. A metric that's legitimately null at a level getting pruned is a
real edge, but one I'm leaving for the SIP work.
--
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]
