You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by am...@apache.org on 2026/06/24 11:46:46 UTC
(superset) branch fix/97551-value-aggregation-persist created (now da53bf777d8)
This is an automated email from the ASF dual-hosted git repository.
amaannawab923 pushed a change to branch fix/97551-value-aggregation-persist
in repository https://gitbox.apache.org/repos/asf/superset.git
at da53bf777d8 fix(plugin-chart-ag-grid-table): persist Value Aggregation changes without a sort (#97551)
This branch includes the following new commits:
new da53bf777d8 fix(plugin-chart-ag-grid-table): persist Value Aggregation changes without a sort (#97551)
The 1 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
(superset) 01/01: fix(plugin-chart-ag-grid-table): persist Value Aggregation changes without a sort (#97551)
Posted by am...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
amaannawab923 pushed a commit to branch fix/97551-value-aggregation-persist
in repository https://gitbox.apache.org/repos/asf/superset.git
commit da53bf777d8a34c5c05828b88ea149b77a5e6e0d
Author: amaannawab923 <am...@gmail.com>
AuthorDate: Wed Jun 24 17:16:35 2026 +0530
fix(plugin-chart-ag-grid-table): persist Value Aggregation changes without a sort (#97551)
Changing a column's Value Aggregation only persisted if another piece of grid
state (such as a sort) changed at the same time, because the grid-state change
detector hashed only column order, sorts and filters. This matches the
reported workaround (trigger a sort, then Update chart).
Include per-column aggFunc in the capture signature so the change is detected
and saved on its own. Same root cause as #107166.
Add unit tests for the Explore 'change aggregation then Update chart' scenario.
---
.../src/AgGridTable/index.tsx | 11 ++--
.../src/utils/getColumnStateSignature.ts | 53 +++++++++++++++++++
.../test/utils/getColumnStateSignature.test.ts | 61 ++++++++++++++++++++++
3 files changed, 120 insertions(+), 5 deletions(-)
diff --git a/superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTable/index.tsx b/superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTable/index.tsx
index ebb66b0db28..606ad0d685d 100644
--- a/superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTable/index.tsx
+++ b/superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTable/index.tsx
@@ -57,6 +57,7 @@ import { SearchOption, SortByItem } from '../types';
import getInitialSortState, { shouldSort } from '../utils/getInitialSortState';
import getInitialFilterModel from '../utils/getInitialFilterModel';
import reconcileColumnState from '../utils/reconcileColumnState';
+import getColumnStateSignature from '../utils/getColumnStateSignature';
import { PAGE_SIZE_OPTIONS } from '../consts';
import { getCompleteFilterState } from '../utils/filterStateManager';
@@ -337,11 +338,11 @@ const AgGridDataTable: FunctionComponent<AgGridTableProps> = memo(
timestamp: Date.now(),
};
- const stateHash = JSON.stringify({
- columnOrder: columnState.map(c => c.colId),
- sorts: sortModel,
- filters: filterModel,
- });
+ const stateHash = getColumnStateSignature(
+ columnState,
+ sortModel,
+ filterModel,
+ );
if (stateHash !== lastCapturedStateRef.current) {
lastCapturedStateRef.current = stateHash;
diff --git a/superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/getColumnStateSignature.ts b/superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/getColumnStateSignature.ts
new file mode 100644
index 00000000000..074500f17de
--- /dev/null
+++ b/superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/getColumnStateSignature.ts
@@ -0,0 +1,53 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import type { ColumnState } from '@superset-ui/core/components/ThemedAgGridReact';
+
+type SortModelEntry = {
+ colId: string;
+ sort: 'asc' | 'desc';
+ sortIndex: number;
+};
+
+/**
+ * Builds a stable signature of the parts of AG Grid column state that must be
+ * persisted (and therefore must trigger an `onColumnStateChange` when they
+ * change): column order, per-column value aggregation, sorts and filters.
+ *
+ * The value aggregation (`aggFunc`) is included so that changing a column's
+ * Value Aggregation is captured and saved. Without it, the change was only
+ * persisted if some *other* state (e.g. a sort) changed at the same time — so
+ * "Update chart"/save reverted the aggregation back to its default.
+ */
+export default function getColumnStateSignature(
+ columnState: ColumnState[],
+ sortModel: SortModelEntry[],
+ filterModel: Record<string, unknown>,
+): string {
+ return JSON.stringify({
+ columnOrder: columnState.map(col => col.colId),
+ aggregations: columnState.map(col => ({
+ colId: col.colId,
+ // Normalize falsy "None" (null/undefined) to an explicit sentinel so it
+ // is preserved and distinguishable from a real aggregation function.
+ aggFunc: col.aggFunc ?? null,
+ })),
+ sorts: sortModel,
+ filters: filterModel,
+ });
+}
diff --git a/superset-frontend/plugins/plugin-chart-ag-grid-table/test/utils/getColumnStateSignature.test.ts b/superset-frontend/plugins/plugin-chart-ag-grid-table/test/utils/getColumnStateSignature.test.ts
new file mode 100644
index 00000000000..000927ba506
--- /dev/null
+++ b/superset-frontend/plugins/plugin-chart-ag-grid-table/test/utils/getColumnStateSignature.test.ts
@@ -0,0 +1,61 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import getColumnStateSignature from '../../src/utils/getColumnStateSignature';
+
+const noSorts: any[] = [];
+const noFilters = {};
+
+test('changing only the value aggregation changes the signature (no sort needed)', () => {
+ // Regression test for #97551: in Explore, changing a metric's Value
+ // Aggregation and clicking "Update chart" must persist without first having
+ // to trigger an unrelated sort change. The capture signature must therefore
+ // react to aggFunc on its own.
+ const before = getColumnStateSignature(
+ [{ colId: 'SUM(sales)', aggFunc: 'sum' }] as any,
+ noSorts,
+ noFilters,
+ );
+ const after = getColumnStateSignature(
+ [{ colId: 'SUM(sales)', aggFunc: 'avg' }] as any,
+ noSorts,
+ noFilters,
+ );
+ expect(after).not.toEqual(before);
+});
+
+test('aggregation change is detected even when order/sort/filter are identical', () => {
+ const columnsA = [
+ { colId: 'state', aggFunc: undefined },
+ { colId: 'SUM(sales)', aggFunc: 'sum' },
+ ];
+ const columnsB = [
+ { colId: 'state', aggFunc: undefined },
+ { colId: 'SUM(sales)', aggFunc: 'max' },
+ ];
+ expect(
+ getColumnStateSignature(columnsA as any, noSorts, noFilters),
+ ).not.toEqual(getColumnStateSignature(columnsB as any, noSorts, noFilters));
+});
+
+test('identical aggregation produces a stable signature', () => {
+ const cols = [{ colId: 'SUM(sales)', aggFunc: 'sum' }];
+ expect(getColumnStateSignature(cols as any, noSorts, noFilters)).toEqual(
+ getColumnStateSignature(cols as any, noSorts, noFilters),
+ );
+});