fix(table): Use extras in queries (#30335)

apache · Sep 19, 2024 · 6c2bd2a · 6c2bd2a
1 parent 68594d9
commit 6c2bd2a
Show file tree

Hide file tree

Showing 2 changed files with 56 additions and 9 deletions.
diff --git a/superset-frontend/plugins/plugin-chart-table/src/buildQuery.ts b/superset-frontend/plugins/plugin-chart-table/src/buildQuery.ts
@@ -114,8 +114,8 @@ const buildQuery: BuildQuery<TableChartFormData> = (
       }
     }
 
-    let temporalColumAdded = false;
-    let temporalColum = null;
+    let temporalColumnAdded = false;
+    let temporalColumn = null;
 
     if (queryMode === QueryMode.Aggregate) {
       metrics = metrics || [];
@@ -169,23 +169,23 @@ const buildQuery: BuildQuery<TableChartFormData> = (
           time_grain_sqla &&
           temporalColumnsLookup?.[col];
 
-        if (shouldBeAdded && !temporalColumAdded) {
-          temporalColum = {
+        if (shouldBeAdded && !temporalColumnAdded) {
+          temporalColumn = {
             timeGrain: time_grain_sqla,
             columnType: 'BASE_AXIS',
             sqlExpression: col,
             label: col,
             expressionType: 'SQL',
           } as AdhocColumn;
-          temporalColumAdded = true;
+          temporalColumnAdded = true;
           return false; // Do not include this in the output; it's added separately
         }
         return true;
       });
 
       // So we ensure the temporal column is added first
-      if (temporalColum) {
-        columns = [temporalColum, ...columns];
+      if (temporalColumn) {
+        columns = [temporalColumn, ...columns];
       }
     }
 
@@ -198,10 +198,15 @@ const buildQuery: BuildQuery<TableChartFormData> = (
         (ownState.currentPage ?? 0) * (ownState.pageSize ?? 0);
     }
 
+    if (!temporalColumn) {
+      // This query is not using temporal column, so it doesn't need time grain
+      extras.time_grain_sqla = undefined;
+    }
+
     let queryObject = {
       ...baseQueryObject,
       columns,
-      extras: !isEmpty(timeOffsets) && !temporalColum ? {} : extras,
+      extras,
       orderby,
       metrics,
       post_processing: postProcessing,
@@ -239,7 +244,6 @@ const buildQuery: BuildQuery<TableChartFormData> = (
         row_limit: 0,
         row_offset: 0,
         post_processing: [],
-        extras: undefined, // we don't need time grain here
         order_desc: undefined, // we don't need orderby stuff here,
         orderby: undefined, // because this query will be used for get total aggregation.
       });

diff --git a/superset-frontend/plugins/plugin-chart-table/test/buildQuery.test.ts b/superset-frontend/plugins/plugin-chart-table/test/buildQuery.test.ts
@@ -25,6 +25,28 @@ const basicFormData: TableChartFormData = {
   datasource: '11__table',
 };
 
+const extraQueryFormData: TableChartFormData = {
+  ...basicFormData,
+  time_grain_sqla: TimeGranularity.MONTH,
+  groupby: ['col1'],
+  query_mode: QueryMode.Aggregate,
+  show_totals: true,
+  metrics: ['aaa', 'aaa'],
+  adhoc_filters: [
+    {
+      expressionType: 'SQL',
+      sqlExpression: "status IN ('In Process')",
+      clause: 'WHERE',
+      subject: null,
+      operator: null,
+      comparator: null,
+      isExtra: false,
+      isNew: false,
+      datasourceWarning: false,
+      filterOptionName: 'filter_v8m9t9oq5re_ndzk6g5am7',
+    } as any,
+  ],
+};
 describe('plugin-chart-table', () => {
   describe('buildQuery', () => {
     it('should add post-processing and ignore duplicate metrics', () => {
@@ -114,5 +136,26 @@ describe('plugin-chart-table', () => {
         expressionType: 'SQL',
       });
     });
+    it('should include time_grain_sqla in extras if temporal colum is used and keep the rest', () => {
+      const { queries } = buildQuery({
+        ...extraQueryFormData,
+        temporal_columns_lookup: { col1: true },
+      });
+      // Extras in regular query
+      expect(queries[0].extras?.time_grain_sqla).toEqual(TimeGranularity.MONTH);
+      expect(queries[0].extras?.where).toEqual("(status IN ('In Process'))");
+      // Extras in summary query
+      expect(queries[1].extras?.time_grain_sqla).toEqual(TimeGranularity.MONTH);
+      expect(queries[1].extras?.where).toEqual("(status IN ('In Process'))");
+    });
+    it('should not include time_grain_sqla in extras if temporal colum is not used and keep the rest', () => {
+      const { queries } = buildQuery(extraQueryFormData);
+      // Extras in regular query
+      expect(queries[0].extras?.time_grain_sqla).toBeUndefined();
+      expect(queries[0].extras?.where).toEqual("(status IN ('In Process'))");
+      // Extras in summary query
+      expect(queries[1].extras?.time_grain_sqla).toBeUndefined();
+      expect(queries[1].extras?.where).toEqual("(status IN ('In Process'))");
+    });
   });
 });