EnxDev commented on code in PR #41754:
URL: https://github.com/apache/superset/pull/41754#discussion_r3529934494
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTableChart.tsx:
##########
@@ -447,7 +480,7 @@ export default function TableChart<D extends DataRecord =
DataRecord>(
isUsingTimeComparison ? renderTimeComparisonVisibility : () => null
}
cleanedTotals={totals || {}}
- showTotals={showTotals}
+ showTotals={showTotals && totals !== undefined}
Review Comment:
Empty {} totals
(https://github.com/apache/superset/pull/41754#discussion_r3529843821) —
confirmed, and there is a concrete path to it: processComparisonTotals returns
{} when the time-comparison totals result has no rows, which slipped past
totals !== undefined and pinned a blank row. The gate is now showTotals &&
totals !== undefined && Object.keys(totals).length > 0, with a RED-first test
(totals = {} → no pinned row). Note the SUM-over-zero-rows case still pins as
intended: the backend returns a row with NULL values there, so the keys are
present.
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/buildQuery.ts:
##########
@@ -661,6 +678,14 @@ const buildQuery: BuildQuery<TableChartFormData> = (
extraQueries.push({
...queryObject,
columns: [],
+ ...(rawSummaryColumns.length > 0 && {
+ metrics: rawSummaryColumns.map(columnName => ({
+ expressionType: 'SIMPLE' as const,
+ aggregate: 'SUM' as const,
+ column: { column_name: columnName },
Review Comment:
Calculated-column coverage
(https://github.com/apache/superset/pull/41754#discussion_r3529843829) — test
added: a calculated column name in all_columns + rawSummaryColumns flows
through the intersection into the SUM metrics. To be precise about what it does
and doesn't prove: a frontend unit test can't exercise backend resolution —
from buildQuery's perspective a calculated column is just another string. The
backend side is the standard SIMPLE adhoc-metric path (column name resolved
through the dataset's columns, expression substituted server-side), which is
the same mechanism Explore's own adhoc metrics use on calculated columns. What
the new test genuinely pins is that the selection intersection stays name-based
and never silently starts assuming physical columns only.
--
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]
