diff --git a/superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTableChart.tsx b/superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTableChart.tsx index b4eae18de41..0673bdd483a 100644 --- a/superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTableChart.tsx +++ b/superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTableChart.tsx @@ -85,6 +85,7 @@ export default function TableChart( width, onChartStateChange, chartState, + metricSqlExpressions, } = props; const [searchOptions, setSearchOptions] = useState([]); @@ -187,6 +188,7 @@ export default function TableChart( lastFilteredColumn: completeFilterState.lastFilteredColumn, lastFilteredInputPosition: completeFilterState.inputPosition, currentPage: 0, // Reset to first page when filtering + metricSqlExpressions, }; updateTableOwnState(setDataMask, modifiedOwnState); @@ -197,6 +199,7 @@ export default function TableChart( serverPaginationData, onChartStateChange, chartState, + metricSqlExpressions, ], ); diff --git a/superset-frontend/plugins/plugin-chart-ag-grid-table/src/buildQuery.ts b/superset-frontend/plugins/plugin-chart-ag-grid-table/src/buildQuery.ts index 8122c86f825..3d04b6c1d1c 100644 --- a/superset-frontend/plugins/plugin-chart-ag-grid-table/src/buildQuery.ts +++ b/superset-frontend/plugins/plugin-chart-ag-grid-table/src/buildQuery.ts @@ -482,12 +482,77 @@ const buildQuery: BuildQuery = ( }; } - // Add AG Grid complex WHERE clause from ownState (non-metric filters) + // Map metric/column labels to SQL expressions for WHERE/HAVING resolution + const sqlExpressionMap: Record = {}; + (metrics || []).forEach((m: QueryFormMetric) => { + if (typeof m === 'object' && 'expressionType' in m) { + const label = getMetricLabel(m); + if (m.expressionType === 'SQL' && m.sqlExpression) { + sqlExpressionMap[label] = m.sqlExpression; + } else if ( + m.expressionType === 'SIMPLE' && + m.aggregate && + m.column?.column_name + ) { + sqlExpressionMap[label] = `${m.aggregate}(${m.column.column_name})`; + } + } + }); + // Map dimension columns with custom SQL expressions + (columns || []).forEach((col: QueryFormColumn) => { + if (typeof col === 'object' && 'sqlExpression' in col) { + const label = getColumnLabel(col); + if (col.sqlExpression) { + sqlExpressionMap[label] = col.sqlExpression; + } + } + }); + // Merge datasource-level saved metrics and calculated columns + if (ownState.metricSqlExpressions) { + Object.entries( + ownState.metricSqlExpressions as Record, + ).forEach(([label, expression]) => { + if (!sqlExpressionMap[label]) { + sqlExpressionMap[label] = expression; + } + }); + } + + const resolveLabelsToSQL = (clause: string): string => { + let resolved = clause; + // Sort by label length descending to prevent substring false positives + const sortedEntries = Object.entries(sqlExpressionMap).sort( + ([a], [b]) => b.length - a.length, + ); + sortedEntries.forEach(([label, expression]) => { + if (resolved.includes(label)) { + const escapedLabel = label.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); + // Wrap complex expressions in parentheses for valid SQL + const isExpression = + expression.includes('(') || + expression.toUpperCase().includes('CASE') || + expression.includes('\n'); + const wrappedExpression = isExpression + ? `(${expression})` + : expression; + resolved = resolved.replace( + new RegExp(`\\b${escapedLabel}\\b`, 'g'), + wrappedExpression, + ); + } + }); + return resolved; + }; + + // Resolve and apply AG Grid WHERE clause if (ownState.agGridComplexWhere && ownState.agGridComplexWhere.trim()) { + const resolvedWhere = resolveLabelsToSQL(ownState.agGridComplexWhere); + (ownState as Record).agGridComplexWhere = + resolvedWhere; const existingWhere = queryObject.extras?.where; const combinedWhere = existingWhere - ? `${existingWhere} AND ${ownState.agGridComplexWhere}` - : ownState.agGridComplexWhere; + ? `${existingWhere} AND ${resolvedWhere}` + : resolvedWhere; queryObject = { ...queryObject, @@ -498,12 +563,15 @@ const buildQuery: BuildQuery = ( }; } - // Add AG Grid HAVING clause from ownState (metric filters only) + // Resolve and apply AG Grid HAVING clause if (ownState.agGridHavingClause && ownState.agGridHavingClause.trim()) { + const resolvedHaving = resolveLabelsToSQL(ownState.agGridHavingClause); + (ownState as Record).agGridHavingClause = + resolvedHaving; const existingHaving = queryObject.extras?.having; const combinedHaving = existingHaving - ? `${existingHaving} AND ${ownState.agGridHavingClause}` - : ownState.agGridHavingClause; + ? `${existingHaving} AND ${resolvedHaving}` + : resolvedHaving; queryObject = { ...queryObject, diff --git a/superset-frontend/plugins/plugin-chart-ag-grid-table/src/transformProps.ts b/superset-frontend/plugins/plugin-chart-ag-grid-table/src/transformProps.ts index be174cb5a21..c27bd856d89 100644 --- a/superset-frontend/plugins/plugin-chart-ag-grid-table/src/transformProps.ts +++ b/superset-frontend/plugins/plugin-chart-ag-grid-table/src/transformProps.ts @@ -34,6 +34,8 @@ import { SMART_DATE_ID, TimeFormats, TimeFormatter, + AgGridChartState, + AgGridFilterModel, } from '@superset-ui/core'; import { GenericDataType } from '@apache-superset/core/common'; import { isEmpty, isEqual, merge } from 'lodash'; @@ -456,6 +458,9 @@ const getPageSize = ( return numRecords * numColumns > 5000 ? 200 : 0; }; +// Tracks slice_ids that have already applied their saved chartState filter on mount +const savedFilterAppliedSet = new Set(); + const transformProps = ( chartProps: TableChartProps, ): AgGridTableChartTransformedProps => { @@ -710,6 +715,36 @@ const transformProps = ( : totalQuery?.data[0] : undefined; + // Map saved metric/calculated column labels to their SQL expressions for filter resolution + const metricSqlExpressions: Record = {}; + chartProps.datasource.metrics.forEach(metric => { + if (metric.metric_name && metric.expression) { + metricSqlExpressions[metric.metric_name] = metric.expression; + } + }); + chartProps.datasource.columns.forEach(col => { + if (col.column_name && col.expression) { + metricSqlExpressions[col.column_name] = col.expression; + if (col.verbose_name && col.verbose_name !== col.column_name) { + metricSqlExpressions[col.verbose_name] = col.expression; + } + } + }); + + // Strip saved filter from chartState after initial application to prevent re-injection + let chartState = serverPaginationData?.chartState as + | AgGridChartState + | undefined; + const chartStateHasFilter = !!( + chartState?.filterModel && Object.keys(chartState.filterModel).length > 0 + ); + + if (chartStateHasFilter && savedFilterAppliedSet.has(slice_id)) { + chartState = { ...chartState!, filterModel: {} as AgGridFilterModel }; + } else if (chartStateHasFilter) { + savedFilterAppliedSet.add(slice_id); + } + return { height, width, @@ -742,7 +777,8 @@ const transformProps = ( basicColorColumnFormatters, basicColorFormatters, formData, - chartState: serverPaginationData?.chartState, + metricSqlExpressions, + chartState, onChartStateChange, }; }; diff --git a/superset-frontend/plugins/plugin-chart-ag-grid-table/src/types.ts b/superset-frontend/plugins/plugin-chart-ag-grid-table/src/types.ts index d97f36b267f..e3d6bf07079 100644 --- a/superset-frontend/plugins/plugin-chart-ag-grid-table/src/types.ts +++ b/superset-frontend/plugins/plugin-chart-ag-grid-table/src/types.ts @@ -128,6 +128,7 @@ export interface AgGridTableChartTransformedProps< basicColorFormatters?: { [Key: string]: BasicColorFormatterType }[]; basicColorColumnFormatters?: { [Key: string]: BasicColorFormatterType }[]; formData: TableChartFormData; + metricSqlExpressions: Record; onChartStateChange?: (chartState: JsonObject) => void; chartState?: AgGridChartState; } diff --git a/superset-frontend/plugins/plugin-chart-ag-grid-table/test/buildQuery.test.ts b/superset-frontend/plugins/plugin-chart-ag-grid-table/test/buildQuery.test.ts index 0778d6fde58..636fc190fcc 100644 --- a/superset-frontend/plugins/plugin-chart-ag-grid-table/test/buildQuery.test.ts +++ b/superset-frontend/plugins/plugin-chart-ag-grid-table/test/buildQuery.test.ts @@ -1090,4 +1090,258 @@ describe('plugin-chart-ag-grid-table', () => { expect(query.metrics).toEqual([]); }); }); + + describe('buildQuery - label-to-SQL resolution in WHERE/HAVING', () => { + test('should resolve inline SQL metric labels in WHERE clause', () => { + const query = buildQuery( + { + ...basicFormData, + server_pagination: true, + metrics: [ + { + expressionType: 'SQL', + sqlExpression: 'SUM(revenue)', + label: 'Total Revenue', + }, + ], + }, + { + ownState: { + agGridComplexWhere: 'Total Revenue > 1000', + }, + }, + ).queries[0]; + + expect(query.extras?.where).toBe('(SUM(revenue)) > 1000'); + }); + + test('should resolve SIMPLE metric labels in HAVING clause', () => { + const query = buildQuery( + { + ...basicFormData, + server_pagination: true, + metrics: [ + { + expressionType: 'SIMPLE', + aggregate: 'SUM', + column: { column_name: 'revenue' }, + label: 'Total Revenue', + }, + ], + }, + { + ownState: { + agGridHavingClause: 'Total Revenue > 1000', + }, + }, + ).queries[0]; + + expect(query.extras?.having).toBe('(SUM(revenue)) > 1000'); + }); + + test('should resolve adhoc column SQL expressions in WHERE clause', () => { + const adhocColumn = createAdhocColumn('UPPER(city)', 'City Upper'); + + const query = buildQuery( + { + ...basicFormData, + server_pagination: true, + groupby: [adhocColumn], + }, + { + ownState: { + agGridComplexWhere: "City Upper = 'NEW YORK'", + }, + }, + ).queries[0]; + + expect(query.extras?.where).toContain('UPPER(city)'); + }); + + test('should wrap CASE expressions in parentheses', () => { + const query = buildQuery( + { + ...basicFormData, + server_pagination: true, + }, + { + ownState: { + agGridComplexWhere: "degree_level = 'High'", + metricSqlExpressions: { + degree_level: + "CASE WHEN degree = 'PhD' THEN 'High' ELSE 'Low' END", + }, + }, + }, + ).queries[0]; + + expect(query.extras?.where).toBe( + "(CASE WHEN degree = 'PhD' THEN 'High' ELSE 'Low' END) = 'High'", + ); + }); + + test('should wrap aggregate expressions in parentheses', () => { + const query = buildQuery( + { + ...basicFormData, + server_pagination: true, + }, + { + ownState: { + agGridHavingClause: 'total_count > 100', + metricSqlExpressions: { + total_count: 'COUNT(*)', + }, + }, + }, + ).queries[0]; + + expect(query.extras?.having).toBe('(COUNT(*)) > 100'); + }); + + test('should quote simple column names without parentheses', () => { + const query = buildQuery( + { + ...basicFormData, + server_pagination: true, + }, + { + ownState: { + agGridComplexWhere: "status = 'active'", + metricSqlExpressions: { + status: 'user_status', + }, + }, + }, + ).queries[0]; + + expect(query.extras?.where).toBe('"user_status" = \'active\''); + }); + + test('should resolve longer labels before shorter ones', () => { + const query = buildQuery( + { + ...basicFormData, + server_pagination: true, + metrics: [ + { + expressionType: 'SQL', + sqlExpression: 'COUNT(*)', + label: 'count', + }, + { + expressionType: 'SQL', + sqlExpression: 'COUNT(DISTINCT id)', + label: 'count_distinct', + }, + ], + }, + { + ownState: { + agGridHavingClause: 'count_distinct > 5 AND count > 10', + }, + }, + ).queries[0]; + + expect(query.extras?.having).toContain('(COUNT(DISTINCT id)) > 5'); + expect(query.extras?.having).toContain('(COUNT(*)) > 10'); + }); + + test('should prefer query-level expressions over datasource-level', () => { + const query = buildQuery( + { + ...basicFormData, + server_pagination: true, + metrics: [ + { + expressionType: 'SQL', + sqlExpression: 'SUM(amount)', + label: 'total', + }, + ], + }, + { + ownState: { + agGridHavingClause: 'total > 500', + metricSqlExpressions: { + total: 'SUM(old_amount)', + }, + }, + }, + ).queries[0]; + + // Query-level SUM(amount) should win over datasource-level SUM(old_amount) + expect(query.extras?.having).toBe('(SUM(amount)) > 500'); + }); + + test('should not modify clause when no labels match', () => { + const query = buildQuery( + { + ...basicFormData, + server_pagination: true, + }, + { + ownState: { + agGridComplexWhere: 'physical_column > 10', + }, + }, + ).queries[0]; + + expect(query.extras?.where).toBe('physical_column > 10'); + }); + + test('should resolve labels in both WHERE and HAVING simultaneously', () => { + const query = buildQuery( + { + ...basicFormData, + server_pagination: true, + metrics: [ + { + expressionType: 'SQL', + sqlExpression: 'SUM(sales)', + label: 'Total Sales', + }, + ], + }, + { + ownState: { + agGridComplexWhere: "region = 'West'", + agGridHavingClause: 'Total Sales > 1000', + metricSqlExpressions: { + region: "CASE WHEN area = 'W' THEN 'West' ELSE 'East' END", + }, + }, + }, + ).queries[0]; + + expect(query.extras?.where).toContain('CASE WHEN'); + expect(query.extras?.having).toBe('(SUM(sales)) > 1000'); + }); + + test('should not resolve labels when server pagination is disabled', () => { + const query = buildQuery( + { + ...basicFormData, + server_pagination: false, + metrics: [ + { + expressionType: 'SQL', + sqlExpression: 'SUM(revenue)', + label: 'Total Revenue', + }, + ], + }, + { + ownState: { + agGridComplexWhere: 'Total Revenue > 1000', + metricSqlExpressions: { + some_col: 'COUNT(*)', + }, + }, + }, + ).queries[0]; + + expect(query.extras?.where || undefined).toBeUndefined(); + }); + }); });