fix(ag-grid-table): fix AND filter conditions not applied (#38369)

2026-04-17 15:15:20 +00:00 · 2026-03-13 19:42:14 +05:30
parent f6106cd26f
commit ca2d26a1e2
5 changed files with 369 additions and 7 deletions
--- 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<D extends DataRecord = DataRecord>(
    width,
    onChartStateChange,
    chartState,
+    metricSqlExpressions,
  } = props;

  const [searchOptions, setSearchOptions] = useState<SearchOption[]>([]);
@@ -187,6 +188,7 @@ export default function TableChart<D extends DataRecord = DataRecord>(
        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<D extends DataRecord = DataRecord>(
      serverPaginationData,
      onChartStateChange,
      chartState,
+      metricSqlExpressions,
    ],
  );

--- 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<TableChartFormData> = (
        };
      }

-      // 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<string, string> = {};
+      (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<string, string>,
+        ).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<string, unknown>).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<TableChartFormData> = (
        };
      }

-      // 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<string, unknown>).agGridHavingClause =
+          resolvedHaving;
        const existingHaving = queryObject.extras?.having;
        const combinedHaving = existingHaving
-          ? `${existingHaving} AND ${ownState.agGridHavingClause}`
-          : ownState.agGridHavingClause;
+          ? `${existingHaving} AND ${resolvedHaving}`
+          : resolvedHaving;

        queryObject = {
          ...queryObject,
--- 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<number>();
+
 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<string, string> = {};
+  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,
  };
 };
--- 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<string, string>;
  onChartStateChange?: (chartState: JsonObject) => void;
  chartState?: AgGridChartState;
 }
--- 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();
+    });
+  });
 });