Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(explore): Frontend implementation of dataset creation from infobox #19855

Merged
Merged
Show file tree
Hide file tree
Changes from 47 commits
Commits
Show all changes
58 commits
Select commit Hold shift + click to select a range
6487ee4
Frontend implementation of create dataset from infobox
lyndsiWilliams Apr 26, 2022
2675ccb
Fixed sl_dataset type
lyndsiWilliams Apr 26, 2022
0bbb69f
Fix test
lyndsiWilliams Apr 26, 2022
a5688f5
Fixed sl_dataset type (forgot to save)
lyndsiWilliams Apr 26, 2022
8a3e4aa
RTL testing
lyndsiWilliams Apr 26, 2022
3e1f913
Adjusted styling/text on infobox and save dataset modal
lyndsiWilliams Apr 26, 2022
d4afdfe
Appease lint
lyndsiWilliams Apr 26, 2022
c32412b
Make infobox invisible and fix tests
lyndsiWilliams Apr 27, 2022
83d2173
Remove unnecessary placeholder
lyndsiWilliams Apr 27, 2022
3430551
Move types to sql lab
lyndsiWilliams Apr 27, 2022
2bdd51a
Moved logic into save dataset modal
lyndsiWilliams May 2, 2022
fe7085a
Change DatasourceMeta type to Dataset
lyndsiWilliams May 4, 2022
29f74e2
Add ExploreDatasource union type to save dataset modal
lyndsiWilliams May 4, 2022
7aeba6b
Get user info from redux inside save dataset modal
lyndsiWilliams May 4, 2022
1d49ef5
Addressed comments
lyndsiWilliams May 11, 2022
4c35710
Adjusting to new query type
lyndsiWilliams May 13, 2022
ff63e6a
Fixed save dataset in explore and union type
lyndsiWilliams May 17, 2022
d28edf4
Added testing
lyndsiWilliams May 17, 2022
5ec5eed
Defined d for queries
lyndsiWilliams May 17, 2022
65da6ac
Remove dataset from SaveDatasetModal
lyndsiWilliams May 17, 2022
2ef4ad4
Clarify useSelector parameter
lyndsiWilliams May 17, 2022
1729fe7
Fix dndControls union type
lyndsiWilliams May 18, 2022
8a45aef
Fix shared-controls union type
lyndsiWilliams May 18, 2022
e4fc404
Fix controlPanel union type
lyndsiWilliams May 18, 2022
106609a
Move ExploreRootState to explore type file
lyndsiWilliams May 19, 2022
4028043
Remove unnecessary testing playground
lyndsiWilliams May 19, 2022
a9212ff
Move datasource type check in DatasourcePanel to a function
lyndsiWilliams May 19, 2022
eab9fc0
Make all sqllab Query imports reference @superset-ui/core Query type
lyndsiWilliams May 19, 2022
9b14992
Deconstruct query props in ResultSet
lyndsiWilliams May 19, 2022
8558460
Fix union type in /legacy-plugin-chart-heatmap/src/controlPanel
lyndsiWilliams May 19, 2022
c1fb8a6
Change SaveDatasetModal tests to RTL
lyndsiWilliams May 19, 2022
9d225c4
Cleaned datasourceTypeCheck
lyndsiWilliams May 19, 2022
383ec2e
Fix infobox styling
lyndsiWilliams May 20, 2022
a0c771b
Fix SaveDatasetModal test
lyndsiWilliams May 20, 2022
bc38438
Fix query fixture in sqllab and Query type in SaveDatasetModal test
lyndsiWilliams May 20, 2022
91f06ea
Fix Query type and make test query fixture
lyndsiWilliams May 20, 2022
55bb379
Added columns to Query type, separated results property, created Quer…
lyndsiWilliams May 26, 2022
dab90b4
Fixed a couple missed broken types
lyndsiWilliams May 26, 2022
c560456
Added ExploreDatasource to SqlLab type file
lyndsiWilliams May 26, 2022
30f4508
Removed unneeded Query import from DatasourcePanel
lyndsiWilliams May 26, 2022
6ee52ea
Address PR comments
lyndsiWilliams Jun 2, 2022
1773954
Fix columnChoices
lyndsiWilliams Jun 2, 2022
20ede6d
Fix all incorrect column property checks
lyndsiWilliams Jun 2, 2022
ccd3e3e
Fix logic on dndGroupByControl
lyndsiWilliams Jun 3, 2022
a585546
Dry up savedMetrics type check
lyndsiWilliams Jun 3, 2022
5c93394
Fixed TIME_COLUMN_OPTION
lyndsiWilliams Jun 3, 2022
b9db9d2
Dried savedMetrics type check even further
lyndsiWilliams Jun 3, 2022
cea65ce
Change savedMetricsTypeCheck to defineSavedMetrics
lyndsiWilliams Jun 4, 2022
90c20b2
Change datasourceTypeCheck to isValidDatasourceType
lyndsiWilliams Jun 4, 2022
faac391
Fix Query path in groupByControl
lyndsiWilliams Jun 4, 2022
ac671a4
dnd_granularity_sqla now sorts Query types with is_dttm at the top
lyndsiWilliams Jun 4, 2022
cf12f1d
Fixed/cleaned query sort
lyndsiWilliams Jun 6, 2022
55c3a79
Merge branch 'master' into lyndsi/create-dataset-from-infobox
lyndsiWilliams Jun 6, 2022
379d9a9
Add sortedQueryColumns and proper optional chaining to granularity_sqla
lyndsiWilliams Jun 6, 2022
23923f4
Move testQuery to core-ui, add test coverage for Queries in columnCho…
lyndsiWilliams Jun 7, 2022
430e43b
Moved DEFAULT_METRICS to core-ui and wrote a test for defineSavedMetrics
lyndsiWilliams Jun 7, 2022
054ee97
Add license and clean dataset test object
lyndsiWilliams Jun 7, 2022
5514305
Change DatasourceType.Dataset to dataset
lyndsiWilliams Jun 7, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,14 @@
* specific language governing permissions and limitations
* under the License.
*/
import { t, QueryMode, DTTM_ALIAS, GenericDataType } from '@superset-ui/core';
import {
t,
QueryMode,
DTTM_ALIAS,
GenericDataType,
QueryColumn,
DatasourceType,
} from '@superset-ui/core';
import { ColumnMeta } from './types';

