amaannawab923 commented on code in PR #41754:
URL: https://github.com/apache/superset/pull/41754#discussion_r3529843821
##########
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:
small correctness thing. this only guards against totals being undefined,
but if an aggregate totals query comes back as an empty `{}` it would still pin
a blank summary row, since `{} !== undefined` is true. we might want
`Object.keys(totals).length > 0` here to cover that case.
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTableChart.tsx:
##########
@@ -113,26 +114,58 @@ export default function TableChart<D extends DataRecord =
DataRecord>(
}
}, [columns]);
+ // A single effect owns every ownState write derived from render state.
+ // updateTableOwnState replaces ownState wholesale, so separate effects that
+ // each spread serverPaginationData in the same render would clobber one
+ // another's keys: clamping the current page, priming the raw-mode summary
+ // columns and nudging a re-query for missing totals must be one combined
+ // delta.
useEffect(() => {
- if (!serverPagination || !serverPaginationData || !rowCount) return;
+ const nextOwnState = { ...serverPaginationData };
+ let changed = false;
+
+ if (serverPagination && serverPaginationData && rowCount !== undefined) {
+ const currentPage = serverPaginationData.currentPage ?? 0;
+ const currentPageSize = serverPaginationData.pageSize ??
serverPageLength;
+ const totalPages = Math.ceil(rowCount / currentPageSize);
+ // An empty result set clamps to page zero; a shrunken one clamps to its
+ // last remaining page.
+ const clampedPage = Math.max(0, Math.min(currentPage, totalPages - 1));
+ if (clampedPage !== currentPage) {
+ nextOwnState.currentPage = clampedPage;
+ changed = true;
+ }
+ }
- const currentPage = serverPaginationData.currentPage ?? 0;
- const currentPageSize = serverPaginationData.pageSize ?? serverPageLength;
- const totalPages = Math.ceil(rowCount / currentPageSize);
+ const primed = (serverPaginationData?.rawSummaryColumns ?? []) as string[];
+ const requested = Boolean(serverPaginationData?.totalsRequested);
+ if (isRawRecords && showTotals && !isEqual(primed, rawSummaryColumns)) {
+ nextOwnState.rawSummaryColumns = rawSummaryColumns;
+ changed = true;
+ }
+ // A renderTrigger toggle re-renders without re-querying; requesting totals
+ // through ownState dispatches the standard re-query whose buildQuery
+ // carries the totals query for the active mode.
+ if (showTotals && totals === undefined && !requested) {
Review Comment:
not a blocker, just want to confirm the intent here. since the checkbox is a
renderTrigger, a saved raw + summary chart does a priming re-query after the
base load, and toggling off fires a query just to reset totalsRequested. its
reasonable given buildQuery cant see column types and you already called it out
in the description, so im mostly flagging the extra round trips, especially the
reset only one, so its a conscious call. all good if the answer is yes, thats
acceptable.
##########
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:
physical columns resolve fine from just `{ column_name }`, but
transformProps also lets calculated columns into rawSummaryColumns and none of
the new tests cover that path. can we add a rawSummaryColumns case backed by a
calculated dataset column, just to be sure the backend actually resolves a
SIMPLE SUM off the name and doesnt need the column's sql expression.
--
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]
