You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "alexandrusoare (via GitHub)" <gi...@apache.org> on 2025/09/30 16:24:43 UTC
[PR] feat(Chart): Save Chart State globally [superset]
alexandrusoare opened a new pull request, #35343:
URL: https://github.com/apache/superset/pull/35343
### SUMMARY
The changes you do in a dashboard for a chart (reorder columns, hide columns, filters etc) should be saved.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] 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] feat(Chart): Save Chart State globally [superset]
Posted by "amaannawab923 (via GitHub)" <gi...@apache.org>.
amaannawab923 commented on code in PR #35343:
URL: https://github.com/apache/superset/pull/35343#discussion_r2459729616
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/buildQuery.ts:
##########
@@ -292,6 +409,18 @@ const buildQuery: BuildQuery<TableChartFormData> = (
}
}
+ // Apply AG Grid filters from export (already in standard filter format)
+ if (
+ isDownloadQuery &&
+ Array.isArray(ownState.filters) &&
+ ownState.filters.length > 0
+ ) {
+ queryObject = {
+ ...queryObject,
+ filters: [...(queryObject.filters || []), ...ownState.filters],
+ };
Review Comment:
How do we handle the complex filters in this case , wouldnt we need extras.where and extras.having for handling multiple condition filter scenario
--
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] feat(Chart): Save Chart State globally [superset]
Posted by "amaannawab923 (via GitHub)" <gi...@apache.org>.
amaannawab923 commented on code in PR #35343:
URL: https://github.com/apache/superset/pull/35343#discussion_r2414316041
##########
superset-frontend/packages/superset-ui-core/src/types/AgGrid.ts:
##########
@@ -0,0 +1,118 @@
+/**
+ * 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.
+ */
+
+// AG Grid filter type enums
+export enum AgGridFilterType {
+ Text = 'text',
+ Number = 'number',
+ Date = 'date',
+ Set = 'set',
+}
+
+export enum AgGridTextFilterOperator {
+ Equals = 'equals',
+ NotEqual = 'notEqual',
+ Contains = 'contains',
+ NotContains = 'notContains',
+ StartsWith = 'startsWith',
+ EndsWith = 'endsWith',
+ Blank = 'blank',
+ NotBlank = 'notBlank',
+}
+
+export enum AgGridNumberFilterOperator {
+ Equals = 'equals',
+ NotEqual = 'notEqual',
+ LessThan = 'lessThan',
+ LessThanOrEqual = 'lessThanOrEqual',
+ GreaterThan = 'greaterThan',
+ GreaterThanOrEqual = 'greaterThanOrEqual',
+ InRange = 'inRange',
+ Blank = 'blank',
+ NotBlank = 'notBlank',
+}
+
+export interface AgGridColumnState {
+ colId: string;
+ width?: number;
+ hide?: boolean;
+ pinned?: 'left' | 'right' | null;
+ sort?: 'asc' | 'desc' | null;
+ sortIndex?: number;
+ aggFunc?: string;
+}
+
+export interface AgGridSortModel {
+ colId: string;
+ sort: 'asc' | 'desc';
+ sortIndex?: number;
+}
+
+interface AgGridTextFilter {
+ filterType: AgGridFilterType.Text;
+ type?: AgGridTextFilterOperator;
+ filter?: string;
+}
+
+interface AgGridNumberFilter {
+ filterType: AgGridFilterType.Number;
+ type?: AgGridNumberFilterOperator;
+ filter?: number;
+ filterTo?: number;
+}
+
+interface AgGridSetFilter {
+ filterType: AgGridFilterType.Set;
+ values?: string[];
+}
+
+interface AgGridDateFilter {
+ filterType: AgGridFilterType.Date;
+ type?: AgGridNumberFilterOperator;
+ dateFrom?: string;
+ dateTo?: string;
+}
+
+type AgGridFilter =
+ | AgGridTextFilter
+ | AgGridNumberFilter
+ | AgGridSetFilter
+ | AgGridDateFilter;
+
+export interface AgGridFilterModel {
+ [colId: string]: AgGridFilter;
+}
+
+export interface AgGridChartState {
+ columnState: AgGridColumnState[];
+ sortModel: AgGridSortModel[];
+ filterModel: AgGridFilterModel;
+ columnOrder?: string[];
+ pageSize?: number;
+ currentPage?: number;
+}
Review Comment:
import type { ColumnState, } from 'ag-grid-community';
export interface AgGridChartState {
columnState: ColumnState[];
sortModel: AgGridSortModel[];
filterModel: AgGridFilterModel;
columnOrder?: string[];
pageSize?: number;
currentPage?: number;
}
Column State from ag grid community
--
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] feat(Chart): Save Chart State globally [superset]
Posted by "amaannawab923 (via GitHub)" <gi...@apache.org>.
amaannawab923 commented on code in PR #35343:
URL: https://github.com/apache/superset/pull/35343#discussion_r2414335281
##########
superset-frontend/src/dashboard/components/gridComponents/Chart/Chart.jsx:
##########
@@ -378,20 +392,139 @@ const Chart = props => {
slice_id: slice.slice_id,
is_cached: isCached,
});
+
+ let ownState = dataMask[props.id]?.ownState || {};
+
+ // For AG Grid tables, convert AG Grid state to backend-compatible format
+ if (slice.viz_type === 'ag-grid-table' && chartStates[props.id]?.state) {
+ const agGridState = chartStates[props.id].state;
+
+ // Convert AG Grid sortModel to backend sortBy format
+ if (agGridState.sortModel && agGridState.sortModel.length > 0) {
+ const sortItem = agGridState.sortModel[0];
+ ownState = {
+ ...ownState,
+ sortBy: [
+ {
+ id: sortItem.colId,
+ key: sortItem.colId,
+ desc: sortItem.sort === 'desc',
+ },
+ ],
+ };
+ }
+
+ // Store column order for backend processing
+ if (agGridState.columnState && agGridState.columnState.length > 0) {
+ ownState = {
+ ...ownState,
+ columnOrder: agGridState.columnState.map(col => col.colId),
+ };
+ }
+
+ if (
+ agGridState.filterModel &&
+ Object.keys(agGridState.filterModel).length > 0
+ ) {
+ const agGridFilters = [];
+
+ Object.keys(agGridState.filterModel).forEach(colId => {
+ const filter = agGridState.filterModel[colId];
+
+ // Text filter
+ if (filter.filterType === 'text' && filter.filter) {
+ const clause = {
+ expressionType: 'SIMPLE',
+ subject: colId,
+ operator:
+ filter.type === 'equals'
+ ? '=='
Review Comment:
Shouldn't we just use a hashmap of mapping of filter type to backend filter
--
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] feat(Chart): Save Chart State globally [superset]
Posted by "alexandrusoare (via GitHub)" <gi...@apache.org>.
alexandrusoare commented on code in PR #35343:
URL: https://github.com/apache/superset/pull/35343#discussion_r2419056051
##########
superset-frontend/src/dashboard/types/chartState.ts:
##########
@@ -0,0 +1,65 @@
+/**
+ * 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.
+ */
+
+export interface AgGridColumnState {
+ colId: string;
+ width?: number;
+ hide?: boolean;
+ pinned?: 'left' | 'right' | null;
+ sort?: 'asc' | 'desc' | null;
+ sortIndex?: number;
+ aggFunc?: string;
+}
+
+export interface AgGridSortModel {
+ colId: string;
+ sort: 'asc' | 'desc';
+ sortIndex?: number;
+}
+
+export interface AgGridFilterModel {
+ [colId: string]: {
+ filterType: string;
+ type?: string;
+ filter?: any;
+ condition1?: any;
+ condition2?: any;
+ operator?: string;
+ };
+}
+
+export interface AgGridChartState {
+ columnState: AgGridColumnState[];
+ sortModel: AgGridSortModel[];
+ filterModel: AgGridFilterModel;
+ columnOrder?: string[];
+ pageSize?: number;
+ currentPage?: number;
+}
+
+export interface ChartState {
+ chartId: number;
+ vizType: string;
+ state: AgGridChartState;
+ lastModified?: number;
+}
Review Comment:
Called it ChartState instead
--
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] feat(Chart): Save Chart State globally [superset]
Posted by "amaannawab923 (via GitHub)" <gi...@apache.org>.
amaannawab923 commented on code in PR #35343:
URL: https://github.com/apache/superset/pull/35343#discussion_r2414312997
##########
superset-frontend/packages/superset-ui-core/src/types/AgGrid.ts:
##########
@@ -0,0 +1,118 @@
+/**
+ * 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.
+ */
+
+// AG Grid filter type enums
+export enum AgGridFilterType {
+ Text = 'text',
+ Number = 'number',
+ Date = 'date',
+ Set = 'set',
+}
+
+export enum AgGridTextFilterOperator {
+ Equals = 'equals',
+ NotEqual = 'notEqual',
+ Contains = 'contains',
+ NotContains = 'notContains',
+ StartsWith = 'startsWith',
+ EndsWith = 'endsWith',
+ Blank = 'blank',
+ NotBlank = 'notBlank',
+}
+
+export enum AgGridNumberFilterOperator {
+ Equals = 'equals',
+ NotEqual = 'notEqual',
+ LessThan = 'lessThan',
+ LessThanOrEqual = 'lessThanOrEqual',
+ GreaterThan = 'greaterThan',
+ GreaterThanOrEqual = 'greaterThanOrEqual',
+ InRange = 'inRange',
+ Blank = 'blank',
+ NotBlank = 'notBlank',
+}
+
+export interface AgGridColumnState {
+ colId: string;
+ width?: number;
+ hide?: boolean;
+ pinned?: 'left' | 'right' | null;
+ sort?: 'asc' | 'desc' | null;
+ sortIndex?: number;
+ aggFunc?: string;
+}
+
+export interface AgGridSortModel {
+ colId: string;
+ sort: 'asc' | 'desc';
+ sortIndex?: number;
+}
Review Comment:
import type { SortModelItem } from 'ag-grid-community';
// Extend SortModelItem to add sortIndex for multi-column sorting
export interface AgGridSortModel extends SortModelItem {
sortIndex?: number;
} ... Can extend existing interface from Ag Grid
--
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] feat(Chart): Save Chart State globally [superset]
Posted by "alexandrusoare (via GitHub)" <gi...@apache.org>.
alexandrusoare commented on code in PR #35343:
URL: https://github.com/apache/superset/pull/35343#discussion_r2419048172
##########
superset-frontend/packages/superset-ui-core/src/types/AgGrid.ts:
##########
@@ -0,0 +1,118 @@
+/**
+ * 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.
+ */
+
+// AG Grid filter type enums
+export enum AgGridFilterType {
+ Text = 'text',
+ Number = 'number',
+ Date = 'date',
+ Set = 'set',
+}
+
+export enum AgGridTextFilterOperator {
+ Equals = 'equals',
+ NotEqual = 'notEqual',
+ Contains = 'contains',
+ NotContains = 'notContains',
+ StartsWith = 'startsWith',
+ EndsWith = 'endsWith',
+ Blank = 'blank',
+ NotBlank = 'notBlank',
+}
+
+export enum AgGridNumberFilterOperator {
+ Equals = 'equals',
+ NotEqual = 'notEqual',
+ LessThan = 'lessThan',
+ LessThanOrEqual = 'lessThanOrEqual',
+ GreaterThan = 'greaterThan',
+ GreaterThanOrEqual = 'greaterThanOrEqual',
+ InRange = 'inRange',
+ Blank = 'blank',
+ NotBlank = 'notBlank',
+}
+
+export interface AgGridColumnState {
+ colId: string;
+ width?: number;
+ hide?: boolean;
+ pinned?: 'left' | 'right' | null;
+ sort?: 'asc' | 'desc' | null;
+ sortIndex?: number;
+ aggFunc?: string;
+}
+
+export interface AgGridSortModel {
+ colId: string;
+ sort: 'asc' | 'desc';
+ sortIndex?: number;
+}
+
+interface AgGridTextFilter {
+ filterType: AgGridFilterType.Text;
+ type?: AgGridTextFilterOperator;
+ filter?: string;
+}
+
+interface AgGridNumberFilter {
+ filterType: AgGridFilterType.Number;
+ type?: AgGridNumberFilterOperator;
+ filter?: number;
+ filterTo?: number;
+}
+
+interface AgGridSetFilter {
+ filterType: AgGridFilterType.Set;
+ values?: string[];
+}
+
+interface AgGridDateFilter {
+ filterType: AgGridFilterType.Date;
+ type?: AgGridNumberFilterOperator;
+ dateFrom?: string;
+ dateTo?: string;
+}
+
+type AgGridFilter =
+ | AgGridTextFilter
+ | AgGridNumberFilter
+ | AgGridSetFilter
+ | AgGridDateFilter;
+
+export interface AgGridFilterModel {
+ [colId: string]: AgGridFilter;
+}
+
+export interface AgGridChartState {
+ columnState: AgGridColumnState[];
+ sortModel: AgGridSortModel[];
+ filterModel: AgGridFilterModel;
+ columnOrder?: string[];
+ pageSize?: number;
+ currentPage?: number;
+}
Review Comment:
Good idea, thank you
--
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] feat(Chart): Save Chart State globally [superset]
Posted by "amaannawab923 (via GitHub)" <gi...@apache.org>.
amaannawab923 commented on code in PR #35343:
URL: https://github.com/apache/superset/pull/35343#discussion_r2414338610
##########
superset-frontend/src/dashboard/components/gridComponents/Chart/Chart.jsx:
##########
@@ -378,20 +392,139 @@ const Chart = props => {
slice_id: slice.slice_id,
is_cached: isCached,
});
+
+ let ownState = dataMask[props.id]?.ownState || {};
+
+ // For AG Grid tables, convert AG Grid state to backend-compatible format
+ if (slice.viz_type === 'ag-grid-table' && chartStates[props.id]?.state) {
+ const agGridState = chartStates[props.id].state;
+
+ // Convert AG Grid sortModel to backend sortBy format
+ if (agGridState.sortModel && agGridState.sortModel.length > 0) {
+ const sortItem = agGridState.sortModel[0];
+ ownState = {
+ ...ownState,
+ sortBy: [
+ {
+ id: sortItem.colId,
+ key: sortItem.colId,
+ desc: sortItem.sort === 'desc',
+ },
+ ],
+ };
+ }
+
+ // Store column order for backend processing
+ if (agGridState.columnState && agGridState.columnState.length > 0) {
+ ownState = {
+ ...ownState,
+ columnOrder: agGridState.columnState.map(col => col.colId),
+ };
+ }
+
+ if (
+ agGridState.filterModel &&
+ Object.keys(agGridState.filterModel).length > 0
+ ) {
+ const agGridFilters = [];
+
+ Object.keys(agGridState.filterModel).forEach(colId => {
+ const filter = agGridState.filterModel[colId];
+
+ // Text filter
+ if (filter.filterType === 'text' && filter.filter) {
+ const clause = {
+ expressionType: 'SIMPLE',
+ subject: colId,
+ operator:
+ filter.type === 'equals'
+ ? '=='
+ : filter.type === 'notEqual'
+ ? '!='
+ : filter.type === 'contains'
+ ? 'ILIKE'
+ : filter.type === 'notContains'
+ ? 'NOT ILIKE'
+ : filter.type === 'startsWith'
+ ? 'ILIKE'
+ : filter.type === 'endsWith'
+ ? 'ILIKE'
+ : 'ILIKE',
+ comparator:
+ filter.type === 'contains' || filter.type === 'notContains'
+ ? `%${filter.filter}%`
+ : filter.type === 'startsWith'
+ ? `${filter.filter}%`
+ : filter.type === 'endsWith'
+ ? `%${filter.filter}`
+ : filter.filter,
+ };
+ agGridFilters.push(clause);
+ } else if (
+ filter.filterType === 'number' &&
+ filter.filter !== undefined
+ ) {
+ const clause = {
+ expressionType: 'SIMPLE',
+ subject: colId,
+ operator:
+ filter.type === 'equals'
+ ? '=='
+ : filter.type === 'notEqual'
+ ? '!='
+ : filter.type === 'lessThan'
+ ? '<'
+ : filter.type === 'lessThanOrEqual'
+ ? '<='
+ : filter.type === 'greaterThan'
+ ? '>'
+ : filter.type === 'greaterThanOrEqual'
+ ? '>='
+ : '==',
+ comparator: filter.filter,
+ };
+ agGridFilters.push(clause);
+ } else if (
+ filter.filterType === 'set' &&
+ Array.isArray(filter.values) &&
+ filter.values.length > 0
+ ) {
+ const clause = {
+ expressionType: 'SIMPLE',
+ subject: colId,
+ operator: 'IN',
+ comparator: filter.values,
+ };
+ agGridFilters.push(clause);
Review Comment:
Why are we creating a simple clause expression, extra form data.filters should be fine as we handled the search query
--
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] feat(Chart): Save Chart State globally [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #35343:
URL: https://github.com/apache/superset/pull/35343#issuecomment-3461109806
πͺ [Showtime](https://github.com/mistercrunch/superset-showtime) deployed environment on [GHA](https://github.com/apache/superset/actions/runs/18906481234) for [bc1a717](https://github.com/apache/superset/commit/bc1a717)
β’ **Environment:** http://35.166.24.243:8080 (admin/admin)
β’ **Lifetime:** 48h auto-cleanup
β’ **Updates:** New commits create fresh environments automatically
--
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] feat(Chart): Save Chart State globally [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #35343:
URL: https://github.com/apache/superset/pull/35343#issuecomment-3426446376
πͺ [Showtime](https://github.com/mistercrunch/superset-showtime) is building environment on [GHA](https://github.com/apache/superset/actions/runs/18684337579) for [7d7dd87](https://github.com/apache/superset/commit/7d7dd87)
--
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] feat(Chart): Save Chart State globally [superset]
Posted by "alexandrusoare (via GitHub)" <gi...@apache.org>.
alexandrusoare commented on PR #35343:
URL: https://github.com/apache/superset/pull/35343#issuecomment-3461510839
> Also, it would be great to add a video!
I am not able to add a video as there a current limit of 10MB of video uploads, I uploaded some screenshots
--
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] feat(Chart): Save Chart State globally [superset]
Posted by "geido (via GitHub)" <gi...@apache.org>.
geido commented on code in PR #35343:
URL: https://github.com/apache/superset/pull/35343#discussion_r2472629174
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTable/index.tsx:
##########
@@ -234,6 +287,55 @@ const AgGridDataTable: FunctionComponent<AgGridTableProps> = memo(
[serverPagination, gridInitialState, percentMetrics, onSortChange],
);
+ const handleGridStateChange = useCallback(
+ debounce(() => {
+ if (onColumnStateChange && gridRef.current?.api) {
+ try {
+ const { api } = gridRef.current;
+
+ const columnState = api.getColumnState ? api.getColumnState() : [];
+
+ const filterModel = api.getFilterModel ? api.getFilterModel() : {};
+
+ const sortModel = columnState
+ .filter(col => col.sort)
+ .map(col => ({
+ colId: col.colId,
+ sort: col.sort as 'asc' | 'desc',
+ sortIndex: col.sortIndex || 0,
+ }))
+ .sort((a, b) => (a.sortIndex || 0) - (b.sortIndex || 0));
+
+ const stateToSave = {
+ columnState,
+ sortModel,
+ filterModel,
+ timestamp: Date.now(),
+ };
+
+ const stateHash = JSON.stringify({
+ columnOrder: columnState.map(c => c.colId),
+ sorts: sortModel,
+ filters: filterModel,
+ });
+
+ if (stateHash !== lastCapturedStateRef.current) {
+ lastCapturedStateRef.current = stateHash;
+
+ onColumnStateChange(stateToSave);
+ }
+ } catch (error) {
+ console.warn('Error capturing AG Grid state:', error);
+ onColumnStateChange({
+ timestamp: Date.now(),
+ hasChanges: true,
Review Comment:
What resets this eventually?
--
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] feat(Chart): Save Chart State globally [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #35343:
URL: https://github.com/apache/superset/pull/35343#issuecomment-3461058465
πͺ [Showtime](https://github.com/mistercrunch/superset-showtime) is building environment on [GHA](https://github.com/apache/superset/actions/runs/18906481234) for [bc1a717](https://github.com/apache/superset/commit/bc1a717)
--
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] feat(Chart): Save Chart State globally [superset]
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on code in PR #35343:
URL: https://github.com/apache/superset/pull/35343#discussion_r2465307686
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/buildQuery.ts:
##########
@@ -211,22 +213,137 @@ const buildQuery: BuildQuery<TableChartFormData> = (
moreProps.row_offset = currentPage * pageSize;
}
- // getting sort by in case of server pagination from own state
let sortByFromOwnState: QueryFormOrderBy[] | undefined;
- if (Array.isArray(ownState?.sortBy) && ownState?.sortBy.length > 0) {
- const sortByItem = ownState?.sortBy[0];
- sortByFromOwnState = [[sortByItem?.key, !sortByItem?.desc]];
+
+ const sortSource =
+ isDownloadQuery && ownState?.sortModel
+ ? ownState.sortModel
+ : ownState?.sortBy;
+
+ if (Array.isArray(sortSource) && sortSource.length > 0) {
+ const mapColIdToIdentifier = (colId: string): string | undefined => {
+ const matchingColumn = columns.find((col: QueryFormColumn) => {
+ if (typeof col === 'string') return col === colId;
+ return col?.sqlExpression === colId || col?.label === colId;
+ });
+
+ if (matchingColumn) {
+ return typeof matchingColumn === 'string'
+ ? matchingColumn
+ : matchingColumn.sqlExpression || matchingColumn.label;
+ }
+
+ const matchingMetric = (metrics || []).find((met: QueryFormMetric) => {
+ if (typeof met === 'string')
+ return met === colId || `%${met}` === colId;
+ const metLabel = getMetricLabel(met);
+ return (
+ metLabel === colId ||
+ met?.label === colId ||
+ `%${metLabel}` === colId
+ );
+ });
+
+ if (matchingMetric) {
+ return typeof matchingMetric === 'string'
+ ? matchingMetric
+ : getMetricLabel(matchingMetric);
Review Comment:
just getMetricLabel is enough - see above
--
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] feat(Chart): Save Chart State globally [superset]
Posted by "amaannawab923 (via GitHub)" <gi...@apache.org>.
amaannawab923 commented on code in PR #35343:
URL: https://github.com/apache/superset/pull/35343#discussion_r2428267124
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/buildQuery.ts:
##########
@@ -218,15 +218,79 @@ const buildQuery: BuildQuery<TableChartFormData> = (
sortByFromOwnState = [[sortByItem?.key, !sortByItem?.desc]];
}
+ // Note: In Superset, "columns" are dimensions and "metrics" are measures,
+ // but AG Grid treats them all as "columns" in the UI
+ let orderedColumns = columns;
+ let orderedMetrics = metrics;
Review Comment:
The build query operations are good amount of logical code , Can we possibly add some Unit tests for this piece of code
--
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] feat(Chart): Save Chart State globally [superset]
Posted by "alexandrusoare (via GitHub)" <gi...@apache.org>.
alexandrusoare commented on code in PR #35343:
URL: https://github.com/apache/superset/pull/35343#discussion_r2417703971
##########
superset-frontend/src/dashboard/components/gridComponents/Chart/Chart.jsx:
##########
@@ -378,20 +392,139 @@ const Chart = props => {
slice_id: slice.slice_id,
is_cached: isCached,
});
+
+ let ownState = dataMask[props.id]?.ownState || {};
+
+ // For AG Grid tables, convert AG Grid state to backend-compatible format
+ if (slice.viz_type === 'ag-grid-table' && chartStates[props.id]?.state) {
+ const agGridState = chartStates[props.id].state;
+
+ // Convert AG Grid sortModel to backend sortBy format
+ if (agGridState.sortModel && agGridState.sortModel.length > 0) {
+ const sortItem = agGridState.sortModel[0];
+ ownState = {
+ ...ownState,
+ sortBy: [
+ {
+ id: sortItem.colId,
+ key: sortItem.colId,
+ desc: sortItem.sort === 'desc',
+ },
+ ],
+ };
+ }
+
+ // Store column order for backend processing
+ if (agGridState.columnState && agGridState.columnState.length > 0) {
+ ownState = {
+ ...ownState,
+ columnOrder: agGridState.columnState.map(col => col.colId),
+ };
+ }
+
+ if (
+ agGridState.filterModel &&
+ Object.keys(agGridState.filterModel).length > 0
+ ) {
+ const agGridFilters = [];
+
+ Object.keys(agGridState.filterModel).forEach(colId => {
+ const filter = agGridState.filterModel[colId];
+
+ // Text filter
+ if (filter.filterType === 'text' && filter.filter) {
+ const clause = {
+ expressionType: 'SIMPLE',
+ subject: colId,
+ operator:
+ filter.type === 'equals'
+ ? '=='
+ : filter.type === 'notEqual'
+ ? '!='
+ : filter.type === 'contains'
+ ? 'ILIKE'
+ : filter.type === 'notContains'
+ ? 'NOT ILIKE'
+ : filter.type === 'startsWith'
+ ? 'ILIKE'
+ : filter.type === 'endsWith'
+ ? 'ILIKE'
+ : 'ILIKE',
+ comparator:
+ filter.type === 'contains' || filter.type === 'notContains'
+ ? `%${filter.filter}%`
+ : filter.type === 'startsWith'
+ ? `${filter.filter}%`
+ : filter.type === 'endsWith'
+ ? `%${filter.filter}`
+ : filter.filter,
+ };
+ agGridFilters.push(clause);
+ } else if (
+ filter.filterType === 'number' &&
+ filter.filter !== undefined
+ ) {
+ const clause = {
+ expressionType: 'SIMPLE',
+ subject: colId,
+ operator:
+ filter.type === 'equals'
+ ? '=='
+ : filter.type === 'notEqual'
+ ? '!='
+ : filter.type === 'lessThan'
+ ? '<'
+ : filter.type === 'lessThanOrEqual'
+ ? '<='
+ : filter.type === 'greaterThan'
+ ? '>'
+ : filter.type === 'greaterThanOrEqual'
+ ? '>='
+ : '==',
+ comparator: filter.filter,
+ };
+ agGridFilters.push(clause);
+ } else if (
+ filter.filterType === 'set' &&
+ Array.isArray(filter.values) &&
+ filter.values.length > 0
+ ) {
+ const clause = {
+ expressionType: 'SIMPLE',
+ subject: colId,
+ operator: 'IN',
+ comparator: filter.values,
+ };
+ agGridFilters.push(clause);
+ }
+ });
+
+ if (agGridFilters.length > 0) {
+ ownState = {
+ ...ownState,
+ agGridFilters,
+ };
+ }
+ }
+ }
+
exportChart({
formData: isFullCSV ? { ...formData, row_limit: maxRows } : formData,
resultType: isPivot ? 'post_processed' : 'full',
resultFormat: format,
force: true,
- ownState: dataMask[props.id]?.ownState,
Review Comment:
Yes we declare it a couple of lines higher and combine the dataMask[props.id]?.ownState with agGrid state
--
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] feat(Chart): Save Chart State globally [superset]
Posted by "alexandrusoare (via GitHub)" <gi...@apache.org>.
alexandrusoare commented on code in PR #35343:
URL: https://github.com/apache/superset/pull/35343#discussion_r2419047657
##########
superset-frontend/packages/superset-ui-core/src/types/AgGrid.ts:
##########
@@ -0,0 +1,118 @@
+/**
+ * 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.
+ */
+
+// AG Grid filter type enums
+export enum AgGridFilterType {
+ Text = 'text',
+ Number = 'number',
+ Date = 'date',
+ Set = 'set',
+}
+
+export enum AgGridTextFilterOperator {
+ Equals = 'equals',
+ NotEqual = 'notEqual',
+ Contains = 'contains',
+ NotContains = 'notContains',
+ StartsWith = 'startsWith',
+ EndsWith = 'endsWith',
+ Blank = 'blank',
+ NotBlank = 'notBlank',
+}
+
+export enum AgGridNumberFilterOperator {
+ Equals = 'equals',
+ NotEqual = 'notEqual',
+ LessThan = 'lessThan',
+ LessThanOrEqual = 'lessThanOrEqual',
+ GreaterThan = 'greaterThan',
+ GreaterThanOrEqual = 'greaterThanOrEqual',
+ InRange = 'inRange',
+ Blank = 'blank',
+ NotBlank = 'notBlank',
+}
+
+export interface AgGridColumnState {
+ colId: string;
+ width?: number;
+ hide?: boolean;
+ pinned?: 'left' | 'right' | null;
+ sort?: 'asc' | 'desc' | null;
+ sortIndex?: number;
+ aggFunc?: string;
+}
+
+export interface AgGridSortModel {
+ colId: string;
+ sort: 'asc' | 'desc';
+ sortIndex?: number;
+}
Review Comment:
Looked into that, thanks
--
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] feat(Chart): Save Chart State globally [superset]
Posted by "alexandrusoare (via GitHub)" <gi...@apache.org>.
alexandrusoare commented on code in PR #35343:
URL: https://github.com/apache/superset/pull/35343#discussion_r2419043832
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/buildQuery.ts:
##########
@@ -218,15 +218,79 @@ const buildQuery: BuildQuery<TableChartFormData> = (
sortByFromOwnState = [[sortByItem?.key, !sortByItem?.desc]];
}
+ // Note: In Superset, "columns" are dimensions and "metrics" are measures,
+ // but AG Grid treats them all as "columns" in the UI
+ let orderedColumns = columns;
+ let orderedMetrics = metrics;
+
+ if (
+ isDownloadQuery &&
+ ownState.columnOrder &&
+ Array.isArray(ownState.columnOrder)
+ ) {
+ const orderedCols: typeof columns = [];
+ const orderedMets: typeof metrics = [];
+
+ ownState.columnOrder.forEach((colId: string) => {
+ const matchingCol = columns.find(col => {
+ if (typeof col === 'string') {
+ return col === colId;
+ }
+ return col?.sqlExpression === colId || col?.label === colId;
+ });
+
+ if (matchingCol && !orderedCols.includes(matchingCol)) {
+ orderedCols.push(matchingCol);
+ return;
+ }
+
+ const matchingMetric = metrics?.find(met => {
+ if (typeof met === 'string') {
+ return met === colId;
+ }
+ return getMetricLabel(met) === colId || met?.label === colId;
+ });
+
+ if (matchingMetric && !orderedMets.includes(matchingMetric)) {
+ orderedMets.push(matchingMetric);
+ }
+ });
+
+ columns.forEach(col => {
+ if (!orderedCols.includes(col)) {
+ orderedCols.push(col);
+ }
+ });
+
+ metrics?.forEach(met => {
+ if (!orderedMets.includes(met)) {
+ orderedMets.push(met);
+ }
+ });
+
+ orderedColumns = orderedCols;
+ orderedMetrics = orderedMets;
+ }
Review Comment:
Took care of, thanks
--
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] feat(Chart): Save Chart State globally [superset]
Posted by "msyavuz (via GitHub)" <gi...@apache.org>.
msyavuz commented on code in PR #35343:
URL: https://github.com/apache/superset/pull/35343#discussion_r2413667129
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTable/index.tsx:
##########
@@ -257,6 +318,24 @@ const AgGridDataTable: FunctionComponent<AgGridTableProps> = memo(
const onGridReady = (params: GridReadyEvent) => {
// This will make columns fill the grid width
params.api.sizeColumnsToFit();
+
+ // Restore saved AG Grid state from permalink if available
+ if (savedAgGridState && params.api) {
+ try {
+ if (savedAgGridState.columnState) {
+ params.api.applyColumnState?.({
+ state: savedAgGridState.columnState as any,
+ applyOrder: true,
+ });
+ }
+
+ if (savedAgGridState.filterModel) {
+ params.api.setFilterModel?.(savedAgGridState.filterModel);
+ }
+ } catch (error) {
+ console.warn('Error restoring AG Grid state:', error);
Review Comment:
Should user be notified of this?
##########
superset-frontend/src/dashboard/components/menu/ShareMenuItems/index.tsx:
##########
@@ -58,20 +58,39 @@ export const useShareMenuItems = (props: ShareMenuItemProps): MenuItem => {
disabled,
...rest
} = props;
- const { dataMask, activeTabs } = useSelector(
+ const { dataMask, activeTabs, chartStates, sliceEntities } = useSelector(
(state: RootState) => ({
dataMask: state.dataMask,
activeTabs: state.dashboardState.activeTabs,
+ chartStates: state.dashboardState.chartStates,
+ sliceEntities: state.sliceEntities,
}),
shallowEqual,
);
async function generateUrl() {
+ // Check if dashboard has AG Grid tables
+ const hasAgGridTables =
+ sliceEntities &&
+ Object.values(sliceEntities).some(
+ slice =>
+ slice &&
+ typeof slice === 'object' &&
+ 'viz_type' in slice &&
+ slice.viz_type === 'ag_grid_table',
+ );
+
Review Comment:
That was used before, can we extract it out to a seperate function?
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/buildQuery.ts:
##########
@@ -218,15 +218,79 @@ const buildQuery: BuildQuery<TableChartFormData> = (
sortByFromOwnState = [[sortByItem?.key, !sortByItem?.desc]];
}
+ // Note: In Superset, "columns" are dimensions and "metrics" are measures,
+ // but AG Grid treats them all as "columns" in the UI
+ let orderedColumns = columns;
+ let orderedMetrics = metrics;
+
+ if (
+ isDownloadQuery &&
+ ownState.columnOrder &&
+ Array.isArray(ownState.columnOrder)
+ ) {
+ const orderedCols: typeof columns = [];
+ const orderedMets: typeof metrics = [];
+
+ ownState.columnOrder.forEach((colId: string) => {
+ const matchingCol = columns.find(col => {
+ if (typeof col === 'string') {
+ return col === colId;
+ }
+ return col?.sqlExpression === colId || col?.label === colId;
+ });
+
+ if (matchingCol && !orderedCols.includes(matchingCol)) {
+ orderedCols.push(matchingCol);
+ return;
+ }
+
+ const matchingMetric = metrics?.find(met => {
+ if (typeof met === 'string') {
+ return met === colId;
+ }
+ return getMetricLabel(met) === colId || met?.label === colId;
+ });
+
+ if (matchingMetric && !orderedMets.includes(matchingMetric)) {
+ orderedMets.push(matchingMetric);
+ }
+ });
+
+ columns.forEach(col => {
+ if (!orderedCols.includes(col)) {
+ orderedCols.push(col);
+ }
+ });
+
+ metrics?.forEach(met => {
+ if (!orderedMets.includes(met)) {
+ orderedMets.push(met);
+ }
+ });
+
+ orderedColumns = orderedCols;
+ orderedMetrics = orderedMets;
+ }
Review Comment:
It looks like there might be some improvements that we can do here. Maybe using maps and drying up a bit could identify some improvements?
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTable/index.tsx:
##########
@@ -313,7 +392,14 @@ const AgGridDataTable: FunctionComponent<AgGridTableProps> = memo(
rowSelection="multiple"
animateRows
onCellClicked={handleCrossFilter}
+ onDragStopped={handleGridStateChange}
+ onColumnResized={handleGridStateChange}
+ onSortChanged={handleGridStateChange}
+ onFilterChanged={handleGridStateChange}
+ onColumnVisible={handleGridStateChange}
+ onColumnPinned={handleGridStateChange}
Review Comment:
Is there no general onChange prop for this?
##########
superset-frontend/src/embedded/api.tsx:
##########
@@ -46,28 +48,49 @@ const getDashboardPermalink = async ({
anchor: string;
}): Promise<string> => {
const state = store?.getState();
- const { dashboardId, dataMask, activeTabs } = {
+ const { dashboardId, dataMask, activeTabs, chartStates, sliceEntities } = {
dashboardId:
state?.dashboardInfo?.id || bootstrapData?.embedded!.dashboard_id,
dataMask: state?.dataMask,
activeTabs: state.dashboardState?.activeTabs,
+ chartStates: state.dashboardState?.chartStates,
+ sliceEntities: state?.sliceEntities,
};
+ const hasAgGridTables =
+ sliceEntities &&
+ Object.values(sliceEntities).some(
+ slice =>
+ slice &&
+ typeof slice === 'object' &&
+ 'viz_type' in slice &&
+ slice.viz_type === 'ag_grid_table',
+ );
Review Comment:
Yep, this is the third usage so we should definitely extract this out
##########
superset-frontend/src/dashboard/components/gridComponents/Chart/Chart.jsx:
##########
@@ -378,20 +392,139 @@ const Chart = props => {
slice_id: slice.slice_id,
is_cached: isCached,
});
+
+ let ownState = dataMask[props.id]?.ownState || {};
+
+ // For AG Grid tables, convert AG Grid state to backend-compatible format
+ if (slice.viz_type === 'ag-grid-table' && chartStates[props.id]?.state) {
+ const agGridState = chartStates[props.id].state;
+
+ // Convert AG Grid sortModel to backend sortBy format
+ if (agGridState.sortModel && agGridState.sortModel.length > 0) {
+ const sortItem = agGridState.sortModel[0];
+ ownState = {
+ ...ownState,
+ sortBy: [
+ {
+ id: sortItem.colId,
+ key: sortItem.colId,
+ desc: sortItem.sort === 'desc',
+ },
+ ],
+ };
+ }
+
+ // Store column order for backend processing
+ if (agGridState.columnState && agGridState.columnState.length > 0) {
+ ownState = {
+ ...ownState,
+ columnOrder: agGridState.columnState.map(col => col.colId),
+ };
+ }
+
+ if (
+ agGridState.filterModel &&
+ Object.keys(agGridState.filterModel).length > 0
+ ) {
+ const agGridFilters = [];
+
+ Object.keys(agGridState.filterModel).forEach(colId => {
+ const filter = agGridState.filterModel[colId];
+
+ // Text filter
+ if (filter.filterType === 'text' && filter.filter) {
+ const clause = {
+ expressionType: 'SIMPLE',
+ subject: colId,
+ operator:
+ filter.type === 'equals'
+ ? '=='
+ : filter.type === 'notEqual'
+ ? '!='
+ : filter.type === 'contains'
+ ? 'ILIKE'
+ : filter.type === 'notContains'
+ ? 'NOT ILIKE'
+ : filter.type === 'startsWith'
+ ? 'ILIKE'
+ : filter.type === 'endsWith'
+ ? 'ILIKE'
+ : 'ILIKE',
+ comparator:
+ filter.type === 'contains' || filter.type === 'notContains'
+ ? `%${filter.filter}%`
+ : filter.type === 'startsWith'
+ ? `${filter.filter}%`
+ : filter.type === 'endsWith'
+ ? `%${filter.filter}`
+ : filter.filter,
+ };
+ agGridFilters.push(clause);
+ } else if (
+ filter.filterType === 'number' &&
+ filter.filter !== undefined
+ ) {
+ const clause = {
+ expressionType: 'SIMPLE',
+ subject: colId,
+ operator:
+ filter.type === 'equals'
+ ? '=='
+ : filter.type === 'notEqual'
+ ? '!='
+ : filter.type === 'lessThan'
+ ? '<'
+ : filter.type === 'lessThanOrEqual'
+ ? '<='
+ : filter.type === 'greaterThan'
+ ? '>'
+ : filter.type === 'greaterThanOrEqual'
+ ? '>='
+ : '==',
+ comparator: filter.filter,
+ };
+ agGridFilters.push(clause);
+ } else if (
+ filter.filterType === 'set' &&
+ Array.isArray(filter.values) &&
+ filter.values.length > 0
+ ) {
+ const clause = {
+ expressionType: 'SIMPLE',
+ subject: colId,
+ operator: 'IN',
+ comparator: filter.values,
+ };
+ agGridFilters.push(clause);
+ }
+ });
+
+ if (agGridFilters.length > 0) {
+ ownState = {
+ ...ownState,
+ agGridFilters,
+ };
+ }
+ }
+ }
Review Comment:
Should this be on the backend instead?
##########
superset-frontend/src/dashboard/types/chartState.ts:
##########
@@ -0,0 +1,65 @@
+/**
+ * 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.
+ */
+
+export interface AgGridColumnState {
+ colId: string;
+ width?: number;
+ hide?: boolean;
+ pinned?: 'left' | 'right' | null;
+ sort?: 'asc' | 'desc' | null;
+ sortIndex?: number;
+ aggFunc?: string;
+}
+
+export interface AgGridSortModel {
+ colId: string;
+ sort: 'asc' | 'desc';
+ sortIndex?: number;
+}
+
+export interface AgGridFilterModel {
+ [colId: string]: {
+ filterType: string;
+ type?: string;
+ filter?: any;
+ condition1?: any;
+ condition2?: any;
+ operator?: string;
+ };
+}
+
+export interface AgGridChartState {
+ columnState: AgGridColumnState[];
+ sortModel: AgGridSortModel[];
+ filterModel: AgGridFilterModel;
+ columnOrder?: string[];
+ pageSize?: number;
+ currentPage?: number;
+}
+
+export interface ChartState {
+ chartId: number;
+ vizType: string;
+ state: AgGridChartState;
+ lastModified?: number;
+}
Review Comment:
Agree with the bot on this one. Maybe we should call `state` type some generic interface instead of `AgGridChartState`?
--
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] feat(Chart): Save Chart State globally [superset]
Posted by "geido (via GitHub)" <gi...@apache.org>.
geido commented on code in PR #35343:
URL: https://github.com/apache/superset/pull/35343#discussion_r2448134592
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/buildQuery.ts:
##########
@@ -211,22 +213,139 @@ const buildQuery: BuildQuery<TableChartFormData> = (
moreProps.row_offset = currentPage * pageSize;
}
- // getting sort by in case of server pagination from own state
let sortByFromOwnState: QueryFormOrderBy[] | undefined;
- if (Array.isArray(ownState?.sortBy) && ownState?.sortBy.length > 0) {
- const sortByItem = ownState?.sortBy[0];
- sortByFromOwnState = [[sortByItem?.key, !sortByItem?.desc]];
+
+ const sortSource =
+ isDownloadQuery && ownState?.sortModel
+ ? ownState.sortModel
+ : ownState?.sortBy;
+
+ if (Array.isArray(sortSource) && sortSource.length > 0) {
+ const mapColIdToIdentifier = (colId: string): string | undefined => {
+ const matchingColumn = columns.find((col: any) => {
+ if (typeof col === 'string') return col === colId;
+ return col?.sqlExpression === colId || col?.label === colId;
+ });
+
+ if (matchingColumn) {
+ return typeof matchingColumn === 'string'
+ ? matchingColumn
+ : matchingColumn.sqlExpression || matchingColumn.label;
+ }
+
+ const matchingMetric = (metrics || []).find((met: any) => {
Review Comment:
Same here, can we have a better type than `any`?
##########
superset-frontend/src/dashboard/components/gridComponents/Chart/Chart.jsx:
##########
@@ -200,6 +201,16 @@ const Chart = props => {
[boundActionCreators.logEvent, boundActionCreators.changeFilter, chart.id],
);
+ // Chart state handler for AG Grid tables
+ const handleChartStateChange = useCallback(
+ chartState => {
+ if (slice?.viz_type === 'ag-grid-table') {
Review Comment:
Can we make this a bit more abstract? Maybe check if the viz supports this type of operation instead of relying on checking the viz type as a string? I know that we have a `BEHAVIORS` property, maybe we can use that or implement something on those lines.
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/buildQuery.ts:
##########
@@ -211,22 +213,139 @@ const buildQuery: BuildQuery<TableChartFormData> = (
moreProps.row_offset = currentPage * pageSize;
}
- // getting sort by in case of server pagination from own state
let sortByFromOwnState: QueryFormOrderBy[] | undefined;
- if (Array.isArray(ownState?.sortBy) && ownState?.sortBy.length > 0) {
- const sortByItem = ownState?.sortBy[0];
- sortByFromOwnState = [[sortByItem?.key, !sortByItem?.desc]];
+
+ const sortSource =
+ isDownloadQuery && ownState?.sortModel
+ ? ownState.sortModel
+ : ownState?.sortBy;
+
+ if (Array.isArray(sortSource) && sortSource.length > 0) {
+ const mapColIdToIdentifier = (colId: string): string | undefined => {
+ const matchingColumn = columns.find((col: any) => {
Review Comment:
Do we have a better representation than `any` for this?
##########
superset-frontend/src/components/Chart/Chart.tsx:
##########
@@ -80,6 +80,7 @@ export interface ChartProps {
datasetsStatus?: 'loading' | 'error' | 'complete';
isInView?: boolean;
emitCrossFilters?: boolean;
+ onChartStateChange?: (chartState: JsonObject) => void;
Review Comment:
Do we have a better type for chartState that we can use?
##########
superset-frontend/src/dashboard/components/gridComponents/Chart/Chart.jsx:
##########
@@ -383,20 +397,132 @@ const Chart = props => {
slice_id: slice.slice_id,
is_cached: isCached,
});
+
+ let ownState = dataMask[props.id]?.ownState || {};
+
+ // For AG Grid tables, convert AG Grid state to backend-compatible format
+ if (slice.viz_type === 'ag-grid-table' && chartStates[props.id]?.state) {
Review Comment:
I am concerned about these hardcoded logics that are specific to the viz type? Have you considered some alternative option?
##########
superset/models/helpers.py:
##########
@@ -2057,13 +2060,33 @@ def get_sqla_query( # pylint: disable=too-many-arguments,too-many-locals,too-ma
rejected_adhoc_filters_columns.append(flt_col)
continue
else:
+ # Check if it's a regular column first
col_obj = columns_by_name.get(cast(str, flt_col))
+ # If not found in columns, check if it's a metric
+ # This supports AG Grid table filters on metric columns
+ if (
+ col_obj is None
+ and extras.get("is_ag_grid_chart")
Review Comment:
Can we avoid this hardcoded logic here and add some tests?
--
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] feat(Chart): Save Chart State globally [superset]
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on code in PR #35343:
URL: https://github.com/apache/superset/pull/35343#discussion_r2465305095
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/buildQuery.ts:
##########
@@ -211,22 +213,137 @@ const buildQuery: BuildQuery<TableChartFormData> = (
moreProps.row_offset = currentPage * pageSize;
}
- // getting sort by in case of server pagination from own state
let sortByFromOwnState: QueryFormOrderBy[] | undefined;
- if (Array.isArray(ownState?.sortBy) && ownState?.sortBy.length > 0) {
- const sortByItem = ownState?.sortBy[0];
- sortByFromOwnState = [[sortByItem?.key, !sortByItem?.desc]];
+
+ const sortSource =
+ isDownloadQuery && ownState?.sortModel
+ ? ownState.sortModel
+ : ownState?.sortBy;
+
+ if (Array.isArray(sortSource) && sortSource.length > 0) {
+ const mapColIdToIdentifier = (colId: string): string | undefined => {
+ const matchingColumn = columns.find((col: QueryFormColumn) => {
+ if (typeof col === 'string') return col === colId;
+ return col?.sqlExpression === colId || col?.label === colId;
Review Comment:
We could use getColumnLabel here, which returns either label, sqlExpression, or just col name if col is string
--
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] feat(Chart): Save Chart State globally [superset]
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on code in PR #35343:
URL: https://github.com/apache/superset/pull/35343#discussion_r2465316111
##########
superset-frontend/src/dashboard/components/gridComponents/Chart/Chart.jsx:
##########
@@ -537,7 +576,13 @@ const Chart = props => {
formData={formData}
labelsColor={labelsColor}
labelsColorMap={labelsColorMap}
- ownState={dataMask[props.id]?.ownState}
+ ownState={{
+ ...dataMask[props.id]?.ownState,
+ ...(hasChartStateConverter(slice.viz_type) &&
+ chartStates[props.id]?.state
+ ? { savedAgGridState: chartStates[props.id].state }
Review Comment:
we shouldn't include a single plugin specific logic in here
--
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] feat(Chart): Save Chart State globally [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #35343:
URL: https://github.com/apache/superset/pull/35343#issuecomment-3426526178
πͺ [Showtime](https://github.com/mistercrunch/superset-showtime) deployed environment on [GHA](https://github.com/apache/superset/actions/runs/18684337579) for [7d7dd87](https://github.com/apache/superset/commit/7d7dd87)
β’ **Environment:** http://35.86.81.228:8080 (admin/admin)
β’ **Lifetime:** 48h auto-cleanup
β’ **Updates:** New commits create fresh environments automatically
--
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] feat(Chart): Save Chart State globally [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #35343:
URL: https://github.com/apache/superset/pull/35343#issuecomment-3426696963
πͺ [Showtime](https://github.com/mistercrunch/superset-showtime) deployed environment on [GHA](https://github.com/apache/superset/actions/runs/18685468551) for [7d7dd87](https://github.com/apache/superset/commit/7d7dd87)
β’ **Environment:** http://35.86.142.87:8080 (admin/admin)
β’ **Lifetime:** 48h auto-cleanup
β’ **Updates:** New commits create fresh environments automatically
--
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] feat(Chart): Save Chart State globally [superset]
Posted by "amaannawab923 (via GitHub)" <gi...@apache.org>.
amaannawab923 commented on code in PR #35343:
URL: https://github.com/apache/superset/pull/35343#discussion_r2414335903
##########
superset-frontend/src/dashboard/components/gridComponents/Chart/Chart.jsx:
##########
@@ -378,20 +392,139 @@ const Chart = props => {
slice_id: slice.slice_id,
is_cached: isCached,
});
+
+ let ownState = dataMask[props.id]?.ownState || {};
+
+ // For AG Grid tables, convert AG Grid state to backend-compatible format
+ if (slice.viz_type === 'ag-grid-table' && chartStates[props.id]?.state) {
+ const agGridState = chartStates[props.id].state;
+
+ // Convert AG Grid sortModel to backend sortBy format
+ if (agGridState.sortModel && agGridState.sortModel.length > 0) {
+ const sortItem = agGridState.sortModel[0];
+ ownState = {
+ ...ownState,
+ sortBy: [
+ {
+ id: sortItem.colId,
+ key: sortItem.colId,
+ desc: sortItem.sort === 'desc',
+ },
+ ],
+ };
+ }
+
+ // Store column order for backend processing
+ if (agGridState.columnState && agGridState.columnState.length > 0) {
+ ownState = {
+ ...ownState,
+ columnOrder: agGridState.columnState.map(col => col.colId),
+ };
+ }
+
+ if (
+ agGridState.filterModel &&
+ Object.keys(agGridState.filterModel).length > 0
+ ) {
+ const agGridFilters = [];
+
+ Object.keys(agGridState.filterModel).forEach(colId => {
+ const filter = agGridState.filterModel[colId];
+
+ // Text filter
+ if (filter.filterType === 'text' && filter.filter) {
+ const clause = {
+ expressionType: 'SIMPLE',
+ subject: colId,
+ operator:
+ filter.type === 'equals'
+ ? '=='
+ : filter.type === 'notEqual'
+ ? '!='
+ : filter.type === 'contains'
+ ? 'ILIKE'
+ : filter.type === 'notContains'
+ ? 'NOT ILIKE'
+ : filter.type === 'startsWith'
+ ? 'ILIKE'
+ : filter.type === 'endsWith'
+ ? 'ILIKE'
+ : 'ILIKE',
+ comparator:
+ filter.type === 'contains' || filter.type === 'notContains'
+ ? `%${filter.filter}%`
+ : filter.type === 'startsWith'
+ ? `${filter.filter}%`
+ : filter.type === 'endsWith'
+ ? `%${filter.filter}`
+ : filter.filter,
+ };
+ agGridFilters.push(clause);
+ } else if (
+ filter.filterType === 'number' &&
+ filter.filter !== undefined
+ ) {
+ const clause = {
+ expressionType: 'SIMPLE',
+ subject: colId,
+ operator:
+ filter.type === 'equals'
Review Comment:
Same here , a hashmap would be better
--
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] feat(Chart): Save Chart State globally [superset]
Posted by "amaannawab923 (via GitHub)" <gi...@apache.org>.
amaannawab923 commented on code in PR #35343:
URL: https://github.com/apache/superset/pull/35343#discussion_r2414331021
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTable/index.tsx:
##########
@@ -313,7 +398,14 @@ const AgGridDataTable: FunctionComponent<AgGridTableProps> = memo(
rowSelection="multiple"
animateRows
onCellClicked={handleCrossFilter}
+ onDragStopped={handleGridStateChange}
+ onColumnResized={handleGridStateChange}
+ onSortChanged={handleGridStateChange}
+ onFilterChanged={handleGridStateChange}
Review Comment:
use onStateUpdated from ag-grid-community
--
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] feat(Chart): Save Chart State globally [superset]
Posted by "amaannawab923 (via GitHub)" <gi...@apache.org>.
amaannawab923 commented on PR #35343:
URL: https://github.com/apache/superset/pull/35343#issuecomment-3461567776
LGTM !!
--
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] feat(Chart): Save Chart State globally [superset]
Posted by "LevisNgigi (via GitHub)" <gi...@apache.org>.
LevisNgigi commented on code in PR #35343:
URL: https://github.com/apache/superset/pull/35343#discussion_r2411579882
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTable/index.tsx:
##########
@@ -257,6 +318,24 @@ const AgGridDataTable: FunctionComponent<AgGridTableProps> = memo(
const onGridReady = (params: GridReadyEvent) => {
// This will make columns fill the grid width
params.api.sizeColumnsToFit();
+
+ // Restore saved AG Grid state from permalink if available
+ if (savedAgGridState && params.api) {
+ try {
+ if (savedAgGridState.columnState) {
+ params.api.applyColumnState?.({
+ state: savedAgGridState.columnState as any,
Review Comment:
Curious if there is a better type definition that we can use for this
##########
superset-frontend/src/dashboard/containers/DashboardPage.tsx:
##########
@@ -174,10 +174,11 @@ export const DashboardPage: FC<PageProps> = ({ idOrSlug }: PageProps) => {
// activeTabs is initialized with undefined so that it doesn't override
// the currently stored value when hydrating
let activeTabs: string[] | undefined;
+ let chartStates: Record<string, any> | undefined;
Review Comment:
same here for the type definition?
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/buildQuery.ts:
##########
@@ -292,6 +356,23 @@ const buildQuery: BuildQuery<TableChartFormData> = (
}
}
+ if (
+ isDownloadQuery &&
+ Array.isArray(ownState.agGridFilters) &&
+ ownState.agGridFilters.length > 0
+ ) {
+ const agGridQueryFilters = ownState.agGridFilters.map((filter: any) => ({
Review Comment:
same here might there be a better type definition?
##########
superset-frontend/src/dashboard/types.ts:
##########
@@ -123,6 +123,7 @@ export type DashboardState = {
dashboardId: number;
data: JsonObject;
};
+ chartStates?: Record<string, any>;
Review Comment:
same here if there is a better type definition
##########
superset-frontend/src/dashboard/types/chartState.ts:
##########
@@ -0,0 +1,65 @@
+/**
+ * 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.
+ */
+
+export interface AgGridColumnState {
+ colId: string;
+ width?: number;
+ hide?: boolean;
+ pinned?: 'left' | 'right' | null;
+ sort?: 'asc' | 'desc' | null;
+ sortIndex?: number;
+ aggFunc?: string;
+}
+
+export interface AgGridSortModel {
+ colId: string;
+ sort: 'asc' | 'desc';
+ sortIndex?: number;
+}
+
+export interface AgGridFilterModel {
+ [colId: string]: {
+ filterType: string;
+ type?: string;
+ filter?: any;
+ condition1?: any;
+ condition2?: any;
Review Comment:
I think it would be great to clean the types?
##########
superset-frontend/src/dashboard/components/gridComponents/Chart/Chart.jsx:
##########
@@ -378,20 +392,139 @@ const Chart = props => {
slice_id: slice.slice_id,
is_cached: isCached,
});
+
+ let ownState = dataMask[props.id]?.ownState || {};
+
+ // For AG Grid tables, convert AG Grid state to backend-compatible format
+ if (slice.viz_type === 'ag-grid-table' && chartStates[props.id]?.state) {
+ const agGridState = chartStates[props.id].state;
+
+ // Convert AG Grid sortModel to backend sortBy format
+ if (agGridState.sortModel && agGridState.sortModel.length > 0) {
+ const sortItem = agGridState.sortModel[0];
+ ownState = {
+ ...ownState,
+ sortBy: [
+ {
+ id: sortItem.colId,
+ key: sortItem.colId,
+ desc: sortItem.sort === 'desc',
+ },
+ ],
+ };
+ }
+
+ // Store column order for backend processing
+ if (agGridState.columnState && agGridState.columnState.length > 0) {
+ ownState = {
+ ...ownState,
+ columnOrder: agGridState.columnState.map(col => col.colId),
+ };
+ }
+
+ if (
+ agGridState.filterModel &&
+ Object.keys(agGridState.filterModel).length > 0
+ ) {
+ const agGridFilters = [];
+
+ Object.keys(agGridState.filterModel).forEach(colId => {
+ const filter = agGridState.filterModel[colId];
+
+ // Text filter
+ if (filter.filterType === 'text' && filter.filter) {
+ const clause = {
+ expressionType: 'SIMPLE',
+ subject: colId,
+ operator:
+ filter.type === 'equals'
+ ? '=='
+ : filter.type === 'notEqual'
+ ? '!='
+ : filter.type === 'contains'
+ ? 'ILIKE'
+ : filter.type === 'notContains'
+ ? 'NOT ILIKE'
+ : filter.type === 'startsWith'
+ ? 'ILIKE'
+ : filter.type === 'endsWith'
+ ? 'ILIKE'
+ : 'ILIKE',
+ comparator:
+ filter.type === 'contains' || filter.type === 'notContains'
+ ? `%${filter.filter}%`
+ : filter.type === 'startsWith'
+ ? `${filter.filter}%`
+ : filter.type === 'endsWith'
+ ? `%${filter.filter}`
+ : filter.filter,
+ };
+ agGridFilters.push(clause);
+ } else if (
+ filter.filterType === 'number' &&
+ filter.filter !== undefined
+ ) {
+ const clause = {
+ expressionType: 'SIMPLE',
+ subject: colId,
+ operator:
+ filter.type === 'equals'
+ ? '=='
+ : filter.type === 'notEqual'
+ ? '!='
+ : filter.type === 'lessThan'
+ ? '<'
+ : filter.type === 'lessThanOrEqual'
+ ? '<='
+ : filter.type === 'greaterThan'
+ ? '>'
+ : filter.type === 'greaterThanOrEqual'
+ ? '>='
+ : '==',
+ comparator: filter.filter,
+ };
+ agGridFilters.push(clause);
+ } else if (
+ filter.filterType === 'set' &&
+ Array.isArray(filter.values) &&
+ filter.values.length > 0
+ ) {
+ const clause = {
+ expressionType: 'SIMPLE',
+ subject: colId,
+ operator: 'IN',
+ comparator: filter.values,
+ };
+ agGridFilters.push(clause);
+ }
+ });
+
+ if (agGridFilters.length > 0) {
+ ownState = {
+ ...ownState,
+ agGridFilters,
+ };
+ }
+ }
+ }
+
exportChart({
formData: isFullCSV ? { ...formData, row_limit: maxRows } : formData,
resultType: isPivot ? 'post_processed' : 'full',
resultFormat: format,
force: true,
- ownState: dataMask[props.id]?.ownState,
Review Comment:
is the dataMask[props.id]?.ownState, no longer needed?
##########
superset-frontend/src/visualizations/presets/MainPreset.js:
##########
@@ -99,6 +99,11 @@ export default class MainPreset extends Preset {
? [new AgGridTableChartPlugin().configure({ key: VizType.TableAgGrid })]
: [];
+ console.log(
+ 'AG_GRID_TABLE_ENABLED',
+ isFeatureEnabled(FeatureFlag.AgGridTableEnabled),
+ );
Review Comment:
is this intentional?
--
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] feat(Chart): Save Chart State globally [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on code in PR #35343:
URL: https://github.com/apache/superset/pull/35343#discussion_r2410978941
##########
superset-frontend/src/dashboard/components/URLShortLinkButton/index.tsx:
##########
@@ -49,21 +49,39 @@ export default function URLShortLinkButton({
const theme = useTheme();
const [shortUrl, setShortUrl] = useState('');
const { addDangerToast } = useToasts();
- const { dataMask, activeTabs } = useSelector(
+ const { dataMask, activeTabs, chartStates, sliceEntities } = useSelector(
(state: RootState) => ({
dataMask: state.dataMask,
activeTabs: state.dashboardState.activeTabs,
+ chartStates: state.dashboardState.chartStates,
+ sliceEntities: state.sliceEntities,
}),
shallowEqual,
);
Review Comment:
### High Redux State Coupling <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The component is directly coupled to multiple Redux state properties, making it tightly coupled to the global state structure.
###### Why this matters
High coupling to global state makes the component less portable and harder to test. Changes to the Redux state structure would require changes to this component.
###### Suggested change β *Feature Preview*
Create a custom hook to abstract the state access and provide a cleaner interface:
```typescript
const useUrlGenerationData = () => {
return useSelector((state: RootState) => ({
dataMask: state.dataMask,
activeTabs: state.dashboardState.activeTabs,
chartStates: state.dashboardState.chartStates,
sliceEntities: state.sliceEntities,
}), shallowEqual);
};
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/43d4b7fd-ff3b-4ae5-b4e3-73bb26413f22/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/43d4b7fd-ff3b-4ae5-b4e3-73bb26413f22?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/43d4b7fd-ff3b-4ae5-b4e3-73bb26413f22?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/43d4b7fd-ff3b-4ae5-b4e3-73bb26413f22?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/43d4b7fd-ff3b-4ae5-b4e3-73bb26413f22)
</details>
<sub>
π¬ Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:b0dfd404-39a6-42ca-94bd-0aa3f93108a9 -->
[](b0dfd404-39a6-42ca-94bd-0aa3f93108a9)
##########
superset-frontend/src/dashboard/types/chartState.ts:
##########
@@ -0,0 +1,65 @@
+/**
+ * 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.
+ */
+
+export interface AgGridColumnState {
+ colId: string;
+ width?: number;
+ hide?: boolean;
+ pinned?: 'left' | 'right' | null;
+ sort?: 'asc' | 'desc' | null;
+ sortIndex?: number;
+ aggFunc?: string;
+}
+
+export interface AgGridSortModel {
+ colId: string;
+ sort: 'asc' | 'desc';
+ sortIndex?: number;
+}
+
+export interface AgGridFilterModel {
+ [colId: string]: {
+ filterType: string;
+ type?: string;
+ filter?: any;
+ condition1?: any;
+ condition2?: any;
+ operator?: string;
+ };
+}
+
+export interface AgGridChartState {
+ columnState: AgGridColumnState[];
+ sortModel: AgGridSortModel[];
+ filterModel: AgGridFilterModel;
+ columnOrder?: string[];
+ pageSize?: number;
+ currentPage?: number;
+}
+
+export interface ChartState {
+ chartId: number;
+ vizType: string;
+ state: AgGridChartState;
+ lastModified?: number;
+}
Review Comment:
### Chart state tightly coupled to AgGrid <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The ChartState interface is tightly coupled to AgGrid implementation details, violating the Interface Segregation Principle.
###### Why this matters
This coupling makes it difficult to support different chart implementations or grid libraries in the future, as the state structure is specifically designed for AgGrid.
###### Suggested change β *Feature Preview*
Create an abstraction layer:
```typescript
export interface BaseChartState {
chartId: number;
vizType: string;
lastModified?: number;
}
export interface AgGridChartState extends BaseChartState {
state: AgGridSpecificState;
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c44bdca3-51a6-4962-9f7a-06f240246113/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c44bdca3-51a6-4962-9f7a-06f240246113?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c44bdca3-51a6-4962-9f7a-06f240246113?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c44bdca3-51a6-4962-9f7a-06f240246113?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c44bdca3-51a6-4962-9f7a-06f240246113)
</details>
<sub>
π¬ Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:2f8d5822-242b-4569-9568-976a83e56e5f -->
[](2f8d5822-242b-4569-9568-976a83e56e5f)
##########
superset-frontend/src/dashboard/actions/dashboardState.js:
##########
@@ -747,6 +747,27 @@ export function setFullSizeChartId(chartId) {
return { type: SET_FULL_SIZE_CHART_ID, chartId };
}
+export const UPDATE_CHART_STATE = 'UPDATE_CHART_STATE';
+export function updateChartState(chartId, vizType, chartState) {
+ return {
+ type: UPDATE_CHART_STATE,
+ chartId,
+ vizType,
+ chartState,
+ lastModified: Date.now(),
+ };
+}
Review Comment:
### Missing parameter validation in updateChartState <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The updateChartState action creator does not validate its parameters, allowing undefined or null values to be passed through to the reducer.
###### Why this matters
This could lead to corrupted state if invalid parameters are passed, potentially causing runtime errors in components that depend on valid chart state data or making it impossible to properly restore chart states.
###### Suggested change β *Feature Preview*
Add parameter validation to ensure required fields are present:
```javascript
export function updateChartState(chartId, vizType, chartState) {
if (!chartId) {
throw new Error('chartId is required for updateChartState');
}
if (!vizType) {
throw new Error('vizType is required for updateChartState');
}
if (chartState === undefined || chartState === null) {
throw new Error('chartState is required for updateChartState');
}
return {
type: UPDATE_CHART_STATE,
chartId,
vizType,
chartState,
lastModified: Date.now(),
};
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8d51da37-b5cc-41ba-8e59-a9d6415aade2/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8d51da37-b5cc-41ba-8e59-a9d6415aade2?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8d51da37-b5cc-41ba-8e59-a9d6415aade2?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8d51da37-b5cc-41ba-8e59-a9d6415aade2?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8d51da37-b5cc-41ba-8e59-a9d6415aade2)
</details>
<sub>
π¬ Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:ddae6aee-f914-4d9b-bacf-3334d0899df9 -->
[](ddae6aee-f914-4d9b-bacf-3334d0899df9)
##########
superset/charts/schemas.py:
##########
@@ -1018,6 +1018,27 @@ class ChartDataExtrasSchema(Schema):
},
allow_none=True,
)
+ is_ag_grid_chart = fields.Boolean(
+ metadata={
+ "description": (
+ "Flag indicating if the query is from an AG Grid table chart. "
+ "When true, enables filtering on metric columns in addition "
+ "to dimension columns."
+ )
+ },
+ allow_none=True,
+ )
Review Comment:
### Missing default value for is_ag_grid_chart boolean field <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The is_ag_grid_chart field lacks validation or default value specification, which could lead to inconsistent behavior when the field is not explicitly set.
###### Why this matters
Without a default value, the field could be None, True, or False depending on how it's used, potentially causing unexpected behavior in downstream logic that depends on this flag to enable metric column filtering.
###### Suggested change β *Feature Preview*
Add a default value to ensure consistent behavior:
```python
is_ag_grid_chart = fields.Boolean(
metadata={
"description": (
"Flag indicating if the query is from an AG Grid table chart. "
"When true, enables filtering on metric columns in addition "
"to dimension columns."
)
},
allow_none=True,
load_default=False,
)
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/75ce4475-d576-41be-a98f-eca939c5cf6e/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/75ce4475-d576-41be-a98f-eca939c5cf6e?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/75ce4475-d576-41be-a98f-eca939c5cf6e?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/75ce4475-d576-41be-a98f-eca939c5cf6e?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/75ce4475-d576-41be-a98f-eca939c5cf6e)
</details>
<sub>
π¬ Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:6ca1ee0c-2231-4c7e-a188-e0d72994e1b2 -->
[](6ca1ee0c-2231-4c7e-a188-e0d72994e1b2)
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTable/index.tsx:
##########
@@ -114,11 +123,14 @@ const AgGridDataTable: FunctionComponent<AgGridTableProps> = memo(
cleanedTotals,
showTotals,
width,
+ onColumnStateChange,
+ savedAgGridState,
}) => {
const gridRef = useRef<AgGridReact>(null);
Review Comment:
### Unused gridRef prop creates broken external API access <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The component creates its own gridRef but also accepts an optional gridRef prop, creating potential conflicts and unused functionality.
###### Why this matters
The gridRef prop is accepted but never used, while the component always uses its internal gridRef. This means external components cannot access the grid API through the provided ref, breaking the expected functionality of the gridRef prop.
###### Suggested change β *Feature Preview*
Use the provided gridRef prop when available, falling back to the internal ref:
```typescript
const internalGridRef = useRef<AgGridReact>(null);
const effectiveGridRef = gridRef || internalGridRef;
```
Then replace all `gridRef.current` references with `effectiveGridRef.current` and pass `effectiveGridRef` to the ThemedAgGridReact component.
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/627ec5ea-add3-47d6-a04d-bc19a8c8eaeb/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/627ec5ea-add3-47d6-a04d-bc19a8c8eaeb?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/627ec5ea-add3-47d6-a04d-bc19a8c8eaeb?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/627ec5ea-add3-47d6-a04d-bc19a8c8eaeb?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/627ec5ea-add3-47d6-a04d-bc19a8c8eaeb)
</details>
<sub>
π¬ Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:3400a8ad-4a2b-4640-8e99-f8b488701240 -->
[](3400a8ad-4a2b-4640-8e99-f8b488701240)
##########
superset-frontend/src/dashboard/components/URLShortLinkButton/index.tsx:
##########
@@ -49,21 +49,39 @@ export default function URLShortLinkButton({
const theme = useTheme();
const [shortUrl, setShortUrl] = useState('');
const { addDangerToast } = useToasts();
- const { dataMask, activeTabs } = useSelector(
+ const { dataMask, activeTabs, chartStates, sliceEntities } = useSelector(
(state: RootState) => ({
dataMask: state.dataMask,
activeTabs: state.dashboardState.activeTabs,
+ chartStates: state.dashboardState.chartStates,
+ sliceEntities: state.sliceEntities,
}),
shallowEqual,
);
const getCopyUrl = async () => {
try {
+ // Check if dashboard has AG Grid tables (Table V2)
+ const hasAgGridTables =
+ sliceEntities &&
+ Object.values(sliceEntities).some(
+ slice =>
+ slice &&
+ typeof slice === 'object' &&
+ 'viz_type' in slice &&
+ slice.viz_type === 'ag_grid_table',
+ );
Review Comment:
### Violation of Single Responsibility Principle <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The logic for checking AG Grid tables is embedded within the URL generation component, violating the Single Responsibility Principle.
###### Why this matters
This coupling makes the component harder to maintain and less reusable. The component should focus on URL generation without needing to know the specific implementation details of table types.
###### Suggested change β *Feature Preview*
Extract the AG Grid table detection logic into a separate utility function or service:
```typescript
// tableUtils.ts
export const hasAgGridTables = (sliceEntities: any) => {
return Object.values(sliceEntities).some(
slice => slice?.viz_type === 'ag_grid_table'
);
};
// URLShortLinkButton
import { hasAgGridTables } from './tableUtils';
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e21cfa76-9bd5-4732-8d88-f2a56b410744/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e21cfa76-9bd5-4732-8d88-f2a56b410744?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e21cfa76-9bd5-4732-8d88-f2a56b410744?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e21cfa76-9bd5-4732-8d88-f2a56b410744?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e21cfa76-9bd5-4732-8d88-f2a56b410744)
</details>
<sub>
π¬ Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:f3e86ab9-d94e-4275-9a1c-16a31e93800b -->
[](f3e86ab9-d94e-4275-9a1c-16a31e93800b)
##########
superset-frontend/src/components/Chart/Chart.tsx:
##########
@@ -80,6 +80,7 @@ export interface ChartProps {
datasetsStatus?: 'loading' | 'error' | 'complete';
isInView?: boolean;
emitCrossFilters?: boolean;
+ onChartStateChange?: (chartState: JsonObject) => void;
Review Comment:
### Unwired chart state callback <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
New prop declared but not used to persist or propagate chart state; the intended global storage and propagation of chart state is not implemented yet.
###### Why this matters
Declaring the callback without wiring into the chart's state changes prevents persisting/restoring states via dashboard state/permalinks, meaning the feature will have no effect at runtime and is likely to confuse future maintainers.
###### Suggested change β *Feature Preview*
Wire the callback into actual chart state changes. For example, when the chart state changes inside the ChartRenderer, invoke the callback with the new state. This can be achieved by:
```tsx
// In Chart.tsx or where the chart state is updated
// Ensure ChartRenderer accepts a callback prop
<ChartRenderer
{...this.props}
onChartStateChange={this.props.onChartStateChange}
// ...other props
/>
```
```ts
// In ChartRenderer.tsx (or equivalent)
interface ChartRendererProps {
// ... existing props
onChartStateChange?: (state: JsonObject) => void;
}
// When chart state updates
private updateChartState(state: JsonObject) {
this.setState({ chartState: state });
this.props.onChartStateChange?.(state);
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bcbfd801-5ad8-49ac-b23b-6fd837be9b19/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bcbfd801-5ad8-49ac-b23b-6fd837be9b19?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bcbfd801-5ad8-49ac-b23b-6fd837be9b19?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bcbfd801-5ad8-49ac-b23b-6fd837be9b19?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bcbfd801-5ad8-49ac-b23b-6fd837be9b19)
</details>
<sub>
π¬ Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:6cb8ed06-d3bb-4bc8-922f-4c6c4d557a27 -->
[](6cb8ed06-d3bb-4bc8-922f-4c6c4d557a27)
--
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] feat(Chart): Save Chart State globally [superset]
Posted by "alexandrusoare (via GitHub)" <gi...@apache.org>.
alexandrusoare commented on code in PR #35343:
URL: https://github.com/apache/superset/pull/35343#discussion_r2435602954
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/buildQuery.ts:
##########
@@ -218,15 +218,79 @@ const buildQuery: BuildQuery<TableChartFormData> = (
sortByFromOwnState = [[sortByItem?.key, !sortByItem?.desc]];
}
+ // Note: In Superset, "columns" are dimensions and "metrics" are measures,
+ // but AG Grid treats them all as "columns" in the UI
+ let orderedColumns = columns;
+ let orderedMetrics = metrics;
Review Comment:
Good idea, took care of that!
--
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] feat(Chart): Save Chart State globally [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #35343:
URL: https://github.com/apache/superset/pull/35343#issuecomment-3389227259
πͺ [Showtime](https://github.com/mistercrunch/superset-showtime) is building environment on [GHA](https://github.com/apache/superset/actions/runs/18403603443) for [8c6885a](https://github.com/apache/superset/commit/8c6885a)
--
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] feat(Chart): Save Chart State globally [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #35343:
URL: https://github.com/apache/superset/pull/35343#issuecomment-3461204212
@kgabryje Ephemeral environment spinning up at http://50.112.177.10:8080. Credentials are 'admin'/'admin'. Please allow several minutes for bootstrapping and startup.
--
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] feat(Chart): Save Chart State globally [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #35343:
URL: https://github.com/apache/superset/pull/35343#issuecomment-3461166975
β οΈ **DEPRECATED WORKFLOW** β οΈ
@kgabryje This workflow is deprecated! Please use the new **Superset Showtime** system instead:
- Replace "testenv-up" label with "πͺ trigger-start"
- Better reliability and easier management
- See https://github.com/mistercrunch/superset-showtime for details
Processing your ephemeral environment request [here](https://github.com/apache/superset/actions/runs/18907217963). Action: **up**. More information on [how to use or configure ephemeral environments](https://superset.apache.org/docs/contributing/howtos/#github-ephemeral-environments)
--
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] feat(Chart): Save Chart State globally [superset]
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on code in PR #35343:
URL: https://github.com/apache/superset/pull/35343#discussion_r2465316111
##########
superset-frontend/src/dashboard/components/gridComponents/Chart/Chart.jsx:
##########
@@ -537,7 +576,13 @@ const Chart = props => {
formData={formData}
labelsColor={labelsColor}
labelsColorMap={labelsColorMap}
- ownState={dataMask[props.id]?.ownState}
+ ownState={{
+ ...dataMask[props.id]?.ownState,
+ ...(hasChartStateConverter(slice.viz_type) &&
+ chartStates[props.id]?.state
+ ? { savedAgGridState: chartStates[props.id].state }
Review Comment:
we shouldn't include a single plugin specific logic in here, but since we introduce a generic state converter logic, we could just name it chartState?
--
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] feat(Chart): Save Chart State globally [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #35343:
URL: https://github.com/apache/superset/pull/35343#issuecomment-3352974330
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/35343?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 50.71%. Comparing base ([`ef78d2a`](https://app.codecov.io/gh/apache/superset/commit/ef78d2af06bee6d613729cb3aba1357a307e977a?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`c0f6bd6`](https://app.codecov.io/gh/apache/superset/commit/c0f6bd690308e3f43a87657cfbe3e174b4f3b4c6?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #35343 +/- ##
===========================================
+ Coverage 0 50.71% +50.71%
===========================================
Files 0 588 +588
Lines 0 43495 +43495
Branches 0 4701 +4701
===========================================
+ Hits 0 22060 +22060
- Misses 0 20818 +20818
- Partials 0 617 +617
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/35343/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Ξ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/35343/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `46.29% <100.00%> (?)` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/35343/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `49.98% <100.00%> (?)` | |
| [python](https://app.codecov.io/gh/apache/superset/pull/35343/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `50.71% <100.00%> (?)` | |
| [unit](https://app.codecov.io/gh/apache/superset/pull/35343/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `100.00% <ΓΈ> (?)` | |
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/35343?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] feat(Chart): Save Chart State globally [superset]
Posted by "amaannawab923 (via GitHub)" <gi...@apache.org>.
amaannawab923 commented on code in PR #35343:
URL: https://github.com/apache/superset/pull/35343#discussion_r2426485247
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/buildQuery.ts:
##########
@@ -218,15 +218,79 @@ const buildQuery: BuildQuery<TableChartFormData> = (
sortByFromOwnState = [[sortByItem?.key, !sortByItem?.desc]];
}
+ // Note: In Superset, "columns" are dimensions and "metrics" are measures,
+ // but AG Grid treats them all as "columns" in the UI
+ let orderedColumns = columns;
+ let orderedMetrics = metrics;
+
+ if (
+ isDownloadQuery &&
+ ownState.columnOrder &&
+ Array.isArray(ownState.columnOrder)
+ ) {
+ const findMatchingItem = <T>(
+ items: T[],
Review Comment:
Do we have any interface for matching item , T is generic
--
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] feat(Chart): Save Chart State globally [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on PR #35343:
URL: https://github.com/apache/superset/pull/35343#issuecomment-3352946492
Based on your review schedule, I'll hold off on reviewing this PR until it's marked as ready for review. If you'd like me to take a look now, comment `/korbit-review`.
> Your admin can change your review schedule in the [Korbit Console](https://app.korbit.ai/settings)
[](c369cc93-4454-44e9-8c64-f74de263057c)
--
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] feat(Chart): Save Chart State globally [superset]
Posted by "LevisNgigi (via GitHub)" <gi...@apache.org>.
LevisNgigi commented on code in PR #35343:
URL: https://github.com/apache/superset/pull/35343#discussion_r2417888371
##########
superset-frontend/src/dashboard/components/gridComponents/Chart/Chart.jsx:
##########
@@ -378,20 +392,139 @@ const Chart = props => {
slice_id: slice.slice_id,
is_cached: isCached,
});
+
+ let ownState = dataMask[props.id]?.ownState || {};
+
+ // For AG Grid tables, convert AG Grid state to backend-compatible format
+ if (slice.viz_type === 'ag-grid-table' && chartStates[props.id]?.state) {
+ const agGridState = chartStates[props.id].state;
+
+ // Convert AG Grid sortModel to backend sortBy format
+ if (agGridState.sortModel && agGridState.sortModel.length > 0) {
+ const sortItem = agGridState.sortModel[0];
+ ownState = {
+ ...ownState,
+ sortBy: [
+ {
+ id: sortItem.colId,
+ key: sortItem.colId,
+ desc: sortItem.sort === 'desc',
+ },
+ ],
+ };
+ }
+
+ // Store column order for backend processing
+ if (agGridState.columnState && agGridState.columnState.length > 0) {
+ ownState = {
+ ...ownState,
+ columnOrder: agGridState.columnState.map(col => col.colId),
+ };
+ }
+
+ if (
+ agGridState.filterModel &&
+ Object.keys(agGridState.filterModel).length > 0
+ ) {
+ const agGridFilters = [];
+
+ Object.keys(agGridState.filterModel).forEach(colId => {
+ const filter = agGridState.filterModel[colId];
+
+ // Text filter
+ if (filter.filterType === 'text' && filter.filter) {
+ const clause = {
+ expressionType: 'SIMPLE',
+ subject: colId,
+ operator:
+ filter.type === 'equals'
+ ? '=='
+ : filter.type === 'notEqual'
+ ? '!='
+ : filter.type === 'contains'
+ ? 'ILIKE'
+ : filter.type === 'notContains'
+ ? 'NOT ILIKE'
+ : filter.type === 'startsWith'
+ ? 'ILIKE'
+ : filter.type === 'endsWith'
+ ? 'ILIKE'
+ : 'ILIKE',
+ comparator:
+ filter.type === 'contains' || filter.type === 'notContains'
+ ? `%${filter.filter}%`
+ : filter.type === 'startsWith'
+ ? `${filter.filter}%`
+ : filter.type === 'endsWith'
+ ? `%${filter.filter}`
+ : filter.filter,
+ };
+ agGridFilters.push(clause);
+ } else if (
+ filter.filterType === 'number' &&
+ filter.filter !== undefined
+ ) {
+ const clause = {
+ expressionType: 'SIMPLE',
+ subject: colId,
+ operator:
+ filter.type === 'equals'
+ ? '=='
+ : filter.type === 'notEqual'
+ ? '!='
+ : filter.type === 'lessThan'
+ ? '<'
+ : filter.type === 'lessThanOrEqual'
+ ? '<='
+ : filter.type === 'greaterThan'
+ ? '>'
+ : filter.type === 'greaterThanOrEqual'
+ ? '>='
+ : '==',
+ comparator: filter.filter,
+ };
+ agGridFilters.push(clause);
+ } else if (
+ filter.filterType === 'set' &&
+ Array.isArray(filter.values) &&
+ filter.values.length > 0
+ ) {
+ const clause = {
+ expressionType: 'SIMPLE',
+ subject: colId,
+ operator: 'IN',
+ comparator: filter.values,
+ };
+ agGridFilters.push(clause);
+ }
+ });
+
+ if (agGridFilters.length > 0) {
+ ownState = {
+ ...ownState,
+ agGridFilters,
+ };
+ }
+ }
+ }
+
exportChart({
formData: isFullCSV ? { ...formData, row_limit: maxRows } : formData,
resultType: isPivot ? 'post_processed' : 'full',
resultFormat: format,
force: true,
- ownState: dataMask[props.id]?.ownState,
Review Comment:
Okay sounds good thanks.
--
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] feat(Chart): Save Chart State globally [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #35343:
URL: https://github.com/apache/superset/pull/35343#issuecomment-3426670802
πͺ [Showtime](https://github.com/mistercrunch/superset-showtime) is building environment on [GHA](https://github.com/apache/superset/actions/runs/18685468551) for [7d7dd87](https://github.com/apache/superset/commit/7d7dd87)
--
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] feat(Chart): Save Chart State globally [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #35343:
URL: https://github.com/apache/superset/pull/35343#issuecomment-3426630496
πͺ [Showtime](https://github.com/mistercrunch/superset-showtime) is building environment on [GHA](https://github.com/apache/superset/actions/runs/18685264221) for [7d7dd87](https://github.com/apache/superset/commit/7d7dd87)
--
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] feat(Chart): Save Chart State globally [superset]
Posted by "amaannawab923 (via GitHub)" <gi...@apache.org>.
amaannawab923 commented on code in PR #35343:
URL: https://github.com/apache/superset/pull/35343#discussion_r2459721705
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/stateConversion.ts:
##########
@@ -0,0 +1,194 @@
+/**
+ * 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 {
+ BackendOwnState,
+ QueryFilterClause,
+ QuerySortBy,
+ type AgGridChartState,
+ type AgGridSortModel,
+ type AgGridFilterModel,
+} from '@superset-ui/core';
+
+/**
+ * AG Grid text filter type to backend operator mapping
+ */
+const TEXT_FILTER_OPERATORS: Record<string, string> = {
+ equals: '==',
+ notEqual: '!=',
+ contains: 'ILIKE',
+ notContains: 'NOT ILIKE',
+ startsWith: 'ILIKE',
+ endsWith: 'ILIKE',
+};
+
+/**
+ * AG Grid number filter type to backend operator mapping
+ */
+const NUMBER_FILTER_OPERATORS: Record<string, string> = {
+ equals: '==',
+ notEqual: '!=',
+ lessThan: '<',
+ lessThanOrEqual: '<=',
+ greaterThan: '>',
+ greaterThanOrEqual: '>=',
+};
+
+/**
+ * Converts text filter value based on filter type
+ */
+function getTextComparator(type: string, value: string): string {
+ if (type === 'contains' || type === 'notContains') {
+ return `%${value}%`;
+ }
+ if (type === 'startsWith') {
+ return `${value}%`;
+ }
+ if (type === 'endsWith') {
+ return `%${value}`;
+ }
+ return value;
+}
+
+/**
+ * Converts AG Grid sortModel to backend sortBy format
+ */
+export function convertSortModel(
+ sortModel: AgGridSortModel[],
+): QuerySortBy[] | undefined {
+ if (!sortModel || sortModel.length === 0) {
+ return undefined;
+ }
+
+ const sortItem = sortModel[0];
+ return [
+ {
+ id: sortItem.colId,
+ key: sortItem.colId,
+ desc: sortItem.sort === 'desc',
+ },
+ ];
+}
+
+/**
+ * Extracts column order from AG Grid columnState
+ */
+export function convertColumnState(
+ columnState: Array<{ colId: string }>,
+): string[] | undefined {
+ if (!columnState || columnState.length === 0) {
+ return undefined;
+ }
+
+ return columnState.map(col => col.colId);
+}
+
+/**
+ * Converts AG Grid filterModel to backend filter clauses
+ */
+export function convertFilterModel(
+ filterModel: AgGridFilterModel,
+): QueryFilterClause[] | undefined {
+ if (!filterModel || Object.keys(filterModel).length === 0) {
Review Comment:
How does this take care of filter model having AND , OR operators , Filter model looks something like this
{
"time_start": {
"filterType": "date",
"operator": "AND",
"conditions": [
{
"dateFrom": "1996-12-12 00:00:00",
"dateTo": null,
"filterType": "date",
"type": "lessThan"
},
{
"dateFrom": "1996-12-18 00:00:00",
"dateTo": null,
"filterType": "date",
"type": "equals"
}
]
}
}
How do we make sure that the conditions are properly applied with the operator, since we dont have conditions array handling here
--
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] feat(Chart): Save Chart State globally [superset]
Posted by "amaannawab923 (via GitHub)" <gi...@apache.org>.
amaannawab923 commented on code in PR #35343:
URL: https://github.com/apache/superset/pull/35343#discussion_r2459739374
##########
superset/models/helpers.py:
##########
@@ -2057,13 +2060,32 @@ def get_sqla_query( # pylint: disable=too-many-arguments,too-many-locals,too-ma
rejected_adhoc_filters_columns.append(flt_col)
continue
else:
+ # Check if it's a regular column first
col_obj = columns_by_name.get(cast(str, flt_col))
+ # If not found in columns, check if it's a metric
+ # This supports filtering on metric columns for any chart type
+ if (
+ col_obj is None
+ and isinstance(flt_col, str)
+ and flt_col in metrics_by_name
+ ):
+ # Convert metric to SQLA column expression
+ sqla_col = metrics_by_name[flt_col].get_sqla_col(
+ template_processor=template_processor
+ )
+ is_metric_filter = True
Review Comment:
Why do we need the is_metric_filter flag , the backend already handles it if you pass it in having clause for extras.having
--
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] feat(Chart): Save Chart State globally [superset]
Posted by "msyavuz (via GitHub)" <gi...@apache.org>.
msyavuz commented on code in PR #35343:
URL: https://github.com/apache/superset/pull/35343#discussion_r2460855641
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/stateConversion.ts:
##########
@@ -0,0 +1,194 @@
+/**
+ * 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 {
+ BackendOwnState,
+ QueryFilterClause,
+ QuerySortBy,
+ type AgGridChartState,
+ type AgGridSortModel,
+ type AgGridFilterModel,
+} from '@superset-ui/core';
+
+/**
+ * AG Grid text filter type to backend operator mapping
+ */
+const TEXT_FILTER_OPERATORS: Record<string, string> = {
+ equals: '==',
+ notEqual: '!=',
+ contains: 'ILIKE',
+ notContains: 'NOT ILIKE',
+ startsWith: 'ILIKE',
+ endsWith: 'ILIKE',
+};
+
+/**
+ * AG Grid number filter type to backend operator mapping
+ */
+const NUMBER_FILTER_OPERATORS: Record<string, string> = {
+ equals: '==',
+ notEqual: '!=',
+ lessThan: '<',
+ lessThanOrEqual: '<=',
+ greaterThan: '>',
+ greaterThanOrEqual: '>=',
+};
+
+/**
+ * Converts text filter value based on filter type
+ */
+function getTextComparator(type: string, value: string): string {
+ if (type === 'contains' || type === 'notContains') {
+ return `%${value}%`;
+ }
+ if (type === 'startsWith') {
+ return `${value}%`;
+ }
+ if (type === 'endsWith') {
+ return `%${value}`;
+ }
+ return value;
+}
+
+/**
+ * Converts AG Grid sortModel to backend sortBy format
+ */
+export function convertSortModel(
+ sortModel: AgGridSortModel[],
+): QuerySortBy[] | undefined {
+ if (!sortModel || sortModel.length === 0) {
+ return undefined;
+ }
+
+ const sortItem = sortModel[0];
+ return [
+ {
+ id: sortItem.colId,
+ key: sortItem.colId,
+ desc: sortItem.sort === 'desc',
+ },
+ ];
+}
+
+/**
+ * Extracts column order from AG Grid columnState
+ */
+export function convertColumnState(
+ columnState: Array<{ colId: string }>,
+): string[] | undefined {
+ if (!columnState || columnState.length === 0) {
+ return undefined;
+ }
+
+ return columnState.map(col => col.colId);
+}
+
+/**
+ * Converts AG Grid filterModel to backend filter clauses
+ */
+export function convertFilterModel(
+ filterModel: AgGridFilterModel,
+): QueryFilterClause[] | undefined {
+ if (!filterModel || Object.keys(filterModel).length === 0) {
+ return undefined;
+ }
+
+ const filters: QueryFilterClause[] = [];
+
+ Object.keys(filterModel).forEach(colId => {
+ const filter = filterModel[colId];
+
+ // Text filter
+ if (filter.filterType === 'text' && filter.filter && filter.type) {
+ filters.push({
+ col: colId,
+ op: TEXT_FILTER_OPERATORS[filter.type] || 'ILIKE',
+ val: getTextComparator(filter.type, filter.filter),
+ });
+ }
+ // Number filter
+ else if (
+ filter.filterType === 'number' &&
+ filter.filter !== undefined &&
+ filter.type
+ ) {
+ filters.push({
+ col: colId,
+ op: NUMBER_FILTER_OPERATORS[filter.type] || '==',
+ val: filter.filter,
+ });
+ }
+ // Set filter (multi-select)
+ else if (
+ filter.filterType === 'set' &&
+ Array.isArray(filter.values) &&
+ filter.values.length > 0
+ ) {
+ filters.push({
+ col: colId,
+ op: 'IN',
+ val: filter.values,
+ });
+ }
+ });
+
+ return filters.length > 0 ? filters : undefined;
+}
+
+/**
+ * Base converter for AG Grid-based charts (table, pivot, etc.)
+ * Converts AG Grid state to backend-compatible format.
+ *
+ * This can be extended by specific implementations (pivot) that need
+ * additional conversion logic.
+ */
+export function convertAgGridStateToOwnState(
Review Comment:
Naming of these could be better to reflect what they are converting to
--
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] feat(Chart): Save Chart State globally [superset]
Posted by "amaannawab923 (via GitHub)" <gi...@apache.org>.
amaannawab923 commented on code in PR #35343:
URL: https://github.com/apache/superset/pull/35343#discussion_r2459725590
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/test/buildQuery.test.ts:
##########
@@ -0,0 +1,478 @@
+/**
+ * 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 { AdhocColumn, QueryMode, VizType } from '@superset-ui/core';
+import buildQuery from '../src/buildQuery';
+import { TableChartFormData } from '../src/types';
+
+const basicFormData: TableChartFormData = {
+ viz_type: VizType.Table,
+ datasource: '11__table',
+ query_mode: QueryMode.Aggregate,
+ groupby: ['state'],
+ metrics: ['count'],
+};
+
+const createAdhocColumn = (
+ sqlExpression: string,
+ label: string,
+): AdhocColumn => ({
+ sqlExpression,
+ label,
+ expressionType: 'SQL',
+});
+
+describe('plugin-chart-ag-grid-table', () => {
+ describe('buildQuery - sort mapping for server pagination', () => {
+ it('should map string column colId to backend identifier', () => {
+ const query = buildQuery(
+ {
+ ...basicFormData,
+ server_pagination: true,
+ },
+ {
+ ownState: {
+ sortBy: [{ key: 'state', desc: false }],
+ },
+ },
+ ).queries[0];
+
+ expect(query.orderby).toEqual([['state', true]]);
+ });
+
+ it('should map AdhocColumn colId by sqlExpression', () => {
+ const adhocColumn = createAdhocColumn('degree_type', 'Highest Degree');
+
+ const query = buildQuery(
+ {
+ ...basicFormData,
+ server_pagination: true,
+ groupby: [adhocColumn],
+ },
+ {
+ ownState: {
+ sortBy: [{ key: 'degree_type', desc: false }],
+ },
+ },
+ ).queries[0];
+
+ expect(query.orderby).toEqual([['degree_type', true]]);
+ });
+
+ it('should map AdhocColumn colId by label', () => {
+ const adhocColumn = createAdhocColumn('degree_type', 'Highest Degree');
+
+ const query = buildQuery(
+ {
+ ...basicFormData,
+ server_pagination: true,
+ groupby: [adhocColumn],
+ },
+ {
+ ownState: {
+ sortBy: [{ key: 'Highest Degree', desc: false }],
+ },
+ },
+ ).queries[0];
+
+ expect(query.orderby).toEqual([['degree_type', true]]);
+ });
+
+ it('should map string metric colId to backend identifier', () => {
+ const query = buildQuery(
+ {
+ ...basicFormData,
+ server_pagination: true,
+ metrics: ['SUM(revenue)'],
+ },
+ {
+ ownState: {
+ sortBy: [{ key: 'SUM(revenue)', desc: true }],
+ },
+ },
+ ).queries[0];
+
+ expect(query.orderby).toEqual([['SUM(revenue)', false]]);
+ });
+
+ it('should map percent metric with % prefix', () => {
+ const query = buildQuery(
+ {
+ ...basicFormData,
+ server_pagination: true,
+ metrics: ['revenue'],
+ percent_metrics: ['revenue'],
+ },
+ {
+ ownState: {
+ sortBy: [{ key: '%revenue', desc: false }],
+ },
+ },
+ ).queries[0];
+
+ expect(query.orderby).toEqual([['revenue', true]]);
+ });
+
+ it('should handle desc sort direction correctly', () => {
+ const query = buildQuery(
+ {
+ ...basicFormData,
+ server_pagination: true,
+ },
+ {
+ ownState: {
+ sortBy: [{ key: 'state', desc: true }],
+ },
+ },
+ ).queries[0];
+
+ expect(query.orderby).toEqual([['state', false]]);
+ });
+ });
+
+ describe('buildQuery - CSV export with sortModel', () => {
+ it('should use sortModel for download queries', () => {
+ const query = buildQuery(
+ {
+ ...basicFormData,
+ result_format: 'csv',
+ },
+ {
+ ownState: {
+ sortModel: [{ colId: 'state', sort: 'asc' }],
+ sortBy: [{ key: 'other', desc: false }],
+ },
+ },
+ ).queries[0];
+
+ expect(query.orderby).toEqual([
+ ['state', true],
+ ['count', false],
+ ]);
+ });
+
+ it('should map sortModel with desc direction', () => {
+ const query = buildQuery(
+ {
+ ...basicFormData,
+ result_format: 'csv',
+ },
+ {
+ ownState: {
+ sortModel: [{ colId: 'state', sort: 'desc' }],
+ },
+ },
+ ).queries[0];
+
+ expect(query.orderby[0]).toEqual(['state', false]);
+ });
+
+ it('should handle multi-column sort from sortModel', () => {
+ const query = buildQuery(
+ {
+ ...basicFormData,
+ groupby: ['state', 'city'],
+ result_format: 'csv',
+ },
+ {
+ ownState: {
+ sortModel: [
+ { colId: 'state', sort: 'asc', sortIndex: 0 },
+ { colId: 'city', sort: 'desc', sortIndex: 1 },
+ ],
+ },
+ },
+ ).queries[0];
+
+ expect(query.orderby).toEqual([
+ ['state', true],
+ ['city', false],
+ ]);
+ });
+ });
+
+ describe('buildQuery - stable sort tie-breaker', () => {
+ it('should add default orderby as tie-breaker for single-column CSV export', () => {
+ const query = buildQuery(
+ {
+ ...basicFormData,
+ result_format: 'csv',
+ metrics: ['count'],
+ },
+ {
+ ownState: {
+ sortModel: [{ colId: 'state', sort: 'asc' }],
+ },
+ },
+ ).queries[0];
+
+ expect(query.orderby).toEqual([
+ ['state', true],
+ ['count', false],
+ ]);
+ });
+
+ it('should not add tie-breaker if primary sort matches default orderby', () => {
+ const query = buildQuery(
+ {
+ ...basicFormData,
+ result_format: 'csv',
+ metrics: ['count'],
+ },
+ {
+ ownState: {
+ sortModel: [{ colId: 'count', sort: 'desc' }],
+ },
+ },
+ ).queries[0];
+
+ expect(query.orderby).toEqual([['count', false]]);
+ });
+
+ it('should not add tie-breaker for multi-column sorts', () => {
+ const query = buildQuery(
+ {
+ ...basicFormData,
+ groupby: ['state', 'city'],
+ result_format: 'csv',
+ },
+ {
+ ownState: {
+ sortModel: [
+ { colId: 'state', sort: 'asc' },
+ { colId: 'city', sort: 'desc' },
+ ],
+ },
+ },
+ ).queries[0];
+
+ expect(query.orderby).toEqual([
+ ['state', true],
+ ['city', false],
+ ]);
+ });
+
+ it('should not add tie-breaker for non-download queries with server pagination', () => {
+ const query = buildQuery(
+ {
+ ...basicFormData,
+ server_pagination: true,
+ },
+ {
+ ownState: {
+ sortBy: [{ key: 'state', desc: false }],
+ },
+ },
+ ).queries[0];
+
+ expect(query.orderby).toEqual([['state', true]]);
+ });
+ });
+
+ describe('buildQuery - filter handling for CSV export', () => {
+ it('should apply AG Grid filters for download queries', () => {
+ const query = buildQuery(
+ {
+ ...basicFormData,
+ result_format: 'csv',
+ },
+ {
+ ownState: {
+ filters: [
+ {
+ col: 'state',
+ op: 'IN',
+ val: ['CA', 'NY'],
+ },
+ ],
+ },
+ },
+ ).queries[0];
+
+ expect(query.filters).toContainEqual({
+ col: 'state',
+ op: 'IN',
+ val: ['CA', 'NY'],
+ });
+ });
+
+ it('should append AG Grid filters to existing filters', () => {
+ const query = buildQuery(
+ {
+ ...basicFormData,
+ result_format: 'csv',
+ adhoc_filters: [
+ {
+ expressionType: 'SIMPLE',
+ subject: 'country',
+ operator: '==',
+ comparator: 'USA',
+ clause: 'WHERE',
+ },
Review Comment:
Do we need adhoc filters , extras.where and extras.having should be handling this
--
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] feat(Chart): Save Chart State globally [superset]
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on code in PR #35343:
URL: https://github.com/apache/superset/pull/35343#discussion_r2465302987
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/buildQuery.ts:
##########
@@ -211,22 +213,137 @@ const buildQuery: BuildQuery<TableChartFormData> = (
moreProps.row_offset = currentPage * pageSize;
}
- // getting sort by in case of server pagination from own state
let sortByFromOwnState: QueryFormOrderBy[] | undefined;
- if (Array.isArray(ownState?.sortBy) && ownState?.sortBy.length > 0) {
- const sortByItem = ownState?.sortBy[0];
- sortByFromOwnState = [[sortByItem?.key, !sortByItem?.desc]];
+
+ const sortSource =
+ isDownloadQuery && ownState?.sortModel
+ ? ownState.sortModel
+ : ownState?.sortBy;
+
+ if (Array.isArray(sortSource) && sortSource.length > 0) {
+ const mapColIdToIdentifier = (colId: string): string | undefined => {
+ const matchingColumn = columns.find((col: QueryFormColumn) => {
+ if (typeof col === 'string') return col === colId;
+ return col?.sqlExpression === colId || col?.label === colId;
+ });
+
+ if (matchingColumn) {
+ return typeof matchingColumn === 'string'
+ ? matchingColumn
+ : matchingColumn.sqlExpression || matchingColumn.label;
+ }
+
+ const matchingMetric = (metrics || []).find((met: QueryFormMetric) => {
+ if (typeof met === 'string')
+ return met === colId || `%${met}` === colId;
+ const metLabel = getMetricLabel(met);
Review Comment:
I think this could be simplified a bit, as getMetricLabel works with string metrics too, so the first check might be redundant
--
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] feat(Chart): Save Chart State globally [superset]
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on code in PR #35343:
URL: https://github.com/apache/superset/pull/35343#discussion_r2465326896
##########
superset-frontend/packages/superset-ui-core/src/types/AgGrid.ts:
##########
@@ -0,0 +1,116 @@
+/**
+ * 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, SortModelItem } from 'ag-grid-community';
+
+// AG Grid filter type enums
+export enum AgGridFilterType {
+ Text = 'text',
+ Number = 'number',
+ Date = 'date',
+ Set = 'set',
+}
+
+export enum AgGridTextFilterOperator {
+ Equals = 'equals',
+ NotEqual = 'notEqual',
+ Contains = 'contains',
+ NotContains = 'notContains',
+ StartsWith = 'startsWith',
+ EndsWith = 'endsWith',
+ Blank = 'blank',
+ NotBlank = 'notBlank',
+}
+
+export enum AgGridNumberFilterOperator {
+ Equals = 'equals',
+ NotEqual = 'notEqual',
+ LessThan = 'lessThan',
+ LessThanOrEqual = 'lessThanOrEqual',
+ GreaterThan = 'greaterThan',
+ GreaterThanOrEqual = 'greaterThanOrEqual',
+ InRange = 'inRange',
+ Blank = 'blank',
+ NotBlank = 'notBlank',
+}
+
+/**
+ * @deprecated Use ColumnState from 'ag-grid-community' instead
+ */
+export type AgGridColumnState = ColumnState;
Review Comment:
Is this used anywhere? We probably shouldn't introduce a new type which is deprecated from start
--
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] feat(Chart): Save Chart State globally [superset]
Posted by "geido (via GitHub)" <gi...@apache.org>.
geido commented on code in PR #35343:
URL: https://github.com/apache/superset/pull/35343#discussion_r2459719136
##########
superset-frontend/src/dashboard/components/gridComponents/Chart/Chart.jsx:
##########
@@ -537,7 +576,13 @@ const Chart = props => {
formData={formData}
labelsColor={labelsColor}
labelsColorMap={labelsColorMap}
- ownState={dataMask[props.id]?.ownState}
+ ownState={{
+ ...dataMask[props.id]?.ownState,
+ ...(hasChartStateConverter(slice.viz_type) &&
+ chartStates[props.id]?.state
+ ? { savedAgGridState: chartStates[props.id].state }
Review Comment:
Do we still need this?
##########
superset/dashboards/permalink/schemas.py:
##########
@@ -52,6 +52,16 @@ class DashboardPermalinkStateSchema(Schema):
allow_none=True,
metadata={"description": "Optional anchor link added to url hash"},
)
+ chartStates = fields.Dict( # noqa: N815
+ required=False,
+ allow_none=True,
+ metadata={
+ "description": (
+ "Chart-level state for AG Grid tables "
Review Comment:
Should this be generalized?
--
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] feat(Chart): Save Chart State globally [superset]
Posted by "geido (via GitHub)" <gi...@apache.org>.
geido commented on PR #35343:
URL: https://github.com/apache/superset/pull/35343#issuecomment-3461051824
Also, it would be great to add a video!
--
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] feat(Chart): Save Chart State globally [superset]
Posted by "geido (via GitHub)" <gi...@apache.org>.
geido merged PR #35343:
URL: https://github.com/apache/superset/pull/35343
--
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] feat(Chart): Save Chart State globally [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #35343:
URL: https://github.com/apache/superset/pull/35343#issuecomment-3389276918
πͺ [Showtime](https://github.com/mistercrunch/superset-showtime) deployed environment on [GHA](https://github.com/apache/superset/actions/runs/18403603443) for [8c6885a](https://github.com/apache/superset/commit/8c6885a)
β’ **Environment:** http://18.236.98.235:8080 (admin/admin)
β’ **Lifetime:** 48h auto-cleanup
β’ **Updates:** New commits create fresh environments automatically
--
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] feat(Chart): Save Chart State globally [superset]
Posted by "alexandrusoare (via GitHub)" <gi...@apache.org>.
alexandrusoare commented on code in PR #35343:
URL: https://github.com/apache/superset/pull/35343#discussion_r2419046002
##########
superset-frontend/src/dashboard/components/gridComponents/Chart/Chart.jsx:
##########
@@ -378,20 +392,139 @@ const Chart = props => {
slice_id: slice.slice_id,
is_cached: isCached,
});
+
+ let ownState = dataMask[props.id]?.ownState || {};
+
+ // For AG Grid tables, convert AG Grid state to backend-compatible format
+ if (slice.viz_type === 'ag-grid-table' && chartStates[props.id]?.state) {
+ const agGridState = chartStates[props.id].state;
+
+ // Convert AG Grid sortModel to backend sortBy format
+ if (agGridState.sortModel && agGridState.sortModel.length > 0) {
+ const sortItem = agGridState.sortModel[0];
+ ownState = {
+ ...ownState,
+ sortBy: [
+ {
+ id: sortItem.colId,
+ key: sortItem.colId,
+ desc: sortItem.sort === 'desc',
+ },
+ ],
+ };
+ }
+
+ // Store column order for backend processing
+ if (agGridState.columnState && agGridState.columnState.length > 0) {
+ ownState = {
+ ...ownState,
+ columnOrder: agGridState.columnState.map(col => col.colId),
+ };
+ }
+
+ if (
+ agGridState.filterModel &&
+ Object.keys(agGridState.filterModel).length > 0
+ ) {
+ const agGridFilters = [];
+
+ Object.keys(agGridState.filterModel).forEach(colId => {
+ const filter = agGridState.filterModel[colId];
+
+ // Text filter
+ if (filter.filterType === 'text' && filter.filter) {
+ const clause = {
+ expressionType: 'SIMPLE',
+ subject: colId,
+ operator:
+ filter.type === 'equals'
+ ? '=='
+ : filter.type === 'notEqual'
+ ? '!='
+ : filter.type === 'contains'
+ ? 'ILIKE'
+ : filter.type === 'notContains'
+ ? 'NOT ILIKE'
+ : filter.type === 'startsWith'
+ ? 'ILIKE'
+ : filter.type === 'endsWith'
+ ? 'ILIKE'
+ : 'ILIKE',
+ comparator:
+ filter.type === 'contains' || filter.type === 'notContains'
+ ? `%${filter.filter}%`
+ : filter.type === 'startsWith'
+ ? `${filter.filter}%`
+ : filter.type === 'endsWith'
+ ? `%${filter.filter}`
+ : filter.filter,
+ };
+ agGridFilters.push(clause);
+ } else if (
+ filter.filterType === 'number' &&
+ filter.filter !== undefined
+ ) {
+ const clause = {
+ expressionType: 'SIMPLE',
+ subject: colId,
+ operator:
+ filter.type === 'equals'
+ ? '=='
+ : filter.type === 'notEqual'
+ ? '!='
+ : filter.type === 'lessThan'
+ ? '<'
+ : filter.type === 'lessThanOrEqual'
+ ? '<='
+ : filter.type === 'greaterThan'
+ ? '>'
+ : filter.type === 'greaterThanOrEqual'
+ ? '>='
+ : '==',
+ comparator: filter.filter,
+ };
+ agGridFilters.push(clause);
+ } else if (
+ filter.filterType === 'set' &&
+ Array.isArray(filter.values) &&
+ filter.values.length > 0
+ ) {
+ const clause = {
+ expressionType: 'SIMPLE',
+ subject: colId,
+ operator: 'IN',
+ comparator: filter.values,
+ };
+ agGridFilters.push(clause);
+ }
+ });
+
+ if (agGridFilters.length > 0) {
+ ownState = {
+ ...ownState,
+ agGridFilters,
+ };
+ }
+ }
+ }
Review Comment:
We still need to send the type of filter and so on, changed a few a things
##########
superset-frontend/src/dashboard/components/menu/ShareMenuItems/index.tsx:
##########
@@ -58,20 +58,39 @@ export const useShareMenuItems = (props: ShareMenuItemProps): MenuItem => {
disabled,
...rest
} = props;
- const { dataMask, activeTabs } = useSelector(
+ const { dataMask, activeTabs, chartStates, sliceEntities } = useSelector(
(state: RootState) => ({
dataMask: state.dataMask,
activeTabs: state.dashboardState.activeTabs,
+ chartStates: state.dashboardState.chartStates,
+ sliceEntities: state.sliceEntities,
}),
shallowEqual,
);
async function generateUrl() {
+ // Check if dashboard has AG Grid tables
+ const hasAgGridTables =
+ sliceEntities &&
+ Object.values(sliceEntities).some(
+ slice =>
+ slice &&
+ typeof slice === 'object' &&
+ 'viz_type' in slice &&
+ slice.viz_type === 'ag_grid_table',
+ );
+
Review Comment:
Done, thanks
--
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