fix: implement extra filter logic (#688)

* fix: implement extra filter logic

* fix bugs and add tests

* remove redundant changes

* improve types

* fix coverage

* improve codevov

superset-frontend/temporary_superset_ui/superset-ui/packages/superset-ui-query/src/buildQueryObject.ts

@@ -1,18 +1,14 @@
 /* eslint-disable camelcase */
 import { QueryObject } from './types/Query';
-import { isSqlaFormData, QueryFormData } from './types/QueryFormData';
+import { QueryFormData } from './types/QueryFormData';
 import processGroupby from './processGroupby';
 import convertMetric from './convertMetric';
 import processFilters from './processFilters';
-import processExtras from './processExtras';
+import extractExtras from './extractExtras';
 import extractQueryFields from './extractQueryFields';
 
 export const DTTM_ALIAS = '__timestamp';
 
-function processGranularity(formData: QueryFormData): string {
-  return isSqlaFormData(formData) ? formData.granularity_sqla : formData.granularity;
-}
-
 /**
  * Build the common segments of all query objects (e.g. the granularity field derived from
  * either sql alchemy or druid). The segments specific to each viz type is constructed in the
@@ -31,6 +27,7 @@ export default function buildQueryObject<T extends QueryFormData>(formData: T):
     limit,
     timeseries_limit_metric,
     queryFields,
+    granularity,
     ...residualFormData
   } = formData;
 
@@ -39,9 +36,19 @@ export default function buildQueryObject<T extends QueryFormData>(formData: T):
   const { metrics, groupby, columns } = extractQueryFields(residualFormData, queryFields);
   const groupbySet = new Set([...columns, ...groupby]);
 
+  const extraFilters = extractExtras(formData);
+  const extrasAndfilters = processFilters({
+    ...formData,
+    ...extraFilters,
+  });
+
   return {
-    extras: processExtras(formData),
+    time_range,
-    granularity: processGranularity(formData),
+    since,
+    until,
+    granularity,
+    ...extraFilters,
+    ...extrasAndfilters,
     groupby: processGroupby(Array.from(groupbySet)),
     is_timeseries: groupbySet.has(DTTM_ALIAS),
     metrics: metrics.map(convertMetric),
@@ -49,13 +56,9 @@ export default function buildQueryObject<T extends QueryFormData>(formData: T):
     orderby: [],
     row_limit: row_limit == null || Number.isNaN(numericRowLimit) ? undefined : numericRowLimit,
     row_offset: row_offset == null || Number.isNaN(numericRowOffset) ? undefined : numericRowOffset,
-    since,
-    time_range,
     timeseries_limit: limit ? Number(limit) : 0,
     timeseries_limit_metric: timeseries_limit_metric
       ? convertMetric(timeseries_limit_metric)
       : null,
-    until,
-    ...processFilters(formData),
   };
 }

superset-frontend/temporary_superset_ui/superset-ui/packages/superset-ui-query/src/extractExtras.ts

@@ -0,0 +1,47 @@
+/* eslint-disable camelcase */
+import { isDruidFormData, QueryFormData } from './types/QueryFormData';
+import { QueryObject } from './types/Query';
+
+export default function extractExtras(formData: QueryFormData): Partial<QueryObject> {
+  const partialQueryObject: Partial<QueryObject> = {
+    filters: formData.filters || [],
+    extras: formData.extras || {},
+  };
+
+  const reservedColumnsToQueryField: Record<string, keyof QueryObject> = {
+    __time_range: 'time_range',
+    __time_col: 'granularity_sqla',
+    __time_grain: 'time_grain_sqla',
+    __time_origin: 'druid_time_origin',
+    __granularity: 'granularity',
+  };
+
+  (formData.extra_filters || []).forEach(filter => {
+    if (filter.col in reservedColumnsToQueryField) {
+      const queryField = reservedColumnsToQueryField[filter.col];
+      partialQueryObject[queryField] = filter.val;
+    } else {
+      // @ts-ignore
+      partialQueryObject.filters.push(filter);
+    }
+  });
+
+  // map to undeprecated names and remove deprecated fields
+  if (isDruidFormData(formData) && !partialQueryObject.druid_time_origin) {
+    partialQueryObject.extras = {
+      druid_time_origin: formData.druid_time_origin,
+    };
+    delete partialQueryObject.druid_time_origin;
+  } else {
+    // SQL
+    partialQueryObject.extras = {
+      ...partialQueryObject.extras,
+      time_grain_sqla: partialQueryObject.time_grain_sqla || formData.time_grain_sqla,
+    };
+    partialQueryObject.granularity =
+      partialQueryObject.granularity_sqla || formData.granularity || formData.granularity_sqla;
+    delete partialQueryObject.granularity_sqla;
+    delete partialQueryObject.time_grain_sqla;
+  }
+  return partialQueryObject;
+}

superset-frontend/temporary_superset_ui/superset-ui/packages/superset-ui-query/src/processExtras.ts

@@ -1,17 +0,0 @@
-/* eslint-disable camelcase */
-import { QueryFormData, isDruidFormData } from './types/QueryFormData';
-import { QueryObjectExtras } from './types/Query';
-
-export default function processExtras(formData: QueryFormData): QueryObjectExtras {
-  const { where = '' } = formData;
-
-  if (isDruidFormData(formData)) {
-    const { druid_time_origin, having_druid } = formData;
-
-    return { druid_time_origin, having_druid, where };
-  }
-
-  const { time_grain_sqla, having } = formData;
-
-  return { having, time_grain_sqla, where };
-}

superset-frontend/temporary_superset_ui/superset-ui/packages/superset-ui-query/src/processFilters.ts

@@ -1,3 +1,4 @@
+/* eslint-disable camelcase */
 import { QueryFormData } from './types/QueryFormData';
 import { QueryObjectFilterClause } from './types/Query';
 import { isSimpleAdhocFilter } from './types/Filter';
@@ -5,24 +6,17 @@ import convertFilter from './convertFilter';
 
 /** Logic formerly in viz.py's process_query_filters */
 export default function processFilters(formData: QueryFormData) {
-  // TODO: Implement
-  // utils.convert_legacy_filters_into_adhoc(self.form_data)
-
-  // TODO: Implement
-  // merge_extra_filters(self.form_data)
-
   // Split adhoc_filters into four fields according to
   // (1) clause (WHERE or HAVING)
   // (2) expressionType
   //     2.1 SIMPLE (subject + operator + comparator)
   //     2.2 SQL (freeform SQL expression))
-
-  // eslint-disable-next-line camelcase
   const { adhoc_filters } = formData;
   if (Array.isArray(adhoc_filters)) {
-    const simpleWhere: QueryObjectFilterClause[] = [];
+    const simpleWhere: QueryObjectFilterClause[] = formData.filters || [];
     const simpleHaving: QueryObjectFilterClause[] = [];
     const freeformWhere: string[] = [];
+    if (formData.where) freeformWhere.push(formData.where);
     const freeformHaving: string[] = [];
 
     adhoc_filters.forEach(filter => {
@@ -44,11 +38,18 @@ export default function processFilters(formData: QueryFormData) {
       }
     });
 
-    return {
+    // some filter-related fields need to go in `extras`
-      filters: simpleWhere,
+    const extras = {
       having: freeformHaving.map(exp => `(${exp})`).join(' AND '),
-      having_filters: simpleHaving,
+      having_druid: simpleHaving,
       where: freeformWhere.map(exp => `(${exp})`).join(' AND '),
+      ...formData.extras,
+    };
+
+    return {
+      // eslint-disable-next-line @typescript-eslint/no-unsafe-call
+      filters: (formData.filters || []).concat(simpleWhere),
+      extras,
     };
   }
 

superset-frontend/temporary_superset_ui/superset-ui/packages/superset-ui-query/src/types/Query.ts

@@ -54,7 +54,7 @@ export type QueryObject = {
   extras?: QueryObjectExtras;
 
   /** Granularity (for steps in time series) */
-  granularity: string;
+  granularity?: string;
 
   /** Free-form WHERE SQL: multiple clauses are concatenated by AND */
   where?: string;

superset-frontend/temporary_superset_ui/superset-ui/packages/superset-ui-query/src/types/QueryFormData.ts

@@ -3,6 +3,7 @@
 import { AdhocMetric } from './Metric';
 import { TimeRange } from './Time';
 import { AdhocFilter } from './Filter';
+import { BinaryOperator, SetOperator } from './Operator';
 
 export type QueryFormDataMetric = string | AdhocMetric;
 export type QueryFormResidualDataValue = string | AdhocMetric;
@@ -10,10 +11,25 @@ export type QueryFormResidualData = {
   // eslint-disable-next-line @typescript-eslint/no-explicit-any
   [key: string]: any;
 };
+
+// Currently only Binary and Set filters are supported
 export type QueryFields = {
   [key: string]: string;
 };
 
+export type QueryFormExtraFilter = {
+  col: string;
+} & (
+  | {
+      op: BinaryOperator;
+      val: string;
+    }
+  | {
+      op: SetOperator;
+      val: string[];
+    }
+);
+
 // Type signature for formData shared by all viz types
 // It will be gradually filled out as we build out the query object
 
@@ -37,6 +53,7 @@ export type BaseFormData = {
   all_columns?: string[];
   /** list of filters */
   adhoc_filters?: AdhocFilter[];
+  extra_filters?: QueryFormExtraFilter[];
   /** order descending */
   order_desc?: boolean;
   /** limit number of time series */
@@ -77,7 +94,3 @@ export type QueryFormData = SqlaFormData | DruidFormData;
 export function isDruidFormData(formData: QueryFormData): formData is DruidFormData {
   return 'granularity' in formData;
 }
-
-export function isSqlaFormData(formData: QueryFormData): formData is SqlaFormData {
-  return 'granularity_sqla' in formData;
-}

superset-frontend/temporary_superset_ui/superset-ui/packages/superset-ui-query/test/extractExtras.test.ts

@@ -0,0 +1,87 @@
+import extractExtras from '../src/extractExtras';
+
+describe('extractExtras', () => {
+  const baseQueryFormData = {
+    datasource: '1__table',
+    granularity_sqla: 'ds',
+    time_grain_sqla: 'PT1M',
+    viz_type: 'my_viz',
+    filters: [
+      {
+        col: 'gender',
+        op: '=',
+        val: 'girl',
+      },
+    ],
+  };
+
+  it('should override formData with double underscored date options', () => {
+    expect(
+      extractExtras({
+        ...baseQueryFormData,
+        extra_filters: [
+          {
+            col: '__time_col',
+            op: '=',
+            val: 'ds2',
+          },
+          {
+            col: '__time_grain',
+            op: '=',
+            val: 'PT5M',
+          },
+          {
+            col: '__time_range',
+            op: '=',
+            val: '2009-07-17T00:00:00 : 2020-07-17T00:00:00',
+          },
+        ],
+      }),
+    ).toEqual({
+      extras: {
+        time_grain_sqla: 'PT5M',
+      },
+      filters: [
+        {
+          col: 'gender',
+          op: '=',
+          val: 'girl',
+        },
+      ],
+      granularity: 'ds2',
+      time_range: '2009-07-17T00:00:00 : 2020-07-17T00:00:00',
+    });
+  });
+
+  it('should create regular filters from non-reserved columns', () => {
+    expect(
+      extractExtras({
+        ...baseQueryFormData,
+        extra_filters: [
+          {
+            col: 'name',
+            op: 'IN',
+            val: ['Eve', 'Evelyn'],
+          },
+        ],
+      }),
+    ).toEqual({
+      extras: {
+        time_grain_sqla: 'PT1M',
+      },
+      filters: [
+        {
+          col: 'gender',
+          op: '=',
+          val: 'girl',
+        },
+        {
+          col: 'name',
+          op: 'IN',
+          val: ['Eve', 'Evelyn'],
+        },
+      ],
+      granularity: 'ds',
+    });
+  });
+});

superset-frontend/temporary_superset_ui/superset-ui/packages/superset-ui-query/test/processFilters.test.ts

@@ -14,6 +14,7 @@ describe('processFilters', () => {
   it('should handle an empty array', () => {
     expect(
       processFilters({
+        where: '1 = 1',
         granularity: 'something',
         viz_type: 'custom',
         datasource: 'boba',
@@ -21,9 +22,11 @@ describe('processFilters', () => {
       }),
     ).toEqual({
       filters: [],
-      having: '',
+      extras: {
-      having_filters: [],
+        having: '',
-      where: '',
+        having_druid: [],
+        where: '(1 = 1)',
+      },
     });
   });
 
@@ -84,6 +87,22 @@ describe('processFilters', () => {
         ],
       }),
     ).toEqual({
+      extras: {
+        having: '(ice = 25 OR ice = 50) AND (waitTime <= 180)',
+        having_druid: [
+          {
+            col: 'sweetness',
+            op: '>',
+            val: '0',
+          },
+          {
+            col: 'sweetness',
+            op: '<=',
+            val: '50',
+          },
+        ],
+        where: '(tea = "jasmine") AND (cup = "large")',
+      },
       filters: [
         {
           col: 'milk',
@@ -95,20 +114,6 @@ describe('processFilters', () => {
           val: 'almond',
         },
       ],
-      having: '(ice = 25 OR ice = 50) AND (waitTime <= 180)',
-      having_filters: [
-        {
-          col: 'sweetness',
-          op: '>',
-          val: '0',
-        },
-        {
-          col: 'sweetness',
-          op: '<=',
-          val: '50',
-        },
-      ],
-      where: '(tea = "jasmine") AND (cup = "large")',
     });
   });
 });

superset-frontend/temporary_superset_ui/superset-ui/plugins/plugin-chart-word-cloud/src/plugin/buildQuery.ts

@@ -3,9 +3,11 @@ import { WordCloudFormData } from '../types';
 
 export default function buildQuery(formData: WordCloudFormData) {
   // Set the single QueryObject's groupby field with series in formData
-  return buildQueryContext(formData, baseQueryObject => [
+  return buildQueryContext(formData, baseQueryObject => {
-    {
+    return [
-      ...baseQueryObject,
+      {
-    },
+        ...baseQueryObject,
-  ]);
+      },
+    ];
+  });
 }