feat: explicit distribute columns on BoxPlot and apply time grain (#21593)

superset-frontend/packages/superset-ui-chart-controls/src/index.ts

@@ -37,4 +37,4 @@ export { legacySortBy } from './shared-controls/legacySortBy';
 export * from './shared-controls/emitFilterControl';
 export * from './shared-controls/components';
 export * from './types';
-export { xAxisMixin, temporalColumnMixin } from './shared-controls/constants';
+export { xAxisMixin, temporalColumnMixin } from './shared-controls/mixins';

superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/dndControls.tsx

@@ -40,7 +40,7 @@ import {
   FilterOption,
   temporalColumnMixin,
 } from '..';
-import { xAxisMixin } from './constants';
+import { xAxisMixin } from './mixins';
 
 type Control = {
   savedMetrics?: Metric[] | null;

superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/mixins.tsx

@@ -20,16 +20,11 @@ import {
   FeatureFlag,
   isFeatureEnabled,
   QueryFormData,
-  QueryResponse,
   t,
   validateNonEmpty,
 } from '@superset-ui/core';
-import {
-  BaseControlConfig,
-  ControlPanelState,
-  ControlState,
-  Dataset,
-} from '../types';
+import { BaseControlConfig, ControlPanelState, ControlState } from '../types';
+import { getTemporalColumns } from '../utils';
 
 const getAxisLabel = (
   formData: QueryFormData,
@@ -60,24 +55,11 @@ export const xAxisMixin = {
 
 export const temporalColumnMixin: Pick<BaseControlConfig, 'mapStateToProps'> = {
   mapStateToProps: ({ datasource }) => {
-    if (datasource?.columns[0]?.hasOwnProperty('column_name')) {
-      const temporalColumns =
-        (datasource as Dataset)?.columns?.filter(c => c.is_dttm) ?? [];
-      return {
-        options: temporalColumns,
-        default:
-          (datasource as Dataset)?.main_dttm_col ||
-          temporalColumns[0]?.column_name ||
-          null,
-        isTemporal: true,
-      };
-    }
-    const sortedQueryColumns = (datasource as QueryResponse)?.columns?.sort(
-      query => (query?.is_dttm ? -1 : 1),
-    );
+    const payload = getTemporalColumns(datasource);
+
     return {
-      options: sortedQueryColumns,
-      default: sortedQueryColumns[0]?.name || null,
+      options: payload.temporalColumns,
+      default: payload.defaultTemporalColumn,
       isTemporal: true,
     };
   },

superset-frontend/packages/superset-ui-chart-controls/src/types.ts

@@ -24,6 +24,7 @@ import type {
   DatasourceType,
   JsonValue,
   Metric,
+  QueryColumn,
   QueryFormColumn,
   QueryFormData,
   QueryFormMetric,
@@ -449,9 +450,9 @@ export type ColorFormatters = {
 export default {};
 
 export function isColumnMeta(
-  column: AdhocColumn | ColumnMeta,
+  column: AdhocColumn | ColumnMeta | QueryColumn,
 ): column is ColumnMeta {
-  return 'column_name' in column;
+  return !!column && 'column_name' in column;
 }
 
 export function isSavedExpression(
@@ -477,9 +478,5 @@ export function isDataset(
 export function isQueryResponse(
   datasource: Dataset | QueryResponse | null | undefined,
 ): datasource is QueryResponse {
-  return (
-    !!datasource &&
-    ('results' in datasource ||
-      datasource?.type === ('query' as DatasourceType.Query))
-  );
+  return !!datasource && 'results' in datasource && 'sql' in datasource;
 }

superset-frontend/packages/superset-ui-chart-controls/src/utils/getTemporalColumns.ts

@@ -0,0 +1,64 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import {
+  ensureIsArray,
+  isDefined,
+  QueryColumn,
+  ValueOf,
+} from '@superset-ui/core';
+import {
+  ColumnMeta,
+  ControlPanelState,
+  isDataset,
+  isQueryResponse,
+} from '@superset-ui/chart-controls';
+
+export const getTemporalColumns = (
+  datasource: ValueOf<Pick<ControlPanelState, 'datasource'>>,
+) => {
+  const rv: {
+    temporalColumns: ColumnMeta[] | QueryColumn[];
+    defaultTemporalColumn: string | null | undefined;
+  } = {
+    temporalColumns: [],
+    defaultTemporalColumn: undefined,
+  };
+
+  if (isDataset(datasource)) {
+    rv.temporalColumns = ensureIsArray(datasource.columns).filter(
+      c => c.is_dttm,
+    );
+  }
+  if (isQueryResponse(datasource)) {
+    rv.temporalColumns = ensureIsArray(datasource.columns).filter(
+      c => c.is_dttm,
+    );
+  }
+
+  if (isDataset(datasource)) {
+    rv.defaultTemporalColumn = datasource.main_dttm_col;
+  }
+  if (!isDefined(rv.defaultTemporalColumn)) {
+    rv.defaultTemporalColumn =
+      (rv.temporalColumns[0] as ColumnMeta)?.column_name ??
+      (rv.temporalColumns[0] as QueryColumn)?.name;
+  }
+
+  return rv;
+};

superset-frontend/packages/superset-ui-chart-controls/src/utils/index.ts

@@ -24,3 +24,4 @@ export { default as mainMetric } from './mainMetric';
 export { default as columnChoices } from './columnChoices';
 export * from './defineSavedMetrics';
 export * from './getStandardizedControls';
+export { getTemporalColumns } from './getTemporalColumns';

superset-frontend/packages/superset-ui-chart-controls/test/fixtures.ts

@@ -0,0 +1,149 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { Dataset } from '@superset-ui/chart-controls';
+import { DatasourceType } from '@superset-ui/core';
+
+export const TestDataset: Dataset = {
+  column_format: {},
+  columns: [
+    {
+      advanced_data_type: null,
+      certification_details: null,
+      certified_by: null,
+      column_name: 'num',
+      description: null,
+      expression: '',
+      filterable: true,
+      groupby: true,
+      id: 332,
+      is_certified: false,
+      is_dttm: false,
+      python_date_format: null,
+      type: 'BIGINT',
+      type_generic: 0,
+      verbose_name: null,
+      warning_markdown: null,
+    },
+    {
+      advanced_data_type: null,
+      certification_details: null,
+      certified_by: null,
+      column_name: 'gender',
+      description: null,
+      expression: '',
+      filterable: true,
+      groupby: true,
+      id: 330,
+      is_certified: false,
+      is_dttm: false,
+      python_date_format: null,
+      type: 'VARCHAR(16)',
+      type_generic: 1,
+      verbose_name: '',
+      warning_markdown: null,
+    },
+    {
+      advanced_data_type: null,
+      certification_details: null,
+      certified_by: null,
+      column_name: 'state',
+      description: null,
+      expression: '',
+      filterable: true,
+      groupby: true,
+      id: 333,
+      is_certified: false,
+      is_dttm: false,
+      python_date_format: null,
+      type: 'VARCHAR(10)',
+      type_generic: 1,
+      verbose_name: null,
+      warning_markdown: null,
+    },
+    {
+      advanced_data_type: null,
+      certification_details: null,
+      certified_by: null,
+      column_name: 'ds',
+      description: null,
+      expression: '',
+      filterable: true,
+      groupby: true,
+      id: 329,
+      is_certified: false,
+      is_dttm: true,
+      python_date_format: null,
+      type: 'TIMESTAMP WITHOUT TIME ZONE',
+      type_generic: 2,
+      verbose_name: null,
+      warning_markdown: null,
+    },
+    {
+      advanced_data_type: null,
+      certification_details: null,
+      certified_by: null,
+      column_name: 'name',
+      description: null,
+      expression: '',
+      filterable: true,
+      groupby: true,
+      id: 331,
+      is_certified: false,
+      is_dttm: false,
+      python_date_format: null,
+      type: 'VARCHAR(255)',
+      type_generic: 1,
+      verbose_name: null,
+      warning_markdown: null,
+    },
+  ],
+  datasource_name: 'birth_names',
+  description: null,
+  granularity_sqla: 'ds',
+  id: 2,
+  main_dttm_col: 'ds',
+  metrics: [
+    {
+      certification_details: null,
+      certified_by: null,
+      d3format: null,
+      description: null,
+      expression: 'COUNT(*)',
+      id: 7,
+      is_certified: false,
+      metric_name: 'count',
+      verbose_name: 'COUNT(*)',
+      warning_markdown: '',
+      warning_text: null,
+    },
+  ],
+  name: 'public.birth_names',
+  order_by_choices: [],
+  owners: [
+    {
+      first_name: 'admin',
+      id: 1,
+      last_name: 'user',
+      username: 'admin',
+    },
+  ],
+  type: DatasourceType.Dataset,
+  uid: '2__table',
+  verbose_map: {},
+};

superset-frontend/packages/superset-ui-chart-controls/test/utils/columnChoices.test.tsx

@@ -16,7 +16,7 @@
  * specific language governing permissions and limitations
  * under the License.
  */
-import { DatasourceType, QueryResponse, testQuery } from '@superset-ui/core';
+import { DatasourceType, testQueryResponse } from '@superset-ui/core';
 import { columnChoices } from '../../src';
 
 describe('columnChoices()', () => {
@@ -58,7 +58,7 @@ describe('columnChoices()', () => {
   });
 
   it('should convert columns to choices when source is a Query', () => {
-    expect(columnChoices(testQuery as QueryResponse)).toEqual([
+    expect(columnChoices(testQueryResponse)).toEqual([
       ['Column 1', 'Column 1'],
       ['Column 2', 'Column 2'],
       ['Column 3', 'Column 3'],

superset-frontend/packages/superset-ui-chart-controls/test/utils/getTemporalColumns.test.ts

@@ -0,0 +1,95 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { testQueryResponse, testQueryResults } from '@superset-ui/core';
+import { Dataset, getTemporalColumns } from '../../src';
+import { TestDataset } from '../fixtures';
+
+test('get temporal columns from a Dataset', () => {
+  expect(getTemporalColumns(TestDataset)).toEqual({
+    temporalColumns: [
+      {
+        advanced_data_type: null,
+        certification_details: null,
+        certified_by: null,
+        column_name: 'ds',
+        description: null,
+        expression: '',
+        filterable: true,
+        groupby: true,
+        id: 329,
+        is_certified: false,
+        is_dttm: true,
+        python_date_format: null,
+        type: 'TIMESTAMP WITHOUT TIME ZONE',
+        type_generic: 2,
+        verbose_name: null,
+        warning_markdown: null,
+      },
+    ],
+    defaultTemporalColumn: 'ds',
+  });
+});
+
+test('get temporal columns from a QueryResponse', () => {
+  expect(getTemporalColumns(testQueryResponse)).toEqual({
+    temporalColumns: [
+      {
+        name: 'Column 2',
+        type: 'TIMESTAMP',
+        is_dttm: true,
+      },
+    ],
+    defaultTemporalColumn: 'Column 2',
+  });
+});
+
+test('get temporal columns from null', () => {
+  expect(getTemporalColumns(null)).toEqual({
+    temporalColumns: [],
+    defaultTemporalColumn: undefined,
+  });
+});
+
+test('should accept empty Dataset or queryResponse', () => {
+  expect(
+    getTemporalColumns({
+      ...TestDataset,
+      ...{
+        columns: [],
+        main_dttm_col: undefined,
+      },
+    } as any as Dataset),
+  ).toEqual({
+    temporalColumns: [],
+    defaultTemporalColumn: undefined,
+  });
+
+  expect(
+    getTemporalColumns({
+      ...testQueryResponse,
+      ...{
+        columns: [],
+        results: { ...testQueryResults.results, ...{ columns: [] } },
+      },
+    }),
+  ).toEqual({
+    temporalColumns: [],
+    defaultTemporalColumn: undefined,
+  });
+});

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

@@ -350,6 +350,7 @@ export type QueryResults = {
 
 export type QueryResponse = Query & QueryResults;
 
+// todo: move out from typing
 export const testQuery: Query = {
   id: 'clientId2353',
   dbId: 1,
@@ -388,22 +389,69 @@ export const testQuery: Query = {
   columns: [
     {
       name: 'Column 1',
-      type: DatasourceType.Query,
+      type: 'STRING',
       is_dttm: false,
     },
     {
       name: 'Column 3',
-      type: DatasourceType.Query,
+      type: 'STRING',
       is_dttm: false,
     },
     {
       name: 'Column 2',
-      type: DatasourceType.Query,
+      type: 'TIMESTAMP',
       is_dttm: true,
     },
   ],
 };
 
+export const testQueryResults = {
+  results: {
+    displayLimitReached: false,
+    columns: [
+      {
+        name: 'Column 1',
+        type: 'STRING',
+        is_dttm: false,
+      },
+      {
+        name: 'Column 3',
+        type: 'STRING',
+        is_dttm: false,
+      },
+      {
+        name: 'Column 2',
+        type: 'TIMESTAMP',
+        is_dttm: true,
+      },
+    ],
+    data: [
+      { 'Column 1': 'a', 'Column 2': 'b', 'Column 3': '2014-11-11T00:00:00' },
+    ],
+    expanded_columns: [],
+    selected_columns: [
+      {
+        name: 'Column 1',
+        type: 'STRING',
+        is_dttm: false,
+      },
+      {
+        name: 'Column 3',
+        type: 'STRING',
+        is_dttm: false,
+      },
+      {
+        name: 'Column 2',
+        type: 'TIMESTAMP',
+        is_dttm: true,
+      },
+    ],
+    query: { limit: 6 },
+  },
+};
+
+export const testQueryResponse = { ...testQuery, ...testQueryResults };
+
 export enum ContributionType {
   Row = 'row',
   Column = 'column',

superset-frontend/packages/superset-ui-core/src/types/index.ts

@@ -19,3 +19,5 @@
 export * from '../query/types';
 
 export type Maybe<T> = T | null;
+
+export type ValueOf<T> = T[keyof T];

superset-frontend/plugins/plugin-chart-echarts/src/BoxPlot/buildQuery.ts

@@ -16,26 +16,44 @@
  * specific language governing permissions and limitations
  * under the License.
  */
-import { buildQueryContext } from '@superset-ui/core';
+import {
+  AdhocColumn,
+  buildQueryContext,
+  ensureIsArray,
+  isPhysicalColumn,
+} from '@superset-ui/core';
 import { boxplotOperator } from '@superset-ui/chart-controls';
 import { BoxPlotQueryFormData } from './types';
 
 export default function buildQuery(formData: BoxPlotQueryFormData) {
-  const { columns = [], granularity_sqla, groupby = [] } = formData;
-  return buildQueryContext(formData, baseQueryObject => {
-    const distributionColumns: string[] = [];
-    // For now default to using the temporal column as distribution column.
-    // In the future this control should be made mandatory.
-    if (!columns.length && granularity_sqla) {
-      distributionColumns.push(granularity_sqla);
-    }
-    return [
-      {
-        ...baseQueryObject,
-        columns: [...distributionColumns, ...columns, ...groupby],
-        series_columns: groupby,
-        post_processing: [boxplotOperator(formData, baseQueryObject)],
-      },
-    ];
-  });
+  return buildQueryContext(formData, baseQueryObject => [
+    {
+      ...baseQueryObject,
+      columns: [
+        ...(ensureIsArray(formData.columns).length === 0 &&
+        formData.granularity_sqla
+          ? [formData.granularity_sqla] // for backwards compatible: if columns control is empty and granularity_sqla was set, the time columns is default distributed column.
+          : ensureIsArray(formData.columns)
+        ).map(col => {
+          if (
+            isPhysicalColumn(col) &&
+            formData.time_grain_sqla &&
+            formData?.datetime_columns_lookup?.[col]
+          ) {
+            return {
+              timeGrain: formData.time_grain_sqla,
+              columnType: 'BASE_AXIS',
+              sqlExpression: col,
+              label: col,
+              expressionType: 'SQL',
+            } as AdhocColumn;
+          }
+          return col;
+        }),
+        ...ensureIsArray(formData.groupby),
+      ],
+      series_columns: formData.groupby,
+      post_processing: [boxplotOperator(formData, baseQueryObject)],
+    },
+  ]);
 }

superset-frontend/plugins/plugin-chart-echarts/src/BoxPlot/controlPanel.ts

@@ -16,7 +16,13 @@
  * specific language governing permissions and limitations
  * under the License.
  */
-import { ensureIsArray, t } from '@superset-ui/core';
+import {
+  ensureIsArray,
+  isAdhocColumn,
+  isPhysicalColumn,
+  t,
+  validateNonEmpty,
+} from '@superset-ui/core';
 import {
   D3_FORMAT_DOCS,
   D3_FORMAT_OPTIONS,
@@ -26,20 +32,53 @@ import {
   emitFilterControl,
   ControlPanelConfig,
   getStandardizedControls,
+  ControlState,
+  ControlPanelState,
+  getTemporalColumns,
+  sharedControls,
 } from '@superset-ui/chart-controls';
 
 const config: ControlPanelConfig = {
   controlPanelSections: [
-    sections.legacyTimeseriesTime,
+    sections.legacyRegularTime,
     {
       label: t('Query'),
       expanded: true,
       controlSetRows: [
+        ['columns'],
+        [
+          {
+            name: 'time_grain_sqla',
+            config: {
+              ...sharedControls.time_grain_sqla,
+              visibility: ({ controls }) => {
+                const dttmLookup = Object.fromEntries(
+                  ensureIsArray(controls?.columns?.options).map(option => [
+                    option.column_name,
+                    option.is_dttm,
+                  ]),
+                );
+
+                return ensureIsArray(controls?.columns.value)
+                  .map(selection => {
+                    if (isAdhocColumn(selection)) {
+                      return true;
+                    }
+                    if (isPhysicalColumn(selection)) {
+                      return !!dttmLookup[selection];
+                    }
+                    return false;
+                  })
+                  .some(Boolean);
+              },
+            },
+          },
+          'datetime_columns_lookup',
+        ],
+        ['groupby'],
         ['metrics'],
         ['adhoc_filters'],
         emitFilterControl,
-        ['groupby'],
-        ['columns'], // TODO: this should be migrated to `series_columns`
         ['series_limit'],
         ['series_limit_metric'],
         [
@@ -132,9 +171,17 @@ const config: ControlPanelConfig = {
     columns: {
       label: t('Distribute across'),
       multi: true,
-      description: t(
-        'Columns to calculate distribution across. Defaults to temporal column if left empty.',
-      ),
+      description: t('Columns to calculate distribution across.'),
+      initialValue: (control: ControlState, state: ControlPanelState) => {
+        if (
+          (state && !control?.value) ||
+          (Array.isArray(control?.value) && control.value.length === 0)
+        ) {
+          return [getTemporalColumns(state.datasource).defaultTemporalColumn];
+        }
+        return control.value;
+      },
+      validators: [validateNonEmpty],
     },
   },
   formDataOverrides: formData => {