feat(native-filters): add timegrain and column filter (#13484)

* feat(native-filters): add timegrain and column filter * add fetch values predicate * bump deps * lint * fix test * add python test for legacy merge * fix default value and isInitialized to not check strict equality * Address comments * add FilterValue type * address review comments
apache · Mar 9, 2021 · 375797f · 375797f
1 parent c91c455
commit 375797f
Show file tree

Hide file tree

Showing 49 changed files with 1,215 additions and 546 deletions.
diff --git a/superset-frontend/package-lock.json b/superset-frontend/package-lock.json
diff --git a/superset-frontend/package.json b/superset-frontend/package.json
@@ -65,34 +65,34 @@
     "@babel/runtime-corejs3": "^7.12.5",
     "@data-ui/sparkline": "^0.0.84",
     "@emotion/core": "^10.0.35",
-    "@superset-ui/chart-controls": "^0.17.14",
-    "@superset-ui/core": "^0.17.14",
-    "@superset-ui/legacy-plugin-chart-calendar": "^0.17.14",
-    "@superset-ui/legacy-plugin-chart-chord": "^0.17.14",
-    "@superset-ui/legacy-plugin-chart-country-map": "^0.17.14",
-    "@superset-ui/legacy-plugin-chart-event-flow": "^0.17.14",
-    "@superset-ui/legacy-plugin-chart-force-directed": "^0.17.14",
-    "@superset-ui/legacy-plugin-chart-heatmap": "^0.17.14",
-    "@superset-ui/legacy-plugin-chart-histogram": "^0.17.14",
-    "@superset-ui/legacy-plugin-chart-horizon": "^0.17.14",
-    "@superset-ui/legacy-plugin-chart-map-box": "^0.17.14",
-    "@superset-ui/legacy-plugin-chart-paired-t-test": "^0.17.14",
-    "@superset-ui/legacy-plugin-chart-parallel-coordinates": "^0.17.14",
-    "@superset-ui/legacy-plugin-chart-partition": "^0.17.14",
-    "@superset-ui/legacy-plugin-chart-pivot-table": "^0.17.14",
-    "@superset-ui/legacy-plugin-chart-rose": "^0.17.14",
-    "@superset-ui/legacy-plugin-chart-sankey": "^0.17.14",
-    "@superset-ui/legacy-plugin-chart-sankey-loop": "^0.17.14",
-    "@superset-ui/legacy-plugin-chart-sunburst": "^0.17.14",
-    "@superset-ui/legacy-plugin-chart-treemap": "^0.17.14",
-    "@superset-ui/legacy-plugin-chart-world-map": "^0.17.14",
-    "@superset-ui/legacy-preset-chart-big-number": "^0.17.14",
+    "@superset-ui/chart-controls": "^0.17.15",
+    "@superset-ui/core": "^0.17.15",
+    "@superset-ui/legacy-plugin-chart-calendar": "^0.17.15",
+    "@superset-ui/legacy-plugin-chart-chord": "^0.17.15",
+    "@superset-ui/legacy-plugin-chart-country-map": "^0.17.15",
+    "@superset-ui/legacy-plugin-chart-event-flow": "^0.17.15",
+    "@superset-ui/legacy-plugin-chart-force-directed": "^0.17.15",
+    "@superset-ui/legacy-plugin-chart-heatmap": "^0.17.15",
+    "@superset-ui/legacy-plugin-chart-histogram": "^0.17.15",
+    "@superset-ui/legacy-plugin-chart-horizon": "^0.17.15",
+    "@superset-ui/legacy-plugin-chart-map-box": "^0.17.15",
+    "@superset-ui/legacy-plugin-chart-paired-t-test": "^0.17.15",
+    "@superset-ui/legacy-plugin-chart-parallel-coordinates": "^0.17.15",
+    "@superset-ui/legacy-plugin-chart-partition": "^0.17.15",
+    "@superset-ui/legacy-plugin-chart-pivot-table": "^0.17.15",
+    "@superset-ui/legacy-plugin-chart-rose": "^0.17.15",
+    "@superset-ui/legacy-plugin-chart-sankey": "^0.17.15",
+    "@superset-ui/legacy-plugin-chart-sankey-loop": "^0.17.15",
+    "@superset-ui/legacy-plugin-chart-sunburst": "^0.17.15",
+    "@superset-ui/legacy-plugin-chart-treemap": "^0.17.15",
+    "@superset-ui/legacy-plugin-chart-world-map": "^0.17.15",
+    "@superset-ui/legacy-preset-chart-big-number": "^0.17.15",
     "@superset-ui/legacy-preset-chart-deckgl": "^0.4.6",
-    "@superset-ui/legacy-preset-chart-nvd3": "^0.17.14",
-    "@superset-ui/plugin-chart-echarts": "^0.17.14",
-    "@superset-ui/plugin-chart-table": "^0.17.14",
-    "@superset-ui/plugin-chart-word-cloud": "^0.17.14",
-    "@superset-ui/preset-chart-xy": "^0.17.14",
+    "@superset-ui/legacy-preset-chart-nvd3": "^0.17.15",
+    "@superset-ui/plugin-chart-echarts": "^0.17.15",
+    "@superset-ui/plugin-chart-table": "^0.17.15",
+    "@superset-ui/plugin-chart-word-cloud": "^0.17.15",
+    "@superset-ui/preset-chart-xy": "^0.17.15",
     "@vx/responsive": "^0.0.195",
     "abortcontroller-polyfill": "^1.1.9",
     "antd": "^4.9.4",

diff --git a/superset-frontend/spec/javascripts/filters/utils_spec.ts b/superset-frontend/spec/javascripts/filters/utils_spec.ts
@@ -107,9 +107,13 @@ describe('Filter utils', () => {
       expect(getSelectExtraFormData('testCol', ['value'], true, false)).toEqual(
         {
           append_form_data: {
-            extras: {
-              where: '1 = 0',
-            },
+            adhoc_filters: [
+              {
+                clause: 'WHERE',
+                expressionType: 'SQL',
+                sqlExpression: '1 = 0',
+              },
+            ],
           },
         },
       );

diff --git a/superset-frontend/src/dashboard/components/FiltersBadge/selectors.ts b/superset-frontend/src/dashboard/components/FiltersBadge/selectors.ts
@@ -22,6 +22,7 @@ import { NativeFiltersState } from 'src/dashboard/reducers/types';
 import { DataMaskStateWithId } from 'src/dataMask/types';
 import { Layout } from '../../types';
 import { getTreeCheckedItems } from '../nativeFilters/FiltersConfigModal/FiltersConfigForm/FilterScope/utils';
+import { FilterValue } from '../nativeFilters/types';
 
 export enum IndicatorStatus {
   Unset = 'UNSET',
@@ -52,7 +53,7 @@ const selectIndicatorValue = (
   columnKey: string,
   filter: Filter,
   datasource: Datasource,
-): string[] => {
+): FilterValue => {
   const values = filter.columns[columnKey];
   const arrValues = Array.isArray(values) ? values : [values];
 
@@ -132,7 +133,7 @@ const getRejectedColumns = (chart: any): Set<string> =>
 export type Indicator = {
   column?: string;
   name: string;
-  value: string[];
+  value: FilterValue;
   status: IndicatorStatus;
   path: string[];
 };
@@ -185,20 +186,22 @@ export const selectNativeIndicatorsForChart = (
   const rejectedColumns = getRejectedColumns(chart);
 
   const getStatus = (
-    value: string[],
+    value: FilterValue,
     isAffectedByScope: boolean,
     column?: string,
   ): IndicatorStatus => {
+    // a filter is only considered unset if it's value is null
+    const hasValue = value !== null;
     if (!isAffectedByScope) {
       return IndicatorStatus.Unset;
     }
-    if (!column) {
+    if (!column && hasValue) {
       // Filter without datasource
       return IndicatorStatus.Applied;
     }
     if (column && rejectedColumns.has(column))
       return IndicatorStatus.Incompatible;
-    if (column && appliedColumns.has(column) && value.length > 0) {
+    if (column && appliedColumns.has(column) && hasValue) {
       return IndicatorStatus.Applied;
     }
     return IndicatorStatus.Unset;

diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterValue.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterValue.tsx
@@ -60,7 +60,7 @@ const FilterValue: React.FC<FilterProps> = ({
     column = {},
   }: Partial<{ datasetId: number; column: { name?: string } }> = target;
   const { name: groupby } = column;
-  const hasDataSource = !!(datasetId && groupby);
+  const hasDataSource = !!datasetId;
   const [loading, setLoading] = useState<boolean>(hasDataSource);
   useEffect(() => {
     const newFormData = getFormData({
@@ -137,7 +137,7 @@ const FilterValue: React.FC<FilterProps> = ({
         width={220}
         formData={formData}
         // For charts that don't have datasource we need workaround for empty placeholder
-        queriesData={hasDataSource ? state : [{ data: [null] }]}
+        queriesData={hasDataSource ? state : [{ data: [{}] }]}
         chartType={filterType}
         behaviors={[Behavior.NATIVE_FILTER]}
         hooks={{ setDataMask }}

diff --git a/.../dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/ControlItems.tsx b/.../dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/ControlItems.tsx
@@ -48,7 +48,6 @@ const ControlItems: FC<ControlItemsProps> = ({
   const controlPanelRegistry = getChartControlPanelRegistry();
   const controlItems =
     getControlItems(controlPanelRegistry.get(filterType)) ?? [];
-
   return (
     <>
       {controlItems

diff --git a/...board/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx b/...board/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx
@@ -78,6 +78,8 @@ export interface FiltersConfigFormProps {
   parentFilters: { id: string; title: string }[];
 }
 
+const FILTERS_WITH_ONLY_DATASOURCE = ['filter_timegrain', 'filter_timecolumn'];
+
 /**
  * The configuration form for a specific filter.
  * Assigns field values to `filters[filterId]` in the form.
@@ -104,17 +106,21 @@ export const FiltersConfigForm: React.FC<FiltersConfigFormProps> = ({
   // @ts-ignore
   const hasDatasource = !!nativeFilterItems[formFilter?.filterType]?.value
     ?.datasourceCount;
+  const hasColumn =
+    hasDatasource &&
+    !FILTERS_WITH_ONLY_DATASOURCE.includes(formFilter?.filterType);
 
   const hasFilledDatasource =
-    (formFilter?.dataset && formFilter?.column) || !hasDatasource;
+    !hasDatasource ||
+    (formFilter?.dataset?.value && (formFilter?.column || !hasColumn));
 
-  useBackendFormUpdate(form, filterId, filterToEdit, hasDatasource);
+  useBackendFormUpdate(form, filterId, filterToEdit, hasDatasource, hasColumn);
 
   const initDatasetId = filterToEdit?.targets[0]?.datasetId;
   const initColumn = filterToEdit?.targets[0]?.column?.name;
   const newFormData = getFormData({
     datasetId: formFilter?.dataset?.value,
-    groupby: formFilter?.column,
+    groupby: hasColumn ? formFilter?.column : undefined,
     defaultValue: formFilter?.defaultValue,
     ...formFilter,
   });
@@ -204,22 +210,24 @@ export const FiltersConfigForm: React.FC<FiltersConfigFormProps> = ({
               }}
             />
           </StyledFormItem>
-          <StyledFormItem
-            // don't show the column select unless we have a dataset
-            // style={{ display: datasetId == null ? undefined : 'none' }}
-            name={['filters', filterId, 'column']}
-            initialValue={initColumn}
-            label={<StyledLabel>{t('Column')}</StyledLabel>}
-            rules={[{ required: !removed, message: t('Field is required') }]}
-            data-test="field-input"
-          >
-            <ColumnSelect
-              form={form}
-              filterId={filterId}
-              datasetId={formFilter?.dataset?.value}
-              onChange={forceUpdate}
-            />
-          </StyledFormItem>
+          {hasColumn && (
+            <StyledFormItem
+              // don't show the column select unless we have a dataset
+              // style={{ display: datasetId == null ? undefined : 'none' }}
+              name={['filters', filterId, 'column']}
+              initialValue={initColumn}
+              label={<StyledLabel>{t('Column')}</StyledLabel>}
+              rules={[{ required: !removed, message: t('Field is required') }]}
+              data-test="field-input"
+            >
+              <ColumnSelect
+                form={form}
+                filterId={filterId}
+                datasetId={formFilter?.dataset?.value}
+                onChange={forceUpdate}
+              />
+            </StyledFormItem>
+          )}
         </>
       )}
       {hasFilledDatasource && (

diff --git a/...tend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/state.ts b/...tend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/state.ts
@@ -31,6 +31,7 @@ export const useBackendFormUpdate = (
   filterId: string,
   filterToEdit?: Filter,
   hasDatasource?: boolean,
+  hasColumn?: boolean,
 ) => {
   const forceUpdate = useForceUpdate();
   const formFilter = (form.getFieldValue('filters') || {})[filterId];
@@ -42,13 +43,17 @@ export const useBackendFormUpdate = (
     }
     // No need to check data set change because it cascading update column
     // So check that column exists is enough
-    if (!formFilter?.column) {
+    if (hasColumn && !formFilter?.column) {
       setFilterFieldValues(form, filterId, {
         defaultValueQueriesData: [],
         defaultValue: resolvedDefaultValue,
       });
       return;
     }
+    if (!formFilter?.dataset?.value) {
+      // no need to make chart data request if no dataset is defined
+      return;
+    }
     const formData = getFormData({
       datasetId: formFilter?.dataset?.value,
       groupby: formFilter?.column,
@@ -63,7 +68,8 @@ export const useBackendFormUpdate = (
       if (
         filterToEdit?.filterType === formFilter?.filterType &&
         filterToEdit?.targets[0].datasetId === formFilter?.dataset?.value &&
-        formFilter?.column === filterToEdit?.targets[0].column?.name
+        (!hasColumn ||
+          formFilter?.column === filterToEdit?.targets[0].column?.name)
       ) {
         resolvedDefaultValue = filterToEdit?.defaultValue;
       }

diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/utils.ts b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/utils.ts
@@ -19,7 +19,7 @@
 import { FormInstance } from 'antd/lib/form';
 import shortid from 'shortid';
 import { FilterRemoval, NativeFiltersForm } from './types';
-import { Filter, FilterConfiguration } from '../types';
+import { Filter, FilterConfiguration, Target } from '../types';
 
 export const REMOVAL_DELAY_SECS = 5;
 
@@ -132,14 +132,12 @@ export const createHandleSave = (
       const formInputs = values.filters[id];
       // if user didn't open a filter, return the original config
       if (!formInputs) return filterConfigMap[id];
-      let target = {};
+      const target: Partial<Target> = {};
+      if (formInputs.dataset) {
+        target.datasetId = formInputs.dataset.value;
+      }
       if (formInputs.dataset && formInputs.column) {
-        target = {
-          datasetId: formInputs.dataset.value,
-          column: {
-            name: formInputs.column,
-          },
-        };
+        target.column = { name: formInputs.column };
       }
       return {
         id,

diff --git a/superset-frontend/src/dashboard/components/nativeFilters/types.ts b/superset-frontend/src/dashboard/components/nativeFilters/types.ts
@@ -37,10 +37,12 @@ export interface Target {
   // clarityColumns?: Column[];
 }
 
+export type FilterValue = string | number | (string | number)[] | null;
+
 export interface Filter {
   cascadeParentIds: string[];
-  defaultValue: any;
-  currentValue?: any;
+  defaultValue: FilterValue;
+  currentValue?: FilterValue;
   isInstant: boolean;
   id: string; // randomly generated at filter creation
   name: string;

diff --git a/superset-frontend/src/dashboard/components/nativeFilters/utils.ts b/superset-frontend/src/dashboard/components/nativeFilters/utils.ts
@@ -44,12 +44,12 @@ export const getFormData = ({
   cascadingFilters?: object;
   groupby?: string;
 }): Partial<QueryFormData> => {
-  let otherProps: { datasource?: string; groupby?: string[] } = {};
-  if (datasetId && groupby) {
-    otherProps = {
-      datasource: `${datasetId}__table`,
-      groupby: [groupby],
-    };
+  const otherProps: { datasource?: string; groupby?: string[] } = {};
+  if (datasetId) {
+    otherProps.datasource = `${datasetId}__table`;
+  }
+  if (groupby) {
+    otherProps.groupby = [groupby];
   }
   return {
     ...controlValues,

diff --git a/superset-frontend/src/filters/components/Range/RangeFilterPlugin.tsx b/superset-frontend/src/filters/components/Range/RangeFilterPlugin.tsx
@@ -16,18 +16,13 @@
  * specific language governing permissions and limitations
  * under the License.
  */
-import { styled, t, DataMask, Behavior } from '@superset-ui/core';
+import { t, DataMask, Behavior } from '@superset-ui/core';
 import React, { useEffect, useState } from 'react';
 import { Slider } from 'src/common/components';
 import { PluginFilterRangeProps } from './types';
-import { PluginFilterStylesProps } from '../types';
+import { Styles } from '../common';
 import { getRangeExtraFormData } from '../../utils';
 
-const Styles = styled.div<PluginFilterStylesProps>`
-  height: ${({ height }) => height};
-  width: ${({ width }) => width};
-`;
-
 export default function RangeFilterPlugin(props: PluginFilterRangeProps) {
   const {
     data,

diff --git a/superset-frontend/src/filters/components/Range/buildQuery.ts b/superset-frontend/src/filters/components/Range/buildQuery.ts
@@ -43,6 +43,7 @@ export default function buildQuery(formData: QueryFormData) {
   return buildQueryContext(formData, baseQueryObject => [
     {
       ...baseQueryObject,
+      apply_fetch_values_predicate: true,
       columns: [],
       groupby: [],
       metrics: [

diff --git a/superset-frontend/src/filters/components/Range/index.ts b/superset-frontend/src/filters/components/Range/index.ts
@@ -26,7 +26,7 @@ export default class RangeFilterPlugin extends ChartPlugin {
   constructor() {
     const metadata = new ChartMetadata({
       name: t('Range filter'),
-      description: 'Range filter plugin using AntD',
+      description: t('Range filter plugin using AntD'),
       behaviors: [Behavior.CROSS_FILTER, Behavior.NATIVE_FILTER],
       thumbnail,
     });