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

lyndsiWilliams · 2022-04-26T20:50:38Z

SUMMARY

This is the front end implementation of dataset creation from an infobox in a chart.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

BEFORE:

AFTER:

TESTING INSTRUCTIONS

Open a chart with a query datasource type
Observe the new infobox on the left panel
Click "Create a dataset" and observe the modal
Close the modal and close the infobox
Refresh the page and observe that the infobox does not return
Close the browser tab containing this instance of Superset and reopen a new one
Observe that the infobox has returned on a new session

ADDITIONAL INFORMATION

Has associated issue:
Required feature flags:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- Migration is atomic, supports rollback & is backwards-compatible
- Confirm DB migration upgrade and downgrade tested
- Runtime estimates and downtime expectations provided
Introduces new feature or API
Removes existing feature or API

lyndsiWilliams · 2022-04-26T21:16:58Z

/testenv up

github-actions · 2022-04-26T21:18:47Z

@lyndsiWilliams Container image not yet published for this PR. Please try again when build is complete.

github-actions · 2022-04-26T21:18:48Z

@lyndsiWilliams Ephemeral environment creation failed. Please check the Actions logs for details.

lyndsiWilliams · 2022-04-26T21:36:47Z

/testenv up

github-actions · 2022-04-26T21:38:53Z

@lyndsiWilliams Ephemeral environment spinning up at http://54.68.5.95:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

codecov · 2022-04-26T21:50:50Z

Codecov Report

Merging #19855 (5514305) into master (6f0d53e) will decrease coverage by 0.04%.
The diff coverage is 34.01%.

@@            Coverage Diff             @@
##           master   #19855      +/-   ##
==========================================
- Coverage   66.65%   66.61%   -0.05%     
==========================================
  Files        1729     1733       +4     
  Lines       64910    64953      +43     
  Branches     6842     6858      +16     
==========================================
  Hits        43268    43268              
- Misses      19893    19929      +36     
- Partials     1749     1756       +7

