You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "richardfogaca (via GitHub)" <gi...@apache.org> on 2026/04/13 21:22:58 UTC
[PR] fix(dashboard): preserve dynamic group by column order [superset]
richardfogaca opened a new pull request, #39333:
URL: https://github.com/apache/superset/pull/39333
### SUMMARY
This fixes a Table V2 dashboard bug where changing a `Dynamic Group By` control could move the selected dimension column to the end of the AG Grid table.
The query path was already correct. In the clean repro, changing the dashboard control from `city` to `order_date` updated the saved chart customization and the next `chart/data` request sent `groupby=["order_date"]` with `order_date` as the first backend column. The bug happened later in the frontend: AG Grid was always restoring the saved `columnState` with `applyOrder: true`, even after the table schema changed. When the old dimension disappeared and a new one appeared, the stale saved order kept the existing metric columns in place and appended the new dimension to the end.
This change reconciles saved column state with the current leaf columns before restoring it. If the column set is unchanged, the existing saved order is preserved. If the schema changed, stale column ids are dropped and order is no longer forced, so the current query order drives the layout again.
This is different from the reverted heatmap change in #39290. That change moved data-ordering behavior to the frontend, which can affect correctness when limits are applied. This fix does not change SQL, sorting semantics, row limits, or query results. It only stops stale frontend column order from overriding the backend column order after a schema change.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before and after screenshots will be attached below.
### TESTING INSTRUCTIONS
1. Create an AG Grid `Table V2` chart with one grouping dimension and a few metrics.
2. Add the chart to a dashboard.
3. Add a `Dynamic group by` display control scoped to that chart.
4. Select a different dimension in the display control and click `Apply filters`.
5. Confirm the rendered first table column matches the selected dimension instead of being appended to the end.
Automated checks run:
- `npm --prefix superset-frontend run test -- plugins/plugin-chart-ag-grid-table/src/utils/reconcileColumnState.test.ts plugins/plugin-chart-ag-grid-table/test/AgGridTableChart.test.tsx plugins/plugin-chart-ag-grid-table/test/buildQuery.test.ts`
- `pre-commit run --files superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTable/index.tsx superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/reconcileColumnState.ts superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/reconcileColumnState.test.ts`
- `pre-commit run --all-files` was also executed, but this local environment still has repo-wide unrelated failures on `master` (`eslint-docs` requires `yarn`, and repo baseline Python lint hooks reported unrelated files outside this change).
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Required feature flags:
- [x] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dashboard): preserve dynamic group by column order [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #39333:
URL: https://github.com/apache/superset/pull/39333#issuecomment-4239811613
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/39333?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `80.76923%` with `5 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 64.42%. Comparing base ([`de40b58`](https://app.codecov.io/gh/apache/superset/commit/de40b58e1050c823167a8d98da5a49603bf4cfa1?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`bba3cea`](https://app.codecov.io/gh/apache/superset/commit/bba3ceac9149983a9fca449298b9dd38eaeaa1c2?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 5 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/39333?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...ugin-chart-ag-grid-table/src/AgGridTable/index.tsx](https://app.codecov.io/gh/apache/superset/pull/39333?src=pr&el=tree&filepath=superset-frontend%2Fplugins%2Fplugin-chart-ag-grid-table%2Fsrc%2FAgGridTable%2Findex.tsx&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtYWctZ3JpZC10YWJsZS9zcmMvQWdHcmlkVGFibGUvaW5kZXgudHN4) | 0.00% | [3 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/39333?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...rt-ag-grid-table/src/utils/reconcileColumnState.ts](https://app.codecov.io/gh/apache/superset/pull/39333?src=pr&el=tree&filepath=superset-frontend%2Fplugins%2Fplugin-chart-ag-grid-table%2Fsrc%2Futils%2FreconcileColumnState.ts&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtYWctZ3JpZC10YWJsZS9zcmMvdXRpbHMvcmVjb25jaWxlQ29sdW1uU3RhdGUudHM=) | 91.30% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/39333?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
:x: Your project check has failed because the head coverage (99.81%) is below the target coverage (100.00%). You can increase the head coverage or adjust the [target](https://docs.codecov.com/docs/commit-status#target) coverage.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #39333 +/- ##
=======================================
Coverage 64.42% 64.42%
=======================================
Files 2553 2554 +1
Lines 132588 132613 +25
Branches 30758 30769 +11
=======================================
+ Hits 85416 85437 +21
- Misses 45686 45690 +4
Partials 1486 1486
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/39333/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [javascript](https://app.codecov.io/gh/apache/superset/pull/39333/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `66.12% <80.76%> (+<0.01%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/superset/pull/39333?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dashboard): preserve dynamic group by column order [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #39333:
URL: https://github.com/apache/superset/pull/39333#issuecomment-4239726965
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dashboard): preserve dynamic group by column order [superset]
Posted by "richardfogaca (via GitHub)" <gi...@apache.org>.
richardfogaca merged PR #39333:
URL: https://github.com/apache/superset/pull/39333
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dashboard): preserve dynamic group by column order [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #39333:
URL: https://github.com/apache/superset/pull/39333#issuecomment-4240105881
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dashboard): preserve dynamic group by column order [superset]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #39333:
URL: https://github.com/apache/superset/pull/39333#discussion_r3075915479
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/reconcileColumnState.ts:
##########
@@ -0,0 +1,81 @@
+/**
+ * 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 ColDef,
+ type ColumnState,
+} from '@superset-ui/core/components/ThemedAgGridReact';
+
+type ColumnGroupDef = ColDef & {
+ children?: ColumnDefLike[];
+};
+
+type ColumnDefLike = ColDef | ColumnGroupDef;
+
+function hasChildren(colDef: ColumnDefLike): colDef is ColumnGroupDef {
+ return 'children' in colDef;
+}
+
+export interface ReconciledColumnState {
+ applyOrder: boolean;
+ columnState: ColumnState[];
+}
+
+export function getLeafColumnIds(colDefs: ColumnDefLike[]): string[] {
+ return colDefs.flatMap(colDef => {
+ if (
+ hasChildren(colDef) &&
+ Array.isArray(colDef.children) &&
+ colDef.children.length > 0
+ ) {
+ return getLeafColumnIds(colDef.children);
+ }
+
+ return typeof colDef.field === 'string' ? [colDef.field] : [];
+ });
+}
+
+export default function reconcileColumnState(
+ savedColumnState: ColumnState[] | undefined,
+ colDefs: ColumnDefLike[],
+): ReconciledColumnState | null {
+ if (!Array.isArray(savedColumnState) || savedColumnState.length === 0) {
+ return null;
+ }
+
+ const currentColumnIds = getLeafColumnIds(colDefs);
+ const currentColumnIdSet = new Set(currentColumnIds);
+ const filteredColumnState = savedColumnState.filter(
+ column =>
+ typeof column.colId === 'string' && currentColumnIdSet.has(column.colId),
+ );
+
+ if (filteredColumnState.length === 0) {
+ return null;
+ }
+
+ const savedColumnIds = filteredColumnState.map(column => column.colId);
+ const hasSameColumnSet =
+ currentColumnIds.length === savedColumnIds.length &&
+ currentColumnIds.every(columnId => savedColumnIds.includes(columnId));
Review Comment:
`hasSameColumnSet` does an O(n²) membership check via `savedColumnIds.includes(...)` inside `every(...)`. Consider converting `savedColumnIds` to a `Set` and comparing set sizes/membership to keep this O(n) and more robust for large tables.
```suggestion
const savedColumnIdSet = new Set(savedColumnIds);
const hasSameColumnSet =
currentColumnIdSet.size === savedColumnIdSet.size &&
Array.from(currentColumnIdSet).every(columnId =>
savedColumnIdSet.has(columnId),
);
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org