// eslint-disable-next-line import/prefer-default-export
Expand All @@ -32,7 +39,7 @@ export const COLUMN_NAME_ALIASES: Record<string, string> = {
[DTTM_ALIAS]: t('Time'),
};

export const TIME_COLUMN_OPTION: ColumnMeta = {
export const DATASET_TIME_COLUMN_OPTION: ColumnMeta = {
verbose_name: COLUMN_NAME_ALIASES[DTTM_ALIAS],
column_name: DTTM_ALIAS,
type_generic: GenericDataType.TEMPORAL,
Expand All @@ -41,7 +48,20 @@ export const TIME_COLUMN_OPTION: ColumnMeta = {
),
};

export const QUERY_TIME_COLUMN_OPTION: QueryColumn = {
name: DTTM_ALIAS,
type: DatasourceType.Query,
is_dttm: false,
};

export const QueryModeLabel = {
[QueryMode.aggregate]: t('Aggregate'),
[QueryMode.raw]: t('Raw records'),
};

export const DEFAULT_METRICS = [
{
metric_name: 'COUNT(*)',
expression: 'COUNT(*)',
},
];
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,14 @@
import {
FeatureFlag,
isFeatureEnabled,
QueryColumn,
QueryResponse,
t,
validateNonEmpty,
} from '@superset-ui/core';
import { ExtraControlProps, SharedControlConfig } from '../types';
import { TIME_COLUMN_OPTION, TIME_FILTER_LABELS } from '../constants';
import { ExtraControlProps, SharedControlConfig, Dataset } from '../types';
import { DATASET_TIME_COLUMN_OPTION, TIME_FILTER_LABELS } from '../constants';
import { QUERY_TIME_COLUMN_OPTION, savedMetricsTypeCheck } from '..';

export const dndGroupByControl: SharedControlConfig<'DndColumnSelect'> = {
type: 'DndColumnSelect',
Expand All @@ -36,15 +39,25 @@ export const dndGroupByControl: SharedControlConfig<'DndColumnSelect'> = {
),
mapStateToProps(state, { includeTime }) {
const newState: ExtraControlProps = {};
if (state.datasource) {
const options = state.datasource.columns.filter(c => c.groupby);
const { datasource } = state;
if (datasource?.columns[0]?.hasOwnProperty('groupby')) {
const options = (datasource as Dataset).columns.filter(c => c.groupby);
if (includeTime) {
options.unshift(TIME_COLUMN_OPTION);
options.unshift(DATASET_TIME_COLUMN_OPTION);
}
newState.options = Object.fromEntries(
options.map(option => [option.column_name, option]),
);
newState.savedMetrics = state.datasource.metrics || [];
newState.savedMetrics = (datasource as Dataset).metrics || [];
} else {
const options = datasource?.columns;
if (includeTime) {
(options as QueryColumn[])?.unshift(QUERY_TIME_COLUMN_OPTION);
}
newState.options = Object.fromEntries(
(options as QueryColumn[])?.map(option => [option.name, option]),
);
newState.options = datasource?.columns;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lyndsiWilliams do you think you can dry up lines 45-60 more?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually, looks like we'll have to wait for the column name property to be the same first. 👍

}
return newState;
},
Expand Down Expand Up @@ -83,8 +96,10 @@ export const dnd_adhoc_filters: SharedControlConfig<'DndFilterSelect'> = {
default: [],
description: '',
mapStateToProps: ({ datasource, form_data }) => ({
columns: datasource?.columns.filter(c => c.filterable) || [],
savedMetrics: datasource?.metrics || [],
columns: datasource?.columns[0]?.hasOwnProperty('filterable')
? (datasource as Dataset)?.columns.filter(c => c.filterable)
: datasource?.columns || [],
savedMetrics: savedMetricsTypeCheck(datasource),
// current active adhoc metrics
selectedMetrics:
form_data.metrics || (form_data.metric ? [form_data.metric] : []),
Expand All @@ -99,8 +114,8 @@ export const dnd_adhoc_metrics: SharedControlConfig<'DndMetricSelect'> = {
label: t('Metrics'),
validators: [validateNonEmpty],
mapStateToProps: ({ datasource }) => ({
columns: datasource ? datasource.columns : [],
savedMetrics: datasource ? datasource.metrics : [],
columns: datasource?.columns || [],
savedMetrics: savedMetricsTypeCheck(datasource),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice

datasource,
datasourceType: datasource?.type,
}),
Expand Down Expand Up @@ -130,7 +145,7 @@ export const dnd_sort_by: SharedControlConfig<'DndMetricSelect'> = {
),
mapStateToProps: ({ datasource }) => ({
columns: datasource?.columns || [],
savedMetrics: datasource?.metrics || [],
savedMetrics: savedMetricsTypeCheck(datasource),
datasource,
datasourceType: datasource?.type,
}),
Expand Down Expand Up @@ -178,14 +193,30 @@ export const dnd_granularity_sqla: typeof dndGroupByControl = {
: 'Drop temporal column here',
),
mapStateToProps: ({ datasource }) => {
const temporalColumns = datasource?.columns.filter(c => c.is_dttm) ?? [];
if (datasource?.columns[0]?.hasOwnProperty('column_name')) {
const temporalColumns =
(datasource as Dataset)?.columns.filter(c => c.is_dttm) ?? [];
const options = Object.fromEntries(
temporalColumns.map(option => [option.column_name, option]),
);
return {
options,
default:
(datasource as Dataset)?.main_dttm_col ||
temporalColumns[0]?.column_name ||
null,
isTemporal: true,
};
}

const temporalColumns =
(datasource as QueryResponse)?.columns.filter(c => c.is_dttm) ?? [];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the one where let's show all the columns but put the is_dttm ones at the top, since users can't turn on/ off the is_dttm property.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ohhh yes that's right. I added a sort for this in this commit.

const options = Object.fromEntries(
temporalColumns.map(option => [option.column_name, option]),
temporalColumns.map(option => [option.name, option]),
);
return {
options,
default:
datasource?.main_dttm_col || temporalColumns[0]?.column_name || null,
default: temporalColumns[0]?.name || null,
isTemporal: true,
};
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,8 @@ import {
legacyValidateInteger,
validateNonEmpty,
ComparisionType,
QueryResponse,
QueryColumn,
} from '@superset-ui/core';

import {
Expand All @@ -55,14 +57,16 @@ import {
D3_TIME_FORMAT_DOCS,
DEFAULT_TIME_FORMAT,
DEFAULT_NUMBER_FORMAT,
savedMetricsTypeCheck,
} from '../utils';
import { TIME_FILTER_LABELS, TIME_COLUMN_OPTION } from '../constants';
import { TIME_FILTER_LABELS, DATASET_TIME_COLUMN_OPTION } from '../constants';
import {
Metric,
SharedControlConfig,
ColumnMeta,
ExtraControlProps,
SelectControlConfig,
Dataset,
} from '../types';
import { ColumnOption } from '../components/ColumnOption';

Expand Down Expand Up @@ -131,13 +135,14 @@ const groupByControl: SharedControlConfig<'SelectControl', ColumnMeta> = {
promptTextCreator: (label: unknown) => label,
mapStateToProps(state, { includeTime }) {
const newState: ExtraControlProps = {};
if (state.datasource) {
const options = state.datasource.columns.filter(c => c.groupby);
const { datasource } = state;
if (datasource?.columns[0]?.hasOwnProperty('groupby')) {
const options = (datasource as Dataset).columns.filter(c => c.groupby);
if (includeTime) {
options.unshift(TIME_COLUMN_OPTION);
options.unshift(DATASET_TIME_COLUMN_OPTION);
}
newState.options = options;
}
} else newState.options = datasource?.columns;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should still do lines 141 and 142 if this is a query.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh good catch, thank you! Fixed in this commit.

return newState;
},
commaChoosesOption: false,
Expand All @@ -149,8 +154,8 @@ const metrics: SharedControlConfig<'MetricsControl'> = {
label: t('Metrics'),
validators: [validateNonEmpty],
mapStateToProps: ({ datasource }) => ({
columns: datasource ? datasource.columns : [],
savedMetrics: datasource ? datasource.metrics : [],
columns: datasource?.columns || [],
savedMetrics: savedMetricsTypeCheck(datasource),
datasource,
datasourceType: datasource?.type,
}),
Expand Down Expand Up @@ -292,15 +297,21 @@ const granularity_sqla: SharedControlConfig<'SelectControl', ColumnMeta> = {
valueRenderer: c => <ColumnOption column={c} />,
valueKey: 'column_name',
mapStateToProps: state => {
const props: Partial<SelectControlConfig<ColumnMeta>> = {};
if (state.datasource) {
props.options = state.datasource.columns.filter(c => c.is_dttm);
const props: Partial<SelectControlConfig<ColumnMeta | QueryColumn>> = {};
const { datasource } = state;
const dataset = datasource as Dataset;
const query = datasource as QueryResponse;
if (datasource?.columns[0]?.hasOwnProperty('main_dttm_col')) {
props.options = dataset.columns.filter((c: ColumnMeta) => c.is_dttm);
props.default = null;
if (state.datasource.main_dttm_col) {
props.default = state.datasource.main_dttm_col;
} else if (props.options && props.options.length > 0) {
props.default = props.options[0].column_name;
if (dataset.main_dttm_col) {
props.default = dataset.main_dttm_col;
} else if (props?.options) {
props.default = (props.options[0] as ColumnMeta).column_name;
}
} else {
props.options = query?.columns.filter((c: QueryColumn) => c.is_dttm);
if (props?.options) props.default = props.options[0].name;
}
return props;
},
Expand All @@ -318,7 +329,7 @@ const time_grain_sqla: SharedControlConfig<'SelectControl'> = {
'engine basis in the Superset source code.',
),
mapStateToProps: ({ datasource }) => ({
choices: datasource?.time_grain_sqla || null,
choices: (datasource as Dataset)?.time_grain_sqla || null,
}),
};

Expand All @@ -335,7 +346,7 @@ const time_range: SharedControlConfig<'DateFilterControl'> = {
"using the engine's local timezone. Note one can explicitly set the timezone " +
'per the ISO 8601 format if specifying either the start and/or end time.',
),
mapStateToProps: ({ datasource, form_data }) => ({
mapStateToProps: ({ datasource }) => ({
datasource,
}),
};
Expand Down Expand Up @@ -401,7 +412,7 @@ const sort_by: SharedControlConfig<'MetricsControl'> = {
),
mapStateToProps: ({ datasource }) => ({
columns: datasource?.columns || [],
savedMetrics: datasource?.metrics || [],
savedMetrics: savedMetricsTypeCheck(datasource),
datasource,
datasourceType: datasource?.type,
}),
Expand Down Expand Up @@ -493,8 +504,10 @@ const adhoc_filters: SharedControlConfig<'AdhocFilterControl'> = {
default: [],
description: '',
mapStateToProps: ({ datasource, form_data }) => ({
columns: datasource?.columns.filter(c => c.filterable) || [],
savedMetrics: datasource?.metrics || [],
columns: datasource?.columns[0]?.hasOwnProperty('filterable')
? (datasource as Dataset)?.columns.filter(c => c.filterable)
: datasource?.columns || [],
savedMetrics: savedMetricsTypeCheck(datasource),
// current active adhoc metrics
selectedMetrics:
form_data.metrics || (form_data.metric ? [form_data.metric] : []),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import type {
JsonValue,
Metric,
QueryFormData,
QueryResponse,
} from '@superset-ui/core';
import { sharedControls } from './shared-controls';
import sharedControlComponents from './shared-controls/components';
Expand All @@ -51,7 +52,7 @@ export type ColumnMeta = Omit<Column, 'id'> & {
id?: number;
} & AnyDict;

export interface DatasourceMeta {
export interface Dataset {
id: number;
type: DatasourceType;
columns: ColumnMeta[];
Expand All @@ -69,7 +70,7 @@ export interface DatasourceMeta {

export interface ControlPanelState {
form_data: QueryFormData;
datasource: DatasourceMeta | null;
datasource: Dataset | QueryResponse | null;
controls: ControlStateMapping;
}

Expand All @@ -88,7 +89,7 @@ export interface ActionDispatcher<
* Mapping of action dispatchers
*/
export interface ControlPanelActionDispatchers {
setDatasource: ActionDispatcher<[DatasourceMeta]>;
setDatasource: ActionDispatcher<[Dataset]>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will need to be Query too in the future

}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,20 +16,30 @@
* specific language governing permissions and limitations
* under the License.
*/
import { DatasourceMeta } from '../types';
import { QueryResponse } from '@superset-ui/core';
import { Dataset } from '../types';

/**
* Convert Datasource columns to column choices
*/
export default function columnChoices(
datasource?: DatasourceMeta | null,
datasource?: Dataset | QueryResponse | null,
): [string, string][] {
if (datasource?.columns[0]?.hasOwnProperty('column_name')) {
return (
(datasource as Dataset)?.columns
.map((col): [string, string] => [
col.column_name,
col.verbose_name || col.column_name,
])
.sort((opt1, opt2) =>
opt1[1].toLowerCase() > opt2[1].toLowerCase() ? 1 : -1,
) || []
);
}
return (
datasource?.columns
.map((col): [string, string] => [
col.column_name,
col.verbose_name || col.column_name,
])
(datasource as QueryResponse)?.columns
.map((col): [string, string] => [col.name, col.name])
.sort((opt1, opt2) =>
opt1[1].toLowerCase() > opt2[1].toLowerCase() ? 1 : -1,
) || []
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,3 +22,4 @@ export * from './expandControlConfig';
export * from './getColorFormatters';
export { default as mainMetric } from './mainMetric';
export { default as columnChoices } from './columnChoices';
export * from './savedMetricsTypeCheck';
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
/* eslint-disable camelcase */
/**
* 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 { QueryResponse } from '@superset-ui/core';
import { Dataset } from '../types';
import { DEFAULT_METRICS } from '..';

export const savedMetricsTypeCheck = (
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to be more specific, I would say that this function is either fetching/defining or creating metrics. The type checking is just a side effect, if you wanted to rename this to something that explains what it does a little more succinctly.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea! I changed it to defineSavedMetrics in this commit.

datasource: Dataset | QueryResponse | null,
) =>
datasource?.hasOwnProperty('metrics')
? (datasource as Dataset)?.metrics || []
: DEFAULT_METRICS;