Flag	Coverage Δ
javascript	`51.51% <34.01%> (-0.07%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...chart-controls/src/shared-controls/dndControls.tsx	`26.92% <0.00%> (-8.98%)`	⬇️
...d/packages/superset-ui-chart-controls/src/types.ts	`100.00% <ø> (ø)`
...egacy-plugin-chart-event-flow/src/controlPanel.tsx	`14.28% <0.00%> (ø)`
...acy-preset-chart-deckgl/src/utilities/controls.jsx	`11.11% <0.00%> (ø)`
...n-chart-handlebars/src/plugin/controls/columns.tsx	`15.00% <0.00%> (-1.67%)`	⬇️
...n-chart-handlebars/src/plugin/controls/metrics.tsx	`41.66% <0.00%> (+3.20%)`	⬆️
...n-chart-handlebars/src/plugin/controls/orderBy.tsx	`40.00% <0.00%> (-10.00%)`	⬇️
...ugin-chart-pivot-table/src/plugin/controlPanel.tsx	`8.33% <0.00%> (-0.76%)`	⬇️
...nd/plugins/plugin-chart-table/src/controlPanel.tsx	`43.47% <0.00%> (-7.27%)`	⬇️
...lLab/components/ExploreCtasResultsButton/index.tsx	`8.33% <0.00%> (ø)`
... and 26 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6f0d53e...5514305. Read the comment docs.

ghost · 2022-04-26T21:54:13Z

Thanks @lyndsiWilliams ! Two things:

The infobox shouldn't fill the full width of the sidebar (it should have the same gutter width on either side as the metrics/columns below it).
I think we should remove the line of copy in the save dataset modal if it's opened from Explore – it doesn't make as much sense outside of SQL Lab.

yousoph · 2022-04-26T22:10:46Z

When opening the save/overwrite modal from Explore, the button text should be "Save" rather than "Save & Explore"

lyndsiWilliams · 2022-04-27T00:31:37Z

/testenv up

github-actions · 2022-04-27T00:35:52Z

@lyndsiWilliams Ephemeral environment spinning up at http://50.112.74.36:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

lyndsiWilliams · 2022-04-27T00:43:57Z

Closing and reopening to reset ephemeral environment.

github-actions · 2022-04-27T00:44:19Z

Ephemeral environment shutdown and build artifacts deleted.

lyndsiWilliams · 2022-04-27T01:10:36Z

/testenv up

github-actions · 2022-04-27T01:12:39Z

@lyndsiWilliams Ephemeral environment spinning up at http://52.88.44.27:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

lyndsiWilliams · 2022-04-27T01:23:21Z

@jess-dillard @yousoph Thanks for the feedback! I implemented the changes in this commit. Please let me know how it looks and if any other changes are needed 😁

ghost · 2022-04-27T16:24:27Z

@lyndsiWilliams Looks good to me (assuming the width issue is on the ephemeral – your screenshot looks good).

hughhhh · 2022-04-27T18:44:32Z

superset-frontend/src/explore/components/DatasourcePanel/index.tsx

+  DatasetOwner,
+  DatasetOptionAutocomplete,
+  updateDataset,
+} from 'src/SqlLab/components/ResultSet';


we should centralize this in a types file now instead of pulling it from ResultSet

Can you move this to superset/superset-frontend/src/SqlLab/types.ts and import from there

Can do! Done in this commit

hughhhh · 2022-04-27T18:46:45Z

superset-frontend/src/explore/components/DatasourcePanel/index.tsx

@@ -190,6 +217,35 @@ export default function DataSourcePanel({
    [_columns],
  );

+  const placeholderSlDataset = {
+    sl_table: [],


why are we using arrays as placeholders here?

I wasn't sure what these properties were going to be but thought I needed to pass in something as a placeholder until the back end is implemented. Turns out I don't need it, so I removed it in this commit. Thanks for this catch!

hughhhh · 2022-04-27T18:47:52Z

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

@@ -65,6 +65,11 @@ export interface DatasourceMeta {
  granularity_sqla?: string;
  datasource_name: string | null;
  description: string | null;
+  sl_dataset?: {
+    sl_table?: any;


can we cast these to values we actually want them to be

Do you know what the types for these will be? I was going to figure that out once the backend came through and change it then.

@eschutho shouldn't the model look like this since sl_dataset it's own entity

sl_datasource?: { sl_dataset: any; sl_table?: any; query?: any; saved_query?: any; };

I think these values would actually be string options or enum of type

So it will look like this?

sl_datasource?: { sl_dataset: string; sl_table?: string; query?: string; saved_query?: string; };

On a query, for example, the data structure will look like datasource.query but there won't be a query on a table, so we should define them separately.

Is the schema for these types anywhere so I can describe them properly?

maybe we can just start with Query: superset-frontend/src/SqlLab/types.ts

looks like there's an interface for savedquery here: superset-frontend/src/views/CRUD/welcome/SavedQueries.tsx but don't be confused.. it's named query :)

bring this into the component:

type ExploreDatasource = Dataset | Query

Rename DatasourceMeta to Dataset

eschutho · 2022-04-27T23:08:50Z

superset-frontend/src/explore/components/DatasourcePanel/index.tsx

+  };
+
+  // eslint-disable-next-line no-param-reassign
+  datasource.sl_dataset = placeholderSlDataset;


I think the value that you're looking for is datasource.type which will come from the api.

This was the hard coded placeholder that I discussed with you the other day in Slack, but I ended up removing it in this commit because the example still works. Should I keep it as is or hard code a placeholder again?

eschutho · 2022-04-27T23:09:49Z

superset-frontend/src/explore/components/DatasourcePanel/index.tsx

+  datasource.sl_dataset = placeholderSlDataset;
+
+  const getDefaultDatasetName = () =>
+    `${datasource?.sl_dataset?.query.tab} ${moment().format(


can you just reference the datasource directly? datasource?.query.tab

There's currently no query on DatasourceMeta, should I add it?

eschutho · 2022-04-27T23:10:40Z

superset-frontend/src/explore/components/DatasourcePanel/index.tsx

+      dbId,
+      datasetToOverwrite.datasetId,
+      sql,
+      // TODO: lyndsiWilliams - Define d


what is this todo for?

I defined d as any because I'm not sure what it will be, I wanted to remind myself to define that once data is properly flowing through it and I can figure out what it is.

eschutho · 2022-04-27T23:18:22Z

superset-frontend/src/explore/components/DatasourcePanel/index.tsx

@@ -287,6 +344,175 @@ export default function DataSourcePanel({
    [lists.columns, showAllColumns],
  );

+  const handleOverwriteDataset = async () => {


can most of this functionality re saving/overwriting the dataset be moved into the dataset modal instead?

eschutho · 2022-06-03T16:22:10Z

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

-      newState.savedMetrics = state.datasource.metrics || [];
-    }
+      newState.savedMetrics = (datasource as Dataset).metrics || [];
+    } else newState.options = datasource?.columns;


@lyndsiWilliams I think we're going to need lines 47-52 applied to query columns as well. Just the filter we can skip.

Done in this commit 😁

eschutho · 2022-06-03T16:26:35Z

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

+    columns: datasource?.columns[0]?.hasOwnProperty('filterable')
+      ? (datasource as Dataset)?.columns.filter(c => c.filterable)
+      : datasource?.columns || [],
+    savedMetrics: datasource?.hasOwnProperty('metrics')


do you think we can dry up these three?

datasource?.hasOwnProperty('metrics') ? (datasource as Dataset)?.metrics || [] : DEFAULT_METRICS,

Done in this commit 😁

eschutho · 2022-06-04T00:12:05Z

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

+      newState.options = Object.fromEntries(
+        (options as QueryColumn[])?.map(option => [option.name, option]),
+      );
+      newState.options = datasource?.columns;


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

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

eschutho · 2022-06-04T00:12:29Z

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

-    columns: datasource ? datasource.columns : [],
-    savedMetrics: datasource ? datasource.metrics : [],
+    columns: datasource?.columns || [],
+    savedMetrics: savedMetricsTypeCheck(datasource),


eschutho · 2022-06-04T00:14:16Z

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

+import { Dataset } from '../types';
+import { DEFAULT_METRICS } from '..';
+
+export const savedMetricsTypeCheck = (


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.

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

eschutho · 2022-06-04T00:17:51Z

superset-frontend/src/explore/components/DatasourcePanel/index.tsx

+    return true;
+  };
+
+  const datasourceTypeCheck =


I would maybe name this something that sounds like a boolean like isValidDatasourceType

Also a good idea! I changed the naming in this commit.

eschutho · 2022-06-04T00:25:53Z

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

+    }
+
+    const temporalColumns =
+      (datasource as QueryResponse)?.columns.filter(c => c.is_dttm) ?? [];


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.

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

eschutho · 2022-06-04T00:28:43Z

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

      }
      newState.options = options;
