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

Conversation

lyndsiWilliams
Copy link
Member

@lyndsiWilliams lyndsiWilliams commented Apr 26, 2022

SUMMARY

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

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

BEFORE:

cdsfibBefore

AFTER:

cdsfibAfter
saveDSmodal

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

@superset-github-bot superset-github-bot bot added the Superset-Community-Partners Preset community partner program participants label Apr 26, 2022
@lyndsiWilliams lyndsiWilliams removed the Superset-Community-Partners Preset community partner program participants label Apr 26, 2022
@lyndsiWilliams
Copy link
Member Author

/testenv up

@github-actions
Copy link
Contributor

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

@github-actions
Copy link
Contributor

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

@lyndsiWilliams
Copy link
Member Author

/testenv up

@github-actions
Copy link
Contributor

@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
Copy link

codecov bot commented Apr 26, 2022

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
Copy link

ghost commented Apr 26, 2022

Thanks @lyndsiWilliams ! Two things:

  1. 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).
  2. 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.

Screen Shot 2022-04-26 at 2 50 44 PM

@yousoph
Copy link
Member

yousoph commented Apr 26, 2022

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

@lyndsiWilliams
Copy link
Member Author

/testenv up

@github-actions
Copy link
Contributor

@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
Copy link
Member Author

Closing and reopening to reset ephemeral environment.

@github-actions
Copy link
Contributor

Ephemeral environment shutdown and build artifacts deleted.

@lyndsiWilliams
Copy link
Member Author

/testenv up

@github-actions
Copy link
Contributor

@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
Copy link
Member Author

@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
Copy link

ghost commented Apr 27, 2022

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

DatasetOwner,
DatasetOptionAutocomplete,
updateDataset,
} from 'src/SqlLab/components/ResultSet';
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 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

Copy link
Member Author

Choose a reason for hiding this comment

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

Can do! Done in this commit

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

const placeholderSlDataset = {
sl_table: [],
Copy link
Member

Choose a reason for hiding this comment

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

why are we using arrays as placeholders here?

Copy link
Member Author

Choose a reason for hiding this comment

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

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!

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

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

@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;
};

Copy link
Member

@eschutho eschutho Apr 27, 2022

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

So it will look like this?

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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 :)

Copy link
Member

Choose a reason for hiding this comment

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

bring this into the component:

type ExploreDatasource =
    Dataset |
    Query 

Rename DatasourceMeta to Dataset

};

// eslint-disable-next-line no-param-reassign
datasource.sl_dataset = placeholderSlDataset;
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

datasource.sl_dataset = placeholderSlDataset;

const getDefaultDatasetName = () =>
`${datasource?.sl_dataset?.query.tab} ${moment().format(
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

dbId,
datasetToOverwrite.datasetId,
sql,
// TODO: lyndsiWilliams - Define d
Copy link
Member

Choose a reason for hiding this comment

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

what is this todo for?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

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

const handleOverwriteDataset = async () => {
Copy link
Member

Choose a reason for hiding this comment

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

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

@pull-request-size pull-request-size bot removed the size/L label May 2, 2022
newState.savedMetrics = state.datasource.metrics || [];
}
newState.savedMetrics = (datasource as Dataset).metrics || [];
} 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.

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Done in this commit 😁

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

Choose a reason for hiding this comment

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

do you think we can dry up these three?

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Done in this commit 😁

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. 👍

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

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.

return true;
};

const datasourceTypeCheck =
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

}

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.

}
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.

Copy link
Member

@eschutho eschutho left a comment

Choose a reason for hiding this comment

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

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

@lyndsiWilliams lyndsiWilliams merged commit ba0c37d into apache:master Jun 7, 2022
@lyndsiWilliams lyndsiWilliams deleted the lyndsi/create-dataset-from-infobox branch June 7, 2022 20:03
@github-actions
Copy link
Contributor

github-actions bot commented Jun 7, 2022

Ephemeral environment shutdown and build artifacts deleted.

@mistercrunch 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels preset-io size/XXL 🚢 2.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants