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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