-    }
+    } else newState.options = datasource?.columns;


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

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

…ices

eschutho

Looks good.. I just left one small suggestion. 🎉

github-actions · 2022-06-07T20:04:21Z

Ephemeral environment shutdown and build artifacts deleted.

superset-github-bot bot added the preset-io label Apr 26, 2022

pull-request-size bot added the size/L label Apr 26, 2022

superset-github-bot bot added the Superset-Community-Partners Preset community partner program participants label Apr 26, 2022

lyndsiWilliams removed the Superset-Community-Partners Preset community partner program participants label Apr 26, 2022

lyndsiWilliams closed this Apr 27, 2022

lyndsiWilliams reopened this Apr 27, 2022

hughhhh reviewed Apr 27, 2022

View reviewed changes

eschutho reviewed Apr 27, 2022

View reviewed changes

pull-request-size bot removed the size/L label May 2, 2022

eschutho reviewed Jun 3, 2022

View reviewed changes

lyndsiWilliams added 4 commits June 3, 2022 13:19

Fix logic on dndGroupByControl

ccd3e3e

Dry up savedMetrics type check

a585546

Fixed TIME_COLUMN_OPTION

5c93394

Dried savedMetrics type check even further

b9db9d2

eschutho reviewed Jun 4, 2022

View reviewed changes

lyndsiWilliams and others added 10 commits June 3, 2022 22:06

Change savedMetricsTypeCheck to defineSavedMetrics

cea65ce

Change datasourceTypeCheck to isValidDatasourceType

90c20b2

Fix Query path in groupByControl

faac391

dnd_granularity_sqla now sorts Query types with is_dttm at the top

ac671a4

Fixed/cleaned query sort

cf12f1d

Merge branch 'master' into lyndsi/create-dataset-from-infobox

55c3a79

Add sortedQueryColumns and proper optional chaining to granularity_sqla

379d9a9

Move testQuery to core-ui, add test coverage for Queries in columnCho…

23923f4

…ices

Moved DEFAULT_METRICS to core-ui and wrote a test for defineSavedMetrics

430e43b

Add license and clean dataset test object

054ee97

eschutho approved these changes Jun 7, 2022

View reviewed changes

Change DatasourceType.Dataset to dataset

5514305

lyndsiWilliams merged commit ba0c37d into apache:master Jun 7, 2022

lyndsiWilliams deleted the lyndsi/create-dataset-from-infobox branch June 7, 2022 20:03

john-bodley mentioned this pull request Jul 15, 2022

fix: Ensure main_dttm_col is adhered to for non-drag-and-drop #20712

Merged

9 tasks

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.0.0 labels Mar 13, 2024

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

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

Conversation

lyndsiWilliams commented Apr 26, 2022 • edited

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

BEFORE:

AFTER:

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

lyndsiWilliams commented Apr 26, 2022

github-actions bot commented Apr 26, 2022

github-actions bot commented Apr 26, 2022

lyndsiWilliams commented Apr 26, 2022

github-actions bot commented Apr 26, 2022

codecov bot commented Apr 26, 2022 • edited

Codecov Report

ghost commented Apr 26, 2022

yousoph commented Apr 26, 2022

lyndsiWilliams commented Apr 27, 2022

github-actions bot commented Apr 27, 2022

lyndsiWilliams commented Apr 27, 2022

github-actions bot commented Apr 27, 2022

lyndsiWilliams commented Apr 27, 2022

github-actions bot commented Apr 27, 2022

lyndsiWilliams commented Apr 27, 2022

ghost commented Apr 27, 2022 • edited by ghost

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eschutho Apr 27, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eschutho left a comment

Choose a reason for hiding this comment

github-actions bot commented Jun 7, 2022

lyndsiWilliams commented Apr 26, 2022 •

edited

codecov bot commented Apr 26, 2022 •

edited

ghost commented Apr 27, 2022 •

edited by ghost

eschutho Apr 27, 2022 •

edited