You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2022/11/16 01:04:55 UTC
[GitHub] [superset] lyndsiWilliams opened a new pull request, #22136: feat: Flow for tables that already have a dataset
lyndsiWilliams opened a new pull request, #22136:
URL: https://github.com/apache/superset/pull/22136
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
This implements a flow in the new dataset creation page for when a table already has a dataset. It uses a simplified dataset fetch in the `AddDataset/index.tsx` file to cross-reference with the table list. If the table has a dataset, it gets a warning icon in the left panel. If the table is selected, the table's columns are still displayed, but the "Create Dataset" button is disabled and it has an info banner at the top informing the user of the pre-existing dataset with a link to that dataset in the upper-left corner of the alert. Clicking the "View Dataset" button in this alert will bring the user to the explore view of the existing dataset in a new tab.
### ANIMATED GIF / SCREENSHOT
<!--- Skip this if not applicable -->
#### Screenshot of selected table with existing dataset
<img width="1388" alt="Screenshot 2022-11-15 at 7 01 47 PM" src="https://user-images.githubusercontent.com/55605634/202057625-a50027ee-bd71-483f-a32e-b3cebfa83990.png">
#### Left panel warning icons
#### "View dataset"
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
- Go to `http://localhost:9000/dataset/add/?testing`
- Select a database and a schema
- Observe that any tables in the left panel with a pre-existing dataset will have a warning icon
- Click a table with a dataset
- Observe that the "Create dataset" button is disabled and there is an info banner at the top informing of the pre-existing dataset
- Click "View dataset" in the alert
- Observe that you are taken to the explore view of the existing dataset in a new tab
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] lyndsiWilliams commented on a diff in pull request #22136: feat: Flow for tables that already have a dataset
Posted by GitBox <gi...@apache.org>.
lyndsiWilliams commented on code in PR #22136:
URL: https://github.com/apache/superset/pull/22136#discussion_r1028719410
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/index.tsx:
##########
@@ -73,6 +75,38 @@ export default function AddDataset() {
Reducer<Partial<DatasetObject> | null, DSReducerActionType>
>(datasetReducer, null);
const [hasColumns, setHasColumns] = useState(false);
+ const [linkedDatasets, setLinkedDatasets] = useState<DatasetObject[]>([]);
Review Comment:
Changed all instances of `linkedDatasets` to `datasets` in [`this commit`](https://github.com/apache/superset/pull/22136/commits/ad13ff4dbb6c72d647397d3b2d106c6fc3dbbe2c).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on a diff in pull request #22136: feat: Flow for tables that already have a dataset
Posted by GitBox <gi...@apache.org>.
eschutho commented on code in PR #22136:
URL: https://github.com/apache/superset/pull/22136#discussion_r1025857796
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/index.tsx:
##########
@@ -73,6 +75,38 @@ export default function AddDataset() {
Reducer<Partial<DatasetObject> | null, DSReducerActionType>
>(datasetReducer, null);
const [hasColumns, setHasColumns] = useState(false);
+ const [linkedDatasets, setLinkedDatasets] = useState<DatasetObject[]>([]);
+ const linkedDatasetNames = linkedDatasets.map(dataset => dataset.table_name);
+ const encodedSchema = dataset?.schema
+ ? encodeURIComponent(dataset?.schema)
+ : undefined;
+
+ const queryParams = dataset?.schema
+ ? rison.encode_uri({
+ filters: [
+ { col: 'schema', opr: 'eq', value: encodedSchema },
+ { col: 'sql', opr: 'dataset_is_null_or_empty', value: '!t' },
+ ],
+ })
+ : undefined;
+
+ const getDatasetsList = () => {
+ SupersetClient.get({
+ endpoint: `/api/v1/dataset/?q=${queryParams}`,
+ })
+ .then(({ json }) => {
+ json.result.map((dataset: any) =>
+ setLinkedDatasets(linkedDatasets => [...linkedDatasets, dataset]),
Review Comment:
I'm not understanding this part here.. what's the difference between looping over the json result and adding the datasets to the list as opposed to just setting linkedDatasets to the entire response?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on a diff in pull request #22136: feat: Flow for tables that already have a dataset
Posted by GitBox <gi...@apache.org>.
eschutho commented on code in PR #22136:
URL: https://github.com/apache/superset/pull/22136#discussion_r1028819508
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/index.tsx:
##########
@@ -73,6 +75,38 @@ export default function AddDataset() {
Reducer<Partial<DatasetObject> | null, DSReducerActionType>
>(datasetReducer, null);
const [hasColumns, setHasColumns] = useState(false);
+ const [linkedDatasets, setLinkedDatasets] = useState<DatasetObject[]>([]);
+ const linkedDatasetNames = linkedDatasets.map(dataset => dataset.table_name);
+ const encodedSchema = dataset?.schema
+ ? encodeURIComponent(dataset?.schema)
+ : undefined;
+
+ const queryParams = dataset?.schema
+ ? rison.encode_uri({
+ filters: [
+ { col: 'schema', opr: 'eq', value: encodedSchema },
+ { col: 'sql', opr: 'dataset_is_null_or_empty', value: '!t' },
+ ],
+ })
+ : undefined;
+
+ const getDatasetsList = () => {
+ SupersetClient.get({
+ endpoint: `/api/v1/dataset/?q=${queryParams}`,
+ })
+ .then(({ json }) => {
+ json.result.map((dataset: any) =>
+ setLinkedDatasets(linkedDatasets => [...linkedDatasets, dataset]),
Review Comment:
Are you adding to the existing list of datasets or creating a new list?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] lyndsiWilliams merged pull request #22136: feat: Flow for tables that already have a dataset
Posted by GitBox <gi...@apache.org>.
lyndsiWilliams merged PR #22136:
URL: https://github.com/apache/superset/pull/22136
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eric-briscoe commented on a diff in pull request #22136: feat: Flow for tables that already have a dataset
Posted by GitBox <gi...@apache.org>.
eric-briscoe commented on code in PR #22136:
URL: https://github.com/apache/superset/pull/22136#discussion_r1025421125
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/DatasetPanel/DatasetPanel.tsx:
##########
@@ -54,8 +56,10 @@ const MARGIN_MULTIPLIER = 3;
const StyledHeader = styled.div<StyledHeaderProps>`
position: ${(props: StyledHeaderProps) => props.position};
- margin: ${({ theme }) => theme.gridUnit * MARGIN_MULTIPLIER}px;
- margin-top: ${({ theme }) => theme.gridUnit * (MARGIN_MULTIPLIER + 1)}px;
+ margin: ${({ theme }) => theme.gridUnit * (MARGIN_MULTIPLIER + 1)}px
+ ${({ theme }) => theme.gridUnit * MARGIN_MULTIPLIER}px
+ ${({ theme }) => theme.gridUnit * MARGIN_MULTIPLIER}px
+ ${({ theme }) => theme.gridUnit * (MARGIN_MULTIPLIER + 3)}px;
Review Comment:
These docs address how you can get props once and not have to make multiple inline functions: https://github.com/apache/superset/wiki/Emotion-Styling-Guidelines-and-Best-Practices#no-need-to-use-theme-the-hard-way
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on a diff in pull request #22136: feat: Flow for tables that already have a dataset
Posted by GitBox <gi...@apache.org>.
eschutho commented on code in PR #22136:
URL: https://github.com/apache/superset/pull/22136#discussion_r1025854683
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/index.tsx:
##########
@@ -73,6 +75,38 @@ export default function AddDataset() {
Reducer<Partial<DatasetObject> | null, DSReducerActionType>
>(datasetReducer, null);
const [hasColumns, setHasColumns] = useState(false);
+ const [linkedDatasets, setLinkedDatasets] = useState<DatasetObject[]>([]);
+ const linkedDatasetNames = linkedDatasets.map(dataset => dataset.table_name);
+ const encodedSchema = dataset?.schema
+ ? encodeURIComponent(dataset?.schema)
+ : undefined;
+
+ const queryParams = dataset?.schema
+ ? rison.encode_uri({
+ filters: [
+ { col: 'schema', opr: 'eq', value: encodedSchema },
+ { col: 'sql', opr: 'dataset_is_null_or_empty', value: '!t' },
+ ],
+ })
+ : undefined;
+
+ const getDatasetsList = () => {
+ SupersetClient.get({
+ endpoint: `/api/v1/dataset/?q=${queryParams}`,
+ })
+ .then(({ json }) => {
+ json.result.map((dataset: any) =>
+ setLinkedDatasets(linkedDatasets => [...linkedDatasets, dataset]),
+ );
+ })
+ .catch(error =>
+ console.log('There was an error fetching dataset', error),
+ );
+ };
+
+ useEffect(() => {
+ if (dataset?.schema) getDatasetsList();
Review Comment:
agree on the lint rule as a follow up +1
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] github-actions[bot] commented on pull request #22136: feat: Flow for tables that already have a dataset
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #22136:
URL: https://github.com/apache/superset/pull/22136#issuecomment-1338216502
Ephemeral environment shutdown and build artifacts deleted.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] EugeneTorap commented on pull request #22136: feat: Flow for tables that already have a dataset
Posted by GitBox <gi...@apache.org>.
EugeneTorap commented on PR #22136:
URL: https://github.com/apache/superset/pull/22136#issuecomment-1316364227
Hi @lyndsiWilliams!
Do we plan to add the ability to create a virtual dataset based on a SQL query to the DatasetPanel?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] lyndsiWilliams commented on a diff in pull request #22136: feat: Flow for tables that already have a dataset
Posted by GitBox <gi...@apache.org>.
lyndsiWilliams commented on code in PR #22136:
URL: https://github.com/apache/superset/pull/22136#discussion_r1028718843
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/DatasetPanel/DatasetPanel.tsx:
##########
@@ -217,17 +273,23 @@ const DatasetPanel = ({
return (
<>
{tableName && (
- <StyledHeader
- position={
- !loading && hasColumns ? EPosition.RELATIVE : EPosition.ABSOLUTE
- }
- title={tableName || ''}
- >
- {tableName && (
- <Icons.Table iconColor={supersetTheme.colors.grayscale.base} />
- )}
- {tableName}
- </StyledHeader>
+ <>
+ {linkedDatasetNames?.includes(tableName) &&
+ renderExistingDatasetAlert(
+ linkedDatasets?.find(dataset => dataset.table_name === tableName),
+ )}
+ <StyledHeader
+ position={
+ !loading && hasColumns ? EPosition.RELATIVE : EPosition.ABSOLUTE
+ }
+ title={tableName || ''}
+ >
+ {tableName && (
+ <Icons.Table iconColor={supersetTheme.colors.grayscale.base} />
Review Comment:
FIxed in [`this commit`](https://github.com/apache/superset/pull/22136/commits/86549660bd4d6ac53e0b7723ab3e9370e37d2a28).
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/DatasetPanel/DatasetPanel.tsx:
##########
@@ -19,10 +19,12 @@
import React from 'react';
import { supersetTheme, t, styled } from '@superset-ui/core';
Review Comment:
FIxed in [`this commit`](https://github.com/apache/superset/pull/22136/commits/86549660bd4d6ac53e0b7723ab3e9370e37d2a28).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] lyndsiWilliams commented on a diff in pull request #22136: feat: Flow for tables that already have a dataset
Posted by GitBox <gi...@apache.org>.
lyndsiWilliams commented on code in PR #22136:
URL: https://github.com/apache/superset/pull/22136#discussion_r1040071192
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/DatasetPanel/DatasetPanel.tsx:
##########
@@ -94,7 +94,7 @@ const LoaderContainer = styled.div`
display: flex;
align-items: center;
justify-content: center;
- height: 100%;
+ height: 98%;
Review Comment:
Implemented the border-box fix we discussed on Slack in [`this commit`](https://github.com/apache/superset/pull/22136/commits/11e2409d78b3e9227fa698f942eb999ae1570351).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] EugeneTorap commented on pull request #22136: feat: Flow for tables that already have a dataset
Posted by GitBox <gi...@apache.org>.
EugeneTorap commented on PR #22136:
URL: https://github.com/apache/superset/pull/22136#issuecomment-1320493203
@eschutho @eric-briscoe @lyndsiWilliams I have some nice suggestions!
- This is the new dataset creation page and we have [[SIP-61]](https://github.com/apache/superset/issues/13632) - Improve the organization of front-end folders.
We can move this page into `src/pages/addDataset` and it'll first component in `src/pages` after that if we create a new page for superset we can do it in `src/pages`.
- Creating Virtual Dataset on this new page. When we run our SQL query here we can see all metadata: columns types, used tables in the query, labels/aliases of columns and so on. Because in SQL Lab you run sql query and only see result data without columns metadata and others additional info.
What do you think about it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on a diff in pull request #22136: feat: Flow for tables that already have a dataset
Posted by GitBox <gi...@apache.org>.
eschutho commented on code in PR #22136:
URL: https://github.com/apache/superset/pull/22136#discussion_r1028817411
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/index.tsx:
##########
@@ -73,6 +75,38 @@ export default function AddDataset() {
Reducer<Partial<DatasetObject> | null, DSReducerActionType>
>(datasetReducer, null);
const [hasColumns, setHasColumns] = useState(false);
+ const [linkedDatasets, setLinkedDatasets] = useState<DatasetObject[]>([]);
Review Comment:
Looks good!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] yousoph commented on a diff in pull request #22136: feat: Flow for tables that already have a dataset
Posted by GitBox <gi...@apache.org>.
yousoph commented on code in PR #22136:
URL: https://github.com/apache/superset/pull/22136#discussion_r1029655603
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/DatasetPanel/DatasetPanel.tsx:
##########
@@ -168,15 +202,52 @@ export interface IDatasetPanelProps {
* Boolean indicating if the component is in a loading state
*/
loading: boolean;
+ datasets?: DatasetObject[] | undefined;
}
+const EXISTING_DATASET_DESCRIPTION = t(
+ 'You can only associate one dataset with one table. This table already has a dataset associated with it in Preset.\n',
Review Comment:
Updated text: This table already has a dataset associated with it. You can only associate one dataset with a table.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] lyndsiWilliams commented on pull request #22136: feat: Flow for tables that already have a dataset
Posted by GitBox <gi...@apache.org>.
lyndsiWilliams commented on PR #22136:
URL: https://github.com/apache/superset/pull/22136#issuecomment-1324313103
/testenv up
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on a diff in pull request #22136: feat: Flow for tables that already have a dataset
Posted by GitBox <gi...@apache.org>.
eschutho commented on code in PR #22136:
URL: https://github.com/apache/superset/pull/22136#discussion_r1025855704
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/index.tsx:
##########
@@ -73,6 +75,38 @@ export default function AddDataset() {
Reducer<Partial<DatasetObject> | null, DSReducerActionType>
>(datasetReducer, null);
const [hasColumns, setHasColumns] = useState(false);
+ const [linkedDatasets, setLinkedDatasets] = useState<DatasetObject[]>([]);
Review Comment:
this is a nit, but has been tripping me up when reading the code. Just for naming purposes, this is a list of all datasets, right? I'm not clear on which part is linked.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] AAfghahi commented on pull request #22136: feat: Flow for tables that already have a dataset
Posted by GitBox <gi...@apache.org>.
AAfghahi commented on PR #22136:
URL: https://github.com/apache/superset/pull/22136#issuecomment-1323833873
/testenv up
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] lyndsiWilliams commented on a diff in pull request #22136: feat: Flow for tables that already have a dataset
Posted by GitBox <gi...@apache.org>.
lyndsiWilliams commented on code in PR #22136:
URL: https://github.com/apache/superset/pull/22136#discussion_r1028718973
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/DatasetPanel/DatasetPanel.tsx:
##########
@@ -123,6 +128,24 @@ const StyledTable = styled(Table)`
right: 0;
`;
+const StyledAlert = styled(Alert)`
Review Comment:
FIxed in [`this commit`](https://github.com/apache/superset/pull/22136/commits/86549660bd4d6ac53e0b7723ab3e9370e37d2a28).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on a diff in pull request #22136: feat: Flow for tables that already have a dataset
Posted by GitBox <gi...@apache.org>.
eschutho commented on code in PR #22136:
URL: https://github.com/apache/superset/pull/22136#discussion_r1035435065
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/index.tsx:
##########
@@ -73,6 +75,38 @@ export default function AddDataset() {
Reducer<Partial<DatasetObject> | null, DSReducerActionType>
>(datasetReducer, null);
const [hasColumns, setHasColumns] = useState(false);
+ const [linkedDatasets, setLinkedDatasets] = useState<DatasetObject[]>([]);
+ const linkedDatasetNames = linkedDatasets.map(dataset => dataset.table_name);
+ const encodedSchema = dataset?.schema
+ ? encodeURIComponent(dataset?.schema)
+ : undefined;
+
+ const queryParams = dataset?.schema
+ ? rison.encode_uri({
+ filters: [
+ { col: 'schema', opr: 'eq', value: encodedSchema },
+ { col: 'sql', opr: 'dataset_is_null_or_empty', value: '!t' },
+ ],
+ })
+ : undefined;
+
+ const getDatasetsList = () => {
+ SupersetClient.get({
+ endpoint: `/api/v1/dataset/?q=${queryParams}`,
+ })
+ .then(({ json }) => {
+ json.result.map((dataset: any) =>
+ setLinkedDatasets(linkedDatasets => [...linkedDatasets, dataset]),
Review Comment:
nice!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov[bot] commented on pull request #22136: feat: Flow for tables that already have a dataset
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #22136:
URL: https://github.com/apache/superset/pull/22136#issuecomment-1316140926
# [Codecov](https://codecov.io/gh/apache/superset/pull/22136?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#22136](https://codecov.io/gh/apache/superset/pull/22136?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (8f47e55) into [master](https://codecov.io/gh/apache/superset/commit/6f6cb1839e8c688a929639dca7d0754e868ebfbf?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (6f6cb18) will **decrease** coverage by `0.19%`.
> The diff coverage is `63.63%`.
```diff
@@ Coverage Diff @@
## master #22136 +/- ##
==========================================
- Coverage 66.98% 66.79% -0.20%
==========================================
Files 1832 1832
Lines 69918 69942 +24
Branches 7570 7576 +6
==========================================
- Hits 46838 46720 -118
- Misses 21122 21260 +138
- Partials 1958 1962 +4
```
| Flag | Coverage Δ | |
|---|---|---|
| hive | `?` | |
| mysql | `?` | |
| postgres | `78.21% <ø> (ø)` | |
| presto | `?` | |
| python | `80.98% <ø> (-0.40%)` | :arrow_down: |
| sqlite | `76.67% <ø> (ø)` | |
| unit | `50.85% <ø> (ø)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/22136?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...RUD/data/dataset/AddDataset/DatasetPanel/index.tsx](https://codecov.io/gh/apache/superset/pull/22136/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvZGF0YS9kYXRhc2V0L0FkZERhdGFzZXQvRGF0YXNldFBhbmVsL2luZGV4LnRzeA==) | `29.03% <ø> (ø)` | |
| [...s/CRUD/data/dataset/AddDataset/LeftPanel/index.tsx](https://codecov.io/gh/apache/superset/pull/22136/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvZGF0YS9kYXRhc2V0L0FkZERhdGFzZXQvTGVmdFBhbmVsL2luZGV4LnRzeA==) | `85.48% <25.00%> (-1.19%)` | :arrow_down: |
| [...d/src/views/CRUD/data/dataset/AddDataset/index.tsx](https://codecov.io/gh/apache/superset/pull/22136/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvZGF0YS9kYXRhc2V0L0FkZERhdGFzZXQvaW5kZXgudHN4) | `51.61% <41.66%> (-8.39%)` | :arrow_down: |
| [...a/dataset/AddDataset/DatasetPanel/DatasetPanel.tsx](https://codecov.io/gh/apache/superset/pull/22136/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvZGF0YS9kYXRhc2V0L0FkZERhdGFzZXQvRGF0YXNldFBhbmVsL0RhdGFzZXRQYW5lbC50c3g=) | `91.37% <86.66%> (-0.29%)` | :arrow_down: |
| [...D/data/dataset/AddDataset/DatasetPanel/fixtures.ts](https://codecov.io/gh/apache/superset/pull/22136/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvZGF0YS9kYXRhc2V0L0FkZERhdGFzZXQvRGF0YXNldFBhbmVsL2ZpeHR1cmVzLnRz) | `100.00% <100.00%> (ø)` | |
| [...iews/CRUD/data/dataset/AddDataset/Footer/index.tsx](https://codecov.io/gh/apache/superset/pull/22136/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvZGF0YS9kYXRhc2V0L0FkZERhdGFzZXQvRm9vdGVyL2luZGV4LnRzeA==) | `37.50% <100.00%> (ø)` | |
| [superset/db\_engines/hive.py](https://codecov.io/gh/apache/superset/pull/22136/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGJfZW5naW5lcy9oaXZlLnB5) | `0.00% <0.00%> (-85.19%)` | :arrow_down: |
| [superset/db\_engine\_specs/hive.py](https://codecov.io/gh/apache/superset/pull/22136/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2hpdmUucHk=) | `71.14% <0.00%> (-16.21%)` | :arrow_down: |
| [superset/db\_engine\_specs/presto.py](https://codecov.io/gh/apache/superset/pull/22136/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL3ByZXN0by5weQ==) | `81.40% <0.00%> (-6.82%)` | :arrow_down: |
| [superset/common/utils/dataframe\_utils.py](https://codecov.io/gh/apache/superset/pull/22136/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvY29tbW9uL3V0aWxzL2RhdGFmcmFtZV91dGlscy5weQ==) | `90.47% <0.00%> (-4.77%)` | :arrow_down: |
| ... and [8 more](https://codecov.io/gh/apache/superset/pull/22136/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on a diff in pull request #22136: feat: Flow for tables that already have a dataset
Posted by GitBox <gi...@apache.org>.
eschutho commented on code in PR #22136:
URL: https://github.com/apache/superset/pull/22136#discussion_r1028815766
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/index.tsx:
##########
@@ -90,13 +91,11 @@ export default function AddDataset() {
})
: undefined;
- const getDatasetsList = () => {
- SupersetClient.get({
- endpoint: `/api/v1/dataset/?q=${queryParams}`,
- })
- .then(({ json }) => {
- json.result.map((dataset: any) =>
- setLinkedDatasets(linkedDatasets => [...linkedDatasets, dataset]),
+ const getDatasetsList = async () => {
+ await UseGetDatasetsList(queryParams)
+ .then(json => {
+ json?.result.map((dataset: DatasetObject) =>
Review Comment:
Bump on this comment before. Can you show the other method that you tried that didn’t work?
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/index.tsx:
##########
@@ -90,13 +91,11 @@ export default function AddDataset() {
})
: undefined;
- const getDatasetsList = () => {
- SupersetClient.get({
- endpoint: `/api/v1/dataset/?q=${queryParams}`,
- })
- .then(({ json }) => {
- json.result.map((dataset: any) =>
- setLinkedDatasets(linkedDatasets => [...linkedDatasets, dataset]),
+ const getDatasetsList = async () => {
+ await UseGetDatasetsList(queryParams)
+ .then(json => {
+ json?.result.map((dataset: DatasetObject) =>
Review Comment:
Bump on this comment from before. Can you show the other method that you tried that didn’t work?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on a diff in pull request #22136: feat: Flow for tables that already have a dataset
Posted by GitBox <gi...@apache.org>.
eschutho commented on code in PR #22136:
URL: https://github.com/apache/superset/pull/22136#discussion_r1028818578
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/index.tsx:
##########
@@ -73,6 +75,38 @@ export default function AddDataset() {
Reducer<Partial<DatasetObject> | null, DSReducerActionType>
>(datasetReducer, null);
const [hasColumns, setHasColumns] = useState(false);
+ const [linkedDatasets, setLinkedDatasets] = useState<DatasetObject[]>([]);
+ const linkedDatasetNames = linkedDatasets.map(dataset => dataset.table_name);
+ const encodedSchema = dataset?.schema
+ ? encodeURIComponent(dataset?.schema)
+ : undefined;
+
+ const queryParams = dataset?.schema
+ ? rison.encode_uri({
+ filters: [
+ { col: 'schema', opr: 'eq', value: encodedSchema },
+ { col: 'sql', opr: 'dataset_is_null_or_empty', value: '!t' },
+ ],
+ })
+ : undefined;
+
+ const getDatasetsList = () => {
+ SupersetClient.get({
+ endpoint: `/api/v1/dataset/?q=${queryParams}`,
+ })
+ .then(({ json }) => {
+ json.result.map((dataset: any) =>
+ setLinkedDatasets(linkedDatasets => [...linkedDatasets, dataset]),
Review Comment:
What about
.then(({ json }) => {
setDatasets(json.result)
})
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on a diff in pull request #22136: feat: Flow for tables that already have a dataset
Posted by GitBox <gi...@apache.org>.
eschutho commented on code in PR #22136:
URL: https://github.com/apache/superset/pull/22136#discussion_r1028634693
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/index.tsx:
##########
@@ -73,6 +75,38 @@ export default function AddDataset() {
Reducer<Partial<DatasetObject> | null, DSReducerActionType>
>(datasetReducer, null);
const [hasColumns, setHasColumns] = useState(false);
+ const [linkedDatasets, setLinkedDatasets] = useState<DatasetObject[]>([]);
Review Comment:
how about just "physicalDatasets"? the name doesn't explain what they're linked to, so maybe physicalDatasets will explain that they are just regular datasets, but only the physical, not virtual, ones.. honestly you could probably just say "datasets" and that would be fine, too. I don't think it's necessary to completely describe them here.. just that it's a list of datasets should be fine.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] lyndsiWilliams commented on a diff in pull request #22136: feat: Flow for tables that already have a dataset
Posted by GitBox <gi...@apache.org>.
lyndsiWilliams commented on code in PR #22136:
URL: https://github.com/apache/superset/pull/22136#discussion_r1029537208
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/DatasetPanel/DatasetPanel.tsx:
##########
@@ -168,15 +202,52 @@ export interface IDatasetPanelProps {
* Boolean indicating if the component is in a loading state
*/
loading: boolean;
+ datasets?: DatasetObject[] | undefined;
}
+const EXISTING_DATASET_DESCRIPTION = t(
+ 'You can only associate one dataset with one table. This table already has a dataset associated with it in Preset.\n',
Review Comment:
That makes sense, but I'm not sure about Preset-specific language here. I think we'd need @yousoph 's input on this one.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] lyndsiWilliams commented on a diff in pull request #22136: feat: Flow for tables that already have a dataset
Posted by GitBox <gi...@apache.org>.
lyndsiWilliams commented on code in PR #22136:
URL: https://github.com/apache/superset/pull/22136#discussion_r1029790139
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/DatasetPanel/DatasetPanel.tsx:
##########
@@ -168,15 +202,52 @@ export interface IDatasetPanelProps {
* Boolean indicating if the component is in a loading state
*/
loading: boolean;
+ datasets?: DatasetObject[] | undefined;
Review Comment:
We need the whole object so we can cross-reference the list of table names with the table names in the list of datasets here: https://github.com/apache/superset/blob/d77263dc348fe1fe0f54f82cd6118c2a6143501f/superset-frontend/src/views/CRUD/data/dataset/AddDataset/DatasetPanel/DatasetPanel.tsx#L292-L295
And when it's passed into `renderExistingDatasetAlert()` we need to be able to pull out the `explore_url` from the dataset object here: https://github.com/apache/superset/blob/d77263dc348fe1fe0f54f82cd6118c2a6143501f/superset-frontend/src/views/CRUD/data/dataset/AddDataset/DatasetPanel/DatasetPanel.tsx#L224-L230
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] lyndsiWilliams commented on a diff in pull request #22136: feat: Flow for tables that already have a dataset
Posted by GitBox <gi...@apache.org>.
lyndsiWilliams commented on code in PR #22136:
URL: https://github.com/apache/superset/pull/22136#discussion_r1028712942
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/Footer/index.tsx:
##########
@@ -113,7 +115,11 @@ function Footer({
<Button onClick={cancelButtonOnClick}>Cancel</Button>
<Button
buttonStyle="primary"
- disabled={!datasetObject?.table_name || !hasColumns}
+ disabled={
Review Comment:
Oh yeah good call, much cleaner! Changed in [`this commit`](https://github.com/apache/superset/pull/22136/commits/86549660bd4d6ac53e0b7723ab3e9370e37d2a28#diff-cefa9cf008be1b4a1474b0ae850f1849756208d2121d3e3bda118e29ff8f56e1R114-R117)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] lyndsiWilliams commented on a diff in pull request #22136: feat: Flow for tables that already have a dataset
Posted by GitBox <gi...@apache.org>.
lyndsiWilliams commented on code in PR #22136:
URL: https://github.com/apache/superset/pull/22136#discussion_r1028708965
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/DatasetPanel/DatasetPanel.tsx:
##########
@@ -168,15 +191,48 @@ export interface IDatasetPanelProps {
* Boolean indicating if the component is in a loading state
*/
loading: boolean;
+ linkedDatasets?: DatasetObject[] | undefined;
}
+const renderExistingDatasetAlert = (linkedDataset: any) => (
Review Comment:
Oops, good catch! Typed in [`this commit`](https://github.com/apache/superset/pull/22136/commits/86549660bd4d6ac53e0b7723ab3e9370e37d2a28#diff-544198bdd029b6e92c7a16fd435544ef915ae29e2de080abc82b23377094138eR213).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] lyndsiWilliams commented on a diff in pull request #22136: feat: Flow for tables that already have a dataset
Posted by GitBox <gi...@apache.org>.
lyndsiWilliams commented on code in PR #22136:
URL: https://github.com/apache/superset/pull/22136#discussion_r1028674246
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/index.tsx:
##########
@@ -73,6 +75,38 @@ export default function AddDataset() {
Reducer<Partial<DatasetObject> | null, DSReducerActionType>
>(datasetReducer, null);
const [hasColumns, setHasColumns] = useState(false);
+ const [linkedDatasets, setLinkedDatasets] = useState<DatasetObject[]>([]);
+ const linkedDatasetNames = linkedDatasets.map(dataset => dataset.table_name);
+ const encodedSchema = dataset?.schema
+ ? encodeURIComponent(dataset?.schema)
+ : undefined;
+
+ const queryParams = dataset?.schema
+ ? rison.encode_uri({
+ filters: [
+ { col: 'schema', opr: 'eq', value: encodedSchema },
+ { col: 'sql', opr: 'dataset_is_null_or_empty', value: '!t' },
+ ],
+ })
+ : undefined;
+
+ const getDatasetsList = () => {
+ SupersetClient.get({
+ endpoint: `/api/v1/dataset/?q=${queryParams}`,
+ })
+ .then(({ json }) => {
+ json.result.map((dataset: any) =>
+ setLinkedDatasets(linkedDatasets => [...linkedDatasets, dataset]),
Review Comment:
When I set the datasets to the entire response like this:
`json.result.map((dataset: any) => setLinkedDatasets(dataset));`
It sets the dataset each time the map runs. So it's always set to one dataset, see this screenshot where I console.logged datasets:
When I originally set this up and ran into this issue setting the datasets into an array, I did some googling for a solution and found this method in [a stackoverflow article](https://stackoverflow.com/questions/54676966/push-method-in-react-hooks-usestate).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] AAfghahi commented on a diff in pull request #22136: feat: Flow for tables that already have a dataset
Posted by GitBox <gi...@apache.org>.
AAfghahi commented on code in PR #22136:
URL: https://github.com/apache/superset/pull/22136#discussion_r1030798691
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/DatasetPanel/DatasetPanel.tsx:
##########
@@ -168,15 +202,52 @@ export interface IDatasetPanelProps {
* Boolean indicating if the component is in a loading state
*/
loading: boolean;
+ datasets?: DatasetObject[] | undefined;
Review Comment:
got it! Makes sense
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] AAfghahi commented on a diff in pull request #22136: feat: Flow for tables that already have a dataset
Posted by GitBox <gi...@apache.org>.
AAfghahi commented on code in PR #22136:
URL: https://github.com/apache/superset/pull/22136#discussion_r1029452626
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/DatasetPanel/DatasetPanel.tsx:
##########
@@ -168,15 +202,52 @@ export interface IDatasetPanelProps {
* Boolean indicating if the component is in a loading state
*/
loading: boolean;
+ datasets?: DatasetObject[] | undefined;
Review Comment:
nit: I kind of want this to have a better name, maybe existingDatasets? or something along those lines.
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/DatasetPanel/DatasetPanel.tsx:
##########
@@ -168,15 +202,52 @@ export interface IDatasetPanelProps {
* Boolean indicating if the component is in a loading state
*/
loading: boolean;
+ datasets?: DatasetObject[] | undefined;
}
+const EXISTING_DATASET_DESCRIPTION = t(
+ 'You can only associate one dataset with one table. This table already has a dataset associated with it in Preset.\n',
Review Comment:
Hmm, does this need to be preset specific language? If so, then I think we need to add that in on the Preset side and have a more generic description in superset.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] lyndsiWilliams commented on a diff in pull request #22136: feat: Flow for tables that already have a dataset
Posted by GitBox <gi...@apache.org>.
lyndsiWilliams commented on code in PR #22136:
URL: https://github.com/apache/superset/pull/22136#discussion_r1029790139
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/DatasetPanel/DatasetPanel.tsx:
##########
@@ -168,15 +202,52 @@ export interface IDatasetPanelProps {
* Boolean indicating if the component is in a loading state
*/
loading: boolean;
+ datasets?: DatasetObject[] | undefined;
Review Comment:
We need the whole object so we can cross-reference the list of table names with the table names in the list of datasets here: https://github.com/apache/superset/blob/d77263dc348fe1fe0f54f82cd6118c2a6143501f/superset-frontend/src/views/CRUD/data/dataset/AddDataset/DatasetPanel/DatasetPanel.tsx#L292-L295
And when it's passed into `renderExistingDatasetAlert()` we need to be able to pull out the `explore_url` from the dataset object here: https://github.com/apache/superset/blob/0bed85aa39b40be715a33d3d8db62ac8ca2eb853/superset-frontend/src/views/CRUD/data/dataset/AddDataset/DatasetPanel/DatasetPanel.tsx#L224-L230
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eric-briscoe commented on a diff in pull request #22136: feat: Flow for tables that already have a dataset
Posted by GitBox <gi...@apache.org>.
eric-briscoe commented on code in PR #22136:
URL: https://github.com/apache/superset/pull/22136#discussion_r1038604696
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/DatasetPanel/DatasetPanel.tsx:
##########
@@ -94,7 +94,7 @@ const LoaderContainer = styled.div`
display: flex;
align-items: center;
justify-content: center;
- height: 100%;
+ height: 98%;
Review Comment:
@lyndsiWilliams typically we we see scrolling with something where width / height is set to 100% due to box-sizing logic. By default CSS does not subtract padding and border thickness from the value of the parent elements size. This makes the child set its size to 100% of the parent element plus the child's padding and border dimensions (100% of parent + child's padding and borders making the child larger than the parents visible area). Getting rid of the scrollbar should be achievable with:
```
height: 100%;
box-sizing: border-box;
```
The CSS `box-sizing` property allows us to include the padding and border in an element's total width and height calculation
The reason we might want to try this instead of reducing the height to 98% is that 2% of the total height in actual pixels varies on the browser window's height. The higher the screen resolution and taller the browser window is, the more that 2% will cause an offset of actual vertical center alignment. For example on a HD screen the offset would be ~ 17px (850px of usable screen after browsers UI. 850 * 0.02 = ~17px). On a 4k screen the offset could be significantly larger. In other words as the browser grows in height, the more non-centered aligned the item becomes. Alternatively, as the screen size reduces it becomes possible that 2% is less than the height - padding+border and scroll bar still ends up appearing. Using 100% with the box-sizing ensures we remain center aligned and void scroll regardless of browser height and screen resolution.
good explanation with interactive examples of box-sizing here: https://www.w3schools.com/css/css3_box-sizing.asp
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] lyndsiWilliams commented on a diff in pull request #22136: feat: Flow for tables that already have a dataset
Posted by GitBox <gi...@apache.org>.
lyndsiWilliams commented on code in PR #22136:
URL: https://github.com/apache/superset/pull/22136#discussion_r1038656637
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/DatasetPanel/DatasetPanel.tsx:
##########
@@ -94,7 +94,7 @@ const LoaderContainer = styled.div`
display: flex;
align-items: center;
justify-content: center;
- height: 100%;
+ height: 98%;
Review Comment:
Oh that makes sense, I didn't know about box-sizing, thank you! But setting height to 100% and box-sizing: border-box did not get rid of the scrollbar on my end. Did it work on your end?
I see the reasoning for wanting to include 100% of the element, but I was thinking it might be acceptable to do 98% height for this since it's just the loading gif that shows in the center, so that 2% will never cut any visual elements off. What do you think?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on a diff in pull request #22136: feat: Flow for tables that already have a dataset
Posted by GitBox <gi...@apache.org>.
eschutho commented on code in PR #22136:
URL: https://github.com/apache/superset/pull/22136#discussion_r1025854683
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/index.tsx:
##########
@@ -73,6 +75,38 @@ export default function AddDataset() {
Reducer<Partial<DatasetObject> | null, DSReducerActionType>
>(datasetReducer, null);
const [hasColumns, setHasColumns] = useState(false);
+ const [linkedDatasets, setLinkedDatasets] = useState<DatasetObject[]>([]);
+ const linkedDatasetNames = linkedDatasets.map(dataset => dataset.table_name);
+ const encodedSchema = dataset?.schema
+ ? encodeURIComponent(dataset?.schema)
+ : undefined;
+
+ const queryParams = dataset?.schema
+ ? rison.encode_uri({
+ filters: [
+ { col: 'schema', opr: 'eq', value: encodedSchema },
+ { col: 'sql', opr: 'dataset_is_null_or_empty', value: '!t' },
+ ],
+ })
+ : undefined;
+
+ const getDatasetsList = () => {
+ SupersetClient.get({
+ endpoint: `/api/v1/dataset/?q=${queryParams}`,
+ })
+ .then(({ json }) => {
+ json.result.map((dataset: any) =>
+ setLinkedDatasets(linkedDatasets => [...linkedDatasets, dataset]),
+ );
+ })
+ .catch(error =>
+ console.log('There was an error fetching dataset', error),
+ );
+ };
+
+ useEffect(() => {
+ if (dataset?.schema) getDatasetsList();
Review Comment:
agree on the lint rule +1
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on a diff in pull request #22136: feat: Flow for tables that already have a dataset
Posted by GitBox <gi...@apache.org>.
eschutho commented on code in PR #22136:
URL: https://github.com/apache/superset/pull/22136#discussion_r1025858728
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/index.tsx:
##########
@@ -73,6 +75,38 @@ export default function AddDataset() {
Reducer<Partial<DatasetObject> | null, DSReducerActionType>
>(datasetReducer, null);
const [hasColumns, setHasColumns] = useState(false);
+ const [linkedDatasets, setLinkedDatasets] = useState<DatasetObject[]>([]);
+ const linkedDatasetNames = linkedDatasets.map(dataset => dataset.table_name);
+ const encodedSchema = dataset?.schema
+ ? encodeURIComponent(dataset?.schema)
+ : undefined;
+
+ const queryParams = dataset?.schema
+ ? rison.encode_uri({
+ filters: [
+ { col: 'schema', opr: 'eq', value: encodedSchema },
+ { col: 'sql', opr: 'dataset_is_null_or_empty', value: '!t' },
+ ],
+ })
+ : undefined;
+
+ const getDatasetsList = () => {
Review Comment:
I'd recommend moving any api calls outside of the view component into either a hook or redux action for separation of concerns.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] lyndsiWilliams commented on pull request #22136: feat: Flow for tables that already have a dataset
Posted by GitBox <gi...@apache.org>.
lyndsiWilliams commented on PR #22136:
URL: https://github.com/apache/superset/pull/22136#issuecomment-1322924790
> @eschutho @eric-briscoe @lyndsiWilliams I have some nice suggestions!
>
> * This is the new dataset creation page and we have [[SIP-61]](https://github.com/apache/superset/issues/13632) - `Improve the organization of front-end folders` created by @michael-s-molina
> We can move this page into `src/pages/addDataset` and it'll be first component in `src/pages` after that we can smoothly move others page to the folder or if we create a new page for superset we can immediately do it in `src/pages`.
> * Creating Virtual Dataset on this new page. It will be good if we run our SQL query here we can see all metadata: columns types, used tables in the query, labels/aliases of columns and so on. Because in SQL Lab if you run sql query you can only see result data without columns metadata and others additional info.
>
> What do you think about it?
Hey @EugeneTorap!
* This is a good suggestion, but it is a pretty substantial change for this PR. It will be better to do this in a future cleanup PR.
* We currently don't have plans to add a virtual dataset from this flow. Now that we can create charts from a query in SQL Lab, the left panel in Explore should show you the column metadata info.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on a diff in pull request #22136: feat: Flow for tables that already have a dataset
Posted by GitBox <gi...@apache.org>.
eschutho commented on code in PR #22136:
URL: https://github.com/apache/superset/pull/22136#discussion_r1028815766
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/index.tsx:
##########
@@ -90,13 +91,11 @@ export default function AddDataset() {
})
: undefined;
- const getDatasetsList = () => {
- SupersetClient.get({
- endpoint: `/api/v1/dataset/?q=${queryParams}`,
- })
- .then(({ json }) => {
- json.result.map((dataset: any) =>
- setLinkedDatasets(linkedDatasets => [...linkedDatasets, dataset]),
+ const getDatasetsList = async () => {
+ await UseGetDatasetsList(queryParams)
+ .then(json => {
+ json?.result.map((dataset: DatasetObject) =>
Review Comment:
Bump on this comment from before. Can you show the other method that you tried that didn’t work?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] lyndsiWilliams commented on a diff in pull request #22136: feat: Flow for tables that already have a dataset
Posted by GitBox <gi...@apache.org>.
lyndsiWilliams commented on code in PR #22136:
URL: https://github.com/apache/superset/pull/22136#discussion_r1028711384
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/DatasetPanel/DatasetPanel.tsx:
##########
@@ -168,15 +191,48 @@ export interface IDatasetPanelProps {
* Boolean indicating if the component is in a loading state
*/
loading: boolean;
+ linkedDatasets?: DatasetObject[] | undefined;
}
+const renderExistingDatasetAlert = (linkedDataset: any) => (
+ <StyledAlert
+ closable={false}
+ type="info"
+ showIcon
+ message={t('This table already has a dataset')}
+ description={
+ <>
+ {t(
Review Comment:
Thanks for the explanation on this, I didn't realize it was running on each render/function call but it definitely makes sense! I fixed these spots and some others that I found in [`this commit`](https://github.com/apache/superset/pull/22136/commits/86549660bd4d6ac53e0b7723ab3e9370e37d2a28).
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/DatasetPanel/index.tsx:
##########
@@ -110,7 +113,7 @@ const DatasetPanelWrapper = ({
if (tableName && schema && dbId) {
getTableMetadata({ tableName, dbId, schema });
}
- // getTableMetadata is a const and should not be independency array
+ // getTableMetadata is a const and should not be in dependency array
Review Comment:
Thanks! 😁
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] lyndsiWilliams commented on a diff in pull request #22136: feat: Flow for tables that already have a dataset
Posted by GitBox <gi...@apache.org>.
lyndsiWilliams commented on code in PR #22136:
URL: https://github.com/apache/superset/pull/22136#discussion_r1028695822
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/DatasetPanel/DatasetPanel.tsx:
##########
@@ -54,8 +56,10 @@ const MARGIN_MULTIPLIER = 3;
const StyledHeader = styled.div<StyledHeaderProps>`
position: ${(props: StyledHeaderProps) => props.position};
- margin: ${({ theme }) => theme.gridUnit * MARGIN_MULTIPLIER}px;
- margin-top: ${({ theme }) => theme.gridUnit * (MARGIN_MULTIPLIER + 1)}px;
+ margin: ${({ theme }) => theme.gridUnit * (MARGIN_MULTIPLIER + 1)}px
+ ${({ theme }) => theme.gridUnit * MARGIN_MULTIPLIER}px
+ ${({ theme }) => theme.gridUnit * MARGIN_MULTIPLIER}px
+ ${({ theme }) => theme.gridUnit * (MARGIN_MULTIPLIER + 3)}px;
Review Comment:
Oh yeah, that is a lot cleaner! I cleaned up this and some other spots that could be cleaned in the same way in [`this commit`](https://github.com/apache/superset/pull/22136/commits/86549660bd4d6ac53e0b7723ab3e9370e37d2a28).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on a diff in pull request #22136: feat: Flow for tables that already have a dataset
Posted by GitBox <gi...@apache.org>.
eschutho commented on code in PR #22136:
URL: https://github.com/apache/superset/pull/22136#discussion_r1028817081
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/DatasetPanel/DatasetPanel.tsx:
##########
@@ -54,8 +56,10 @@ const MARGIN_MULTIPLIER = 3;
const StyledHeader = styled.div<StyledHeaderProps>`
position: ${(props: StyledHeaderProps) => props.position};
- margin: ${({ theme }) => theme.gridUnit * MARGIN_MULTIPLIER}px;
- margin-top: ${({ theme }) => theme.gridUnit * (MARGIN_MULTIPLIER + 1)}px;
+ margin: ${({ theme }) => theme.gridUnit * (MARGIN_MULTIPLIER + 1)}px
+ ${({ theme }) => theme.gridUnit * MARGIN_MULTIPLIER}px
+ ${({ theme }) => theme.gridUnit * MARGIN_MULTIPLIER}px
+ ${({ theme }) => theme.gridUnit * (MARGIN_MULTIPLIER + 3)}px;
Review Comment:
Looks good!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] lyndsiWilliams commented on a diff in pull request #22136: feat: Flow for tables that already have a dataset
Posted by GitBox <gi...@apache.org>.
lyndsiWilliams commented on code in PR #22136:
URL: https://github.com/apache/superset/pull/22136#discussion_r1029536297
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/DatasetPanel/DatasetPanel.tsx:
##########
@@ -168,15 +202,52 @@ export interface IDatasetPanelProps {
* Boolean indicating if the component is in a loading state
*/
loading: boolean;
+ datasets?: DatasetObject[] | undefined;
Review Comment:
I was going off of what @eschutho suggested in [this review comment](https://github.com/apache/superset/pull/22136#discussion_r1028634693). I'm not really settled on the best name here but I'd be fine changing it, would like to hear what Elizabeth thinks of `existingDatasets` as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] lyndsiWilliams commented on a diff in pull request #22136: feat: Flow for tables that already have a dataset
Posted by GitBox <gi...@apache.org>.
lyndsiWilliams commented on code in PR #22136:
URL: https://github.com/apache/superset/pull/22136#discussion_r1028359385
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/index.tsx:
##########
@@ -73,6 +75,38 @@ export default function AddDataset() {
Reducer<Partial<DatasetObject> | null, DSReducerActionType>
>(datasetReducer, null);
const [hasColumns, setHasColumns] = useState(false);
+ const [linkedDatasets, setLinkedDatasets] = useState<DatasetObject[]>([]);
Review Comment:
These are the datasets that are linked to a table, I wasn't sure of a great name for this. What would be a better name?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] lyndsiWilliams commented on a diff in pull request #22136: feat: Flow for tables that already have a dataset
Posted by GitBox <gi...@apache.org>.
lyndsiWilliams commented on code in PR #22136:
URL: https://github.com/apache/superset/pull/22136#discussion_r1029661606
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/DatasetPanel/DatasetPanel.tsx:
##########
@@ -168,15 +202,52 @@ export interface IDatasetPanelProps {
* Boolean indicating if the component is in a loading state
*/
loading: boolean;
+ datasets?: DatasetObject[] | undefined;
}
+const EXISTING_DATASET_DESCRIPTION = t(
+ 'You can only associate one dataset with one table. This table already has a dataset associated with it in Preset.\n',
Review Comment:
I spoke with Sophie and Karan in Slack and we discussed changing the text to be more general and not include Superset or Preset. Changed in [`this commit`](https://github.com/apache/superset/pull/22136/commits/f4a42c14eca0c2b1283a0b9b68cc27e31cc2e2d3).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] github-actions[bot] commented on pull request #22136: feat: Flow for tables that already have a dataset
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #22136:
URL: https://github.com/apache/superset/pull/22136#issuecomment-1324317788
@lyndsiWilliams Ephemeral environment spinning up at http://54.202.36.48:8080. Credentials are `admin`/`admin`. Please allow several minutes for bootstrapping and startup.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on a diff in pull request #22136: feat: Flow for tables that already have a dataset
Posted by GitBox <gi...@apache.org>.
eschutho commented on code in PR #22136:
URL: https://github.com/apache/superset/pull/22136#discussion_r1024748499
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/DatasetPanel/DatasetPanel.tsx:
##########
@@ -54,8 +56,10 @@ const MARGIN_MULTIPLIER = 3;
const StyledHeader = styled.div<StyledHeaderProps>`
position: ${(props: StyledHeaderProps) => props.position};
- margin: ${({ theme }) => theme.gridUnit * MARGIN_MULTIPLIER}px;
- margin-top: ${({ theme }) => theme.gridUnit * (MARGIN_MULTIPLIER + 1)}px;
+ margin: ${({ theme }) => theme.gridUnit * (MARGIN_MULTIPLIER + 1)}px
+ ${({ theme }) => theme.gridUnit * MARGIN_MULTIPLIER}px
+ ${({ theme }) => theme.gridUnit * MARGIN_MULTIPLIER}px
+ ${({ theme }) => theme.gridUnit * (MARGIN_MULTIPLIER + 3)}px;
Review Comment:
I believe you can make this just one function for all margin sides
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/DatasetPanel/DatasetPanel.tsx:
##########
@@ -168,15 +191,48 @@ export interface IDatasetPanelProps {
* Boolean indicating if the component is in a loading state
*/
loading: boolean;
+ linkedDatasets?: DatasetObject[] | undefined;
}
+const renderExistingDatasetAlert = (linkedDataset: any) => (
Review Comment:
Can we put a type on this linkeddataset?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on a diff in pull request #22136: feat: Flow for tables that already have a dataset
Posted by GitBox <gi...@apache.org>.
eschutho commented on code in PR #22136:
URL: https://github.com/apache/superset/pull/22136#discussion_r1025857796
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/index.tsx:
##########
@@ -73,6 +75,38 @@ export default function AddDataset() {
Reducer<Partial<DatasetObject> | null, DSReducerActionType>
>(datasetReducer, null);
const [hasColumns, setHasColumns] = useState(false);
+ const [linkedDatasets, setLinkedDatasets] = useState<DatasetObject[]>([]);
+ const linkedDatasetNames = linkedDatasets.map(dataset => dataset.table_name);
+ const encodedSchema = dataset?.schema
+ ? encodeURIComponent(dataset?.schema)
+ : undefined;
+
+ const queryParams = dataset?.schema
+ ? rison.encode_uri({
+ filters: [
+ { col: 'schema', opr: 'eq', value: encodedSchema },
+ { col: 'sql', opr: 'dataset_is_null_or_empty', value: '!t' },
+ ],
+ })
+ : undefined;
+
+ const getDatasetsList = () => {
+ SupersetClient.get({
+ endpoint: `/api/v1/dataset/?q=${queryParams}`,
+ })
+ .then(({ json }) => {
+ json.result.map((dataset: any) =>
+ setLinkedDatasets(linkedDatasets => [...linkedDatasets, dataset]),
Review Comment:
I'm not understand this part here.. what's the difference between looping over the json result and adding the datasets to the list as opposed to just setting linkedDatasets to the entire repsonse?
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/index.tsx:
##########
@@ -73,6 +75,38 @@ export default function AddDataset() {
Reducer<Partial<DatasetObject> | null, DSReducerActionType>
>(datasetReducer, null);
const [hasColumns, setHasColumns] = useState(false);
+ const [linkedDatasets, setLinkedDatasets] = useState<DatasetObject[]>([]);
+ const linkedDatasetNames = linkedDatasets.map(dataset => dataset.table_name);
+ const encodedSchema = dataset?.schema
+ ? encodeURIComponent(dataset?.schema)
+ : undefined;
+
+ const queryParams = dataset?.schema
+ ? rison.encode_uri({
+ filters: [
+ { col: 'schema', opr: 'eq', value: encodedSchema },
+ { col: 'sql', opr: 'dataset_is_null_or_empty', value: '!t' },
+ ],
+ })
+ : undefined;
+
+ const getDatasetsList = () => {
+ SupersetClient.get({
+ endpoint: `/api/v1/dataset/?q=${queryParams}`,
+ })
+ .then(({ json }) => {
+ json.result.map((dataset: any) =>
+ setLinkedDatasets(linkedDatasets => [...linkedDatasets, dataset]),
Review Comment:
I'm not understand this part here.. what's the difference between looping over the json result and adding the datasets to the list as opposed to just setting linkedDatasets to the entire response?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eric-briscoe commented on a diff in pull request #22136: feat: Flow for tables that already have a dataset
Posted by GitBox <gi...@apache.org>.
eric-briscoe commented on code in PR #22136:
URL: https://github.com/apache/superset/pull/22136#discussion_r1024505030
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/Footer/index.tsx:
##########
@@ -113,7 +115,11 @@ function Footer({
<Button onClick={cancelButtonOnClick}>Cancel</Button>
<Button
buttonStyle="primary"
- disabled={!datasetObject?.table_name || !hasColumns}
+ disabled={
Review Comment:
This is a nit, but since this disabled check is getting kind of long it may be worth defining a variable doing this check before the return JSX block which will make the code easier to debug in future and keep the actual JSX portion cleaner.
Example:
```
const disabled = !datasetObject?.table_name ||
!hasColumns ||
linkedDatasets?.includes(datasetObject?.table_name);
return (
<>
<Button onClick={cancelButtonOnClick}>Cancel</Button>
<Button
buttonStyle="primary"
disabled
tooltip={!datasetObject?.table_name ? tooltipText : undefined}
onClick={onSave}
>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] lyndsiWilliams commented on a diff in pull request #22136: feat: Flow for tables that already have a dataset
Posted by GitBox <gi...@apache.org>.
lyndsiWilliams commented on code in PR #22136:
URL: https://github.com/apache/superset/pull/22136#discussion_r1028712022
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/LeftPanel/index.tsx:
##########
@@ -166,9 +182,7 @@ export default function LeftPanel({
setResetTables(false);
setRefresh(false);
})
- .catch(e => {
- console.log('error', e);
- });
+ .catch(error => console.log('There was an error fetching tables', error));
Review Comment:
I actually didn't know about this logging utility, thanks for the info! Changed the catch in [`this commit`](https://github.com/apache/superset/pull/22136/commits/86549660bd4d6ac53e0b7723ab3e9370e37d2a28).
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/index.tsx:
##########
@@ -73,6 +75,38 @@ export default function AddDataset() {
Reducer<Partial<DatasetObject> | null, DSReducerActionType>
>(datasetReducer, null);
const [hasColumns, setHasColumns] = useState(false);
+ const [linkedDatasets, setLinkedDatasets] = useState<DatasetObject[]>([]);
+ const linkedDatasetNames = linkedDatasets.map(dataset => dataset.table_name);
+ const encodedSchema = dataset?.schema
+ ? encodeURIComponent(dataset?.schema)
+ : undefined;
+
+ const queryParams = dataset?.schema
+ ? rison.encode_uri({
+ filters: [
+ { col: 'schema', opr: 'eq', value: encodedSchema },
+ { col: 'sql', opr: 'dataset_is_null_or_empty', value: '!t' },
+ ],
+ })
+ : undefined;
+
+ const getDatasetsList = () => {
+ SupersetClient.get({
+ endpoint: `/api/v1/dataset/?q=${queryParams}`,
+ })
+ .then(({ json }) => {
+ json.result.map((dataset: any) =>
+ setLinkedDatasets(linkedDatasets => [...linkedDatasets, dataset]),
+ );
+ })
+ .catch(error =>
+ console.log('There was an error fetching dataset', error),
Review Comment:
Changed the catch in [`this commit`](https://github.com/apache/superset/pull/22136/commits/86549660bd4d6ac53e0b7723ab3e9370e37d2a28).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] lyndsiWilliams commented on a diff in pull request #22136: feat: Flow for tables that already have a dataset
Posted by GitBox <gi...@apache.org>.
lyndsiWilliams commented on code in PR #22136:
URL: https://github.com/apache/superset/pull/22136#discussion_r1029537208
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/DatasetPanel/DatasetPanel.tsx:
##########
@@ -168,15 +202,52 @@ export interface IDatasetPanelProps {
* Boolean indicating if the component is in a loading state
*/
loading: boolean;
+ datasets?: DatasetObject[] | undefined;
}
+const EXISTING_DATASET_DESCRIPTION = t(
+ 'You can only associate one dataset with one table. This table already has a dataset associated with it in Preset.\n',
Review Comment:
I'm not sure about Preset-specific language here, I think we'd need @yousoph 's input on this one.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on a diff in pull request #22136: feat: Flow for tables that already have a dataset
Posted by GitBox <gi...@apache.org>.
eschutho commented on code in PR #22136:
URL: https://github.com/apache/superset/pull/22136#discussion_r1028637288
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/index.tsx:
##########
@@ -73,6 +75,38 @@ export default function AddDataset() {
Reducer<Partial<DatasetObject> | null, DSReducerActionType>
>(datasetReducer, null);
const [hasColumns, setHasColumns] = useState(false);
+ const [linkedDatasets, setLinkedDatasets] = useState<DatasetObject[]>([]);
+ const linkedDatasetNames = linkedDatasets.map(dataset => dataset.table_name);
+ const encodedSchema = dataset?.schema
+ ? encodeURIComponent(dataset?.schema)
+ : undefined;
+
+ const queryParams = dataset?.schema
+ ? rison.encode_uri({
+ filters: [
+ { col: 'schema', opr: 'eq', value: encodedSchema },
+ { col: 'sql', opr: 'dataset_is_null_or_empty', value: '!t' },
+ ],
+ })
+ : undefined;
+
+ const getDatasetsList = () => {
+ SupersetClient.get({
+ endpoint: `/api/v1/dataset/?q=${queryParams}`,
+ })
+ .then(({ json }) => {
+ json.result.map((dataset: any) =>
+ setLinkedDatasets(linkedDatasets => [...linkedDatasets, dataset]),
Review Comment:
can you paste an example of that?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] lyndsiWilliams commented on a diff in pull request #22136: feat: Flow for tables that already have a dataset
Posted by GitBox <gi...@apache.org>.
lyndsiWilliams commented on code in PR #22136:
URL: https://github.com/apache/superset/pull/22136#discussion_r1028827114
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/index.tsx:
##########
@@ -73,6 +75,38 @@ export default function AddDataset() {
Reducer<Partial<DatasetObject> | null, DSReducerActionType>
>(datasetReducer, null);
const [hasColumns, setHasColumns] = useState(false);
+ const [linkedDatasets, setLinkedDatasets] = useState<DatasetObject[]>([]);
+ const linkedDatasetNames = linkedDatasets.map(dataset => dataset.table_name);
+ const encodedSchema = dataset?.schema
+ ? encodeURIComponent(dataset?.schema)
+ : undefined;
+
+ const queryParams = dataset?.schema
+ ? rison.encode_uri({
+ filters: [
+ { col: 'schema', opr: 'eq', value: encodedSchema },
+ { col: 'sql', opr: 'dataset_is_null_or_empty', value: '!t' },
+ ],
+ })
+ : undefined;
+
+ const getDatasetsList = () => {
+ SupersetClient.get({
+ endpoint: `/api/v1/dataset/?q=${queryParams}`,
+ })
+ .then(({ json }) => {
+ json.result.map((dataset: any) =>
+ setLinkedDatasets(linkedDatasets => [...linkedDatasets, dataset]),
Review Comment:
That worked, thank you! Fixed in [`this commit`](https://github.com/apache/superset/pull/22136/commits/955ffc0b4425550a825757cd9b55f240d0d11b98).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] github-actions[bot] commented on pull request #22136: feat: Flow for tables that already have a dataset
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #22136:
URL: https://github.com/apache/superset/pull/22136#issuecomment-1316229630
@lyndsiWilliams Ephemeral environment spinning up at http://35.85.146.196:8080. Credentials are `admin`/`admin`. Please allow several minutes for bootstrapping and startup.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eric-briscoe commented on a diff in pull request #22136: feat: Flow for tables that already have a dataset
Posted by GitBox <gi...@apache.org>.
eric-briscoe commented on code in PR #22136:
URL: https://github.com/apache/superset/pull/22136#discussion_r1023414273
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/DatasetPanel/DatasetPanel.tsx:
##########
@@ -168,15 +191,48 @@ export interface IDatasetPanelProps {
* Boolean indicating if the component is in a loading state
*/
loading: boolean;
+ linkedDatasets?: DatasetObject[] | undefined;
}
+const renderExistingDatasetAlert = (linkedDataset: any) => (
+ <StyledAlert
+ closable={false}
+ type="info"
+ showIcon
+ message={t('This table already has a dataset')}
+ description={
+ <>
+ {t(
Review Comment:
I recommend defining all text using `t()` as const outside of functions so that the text only gets translated once, not per render / function call. It also makes the text exportable for use in test files so you don't have to keep string literals defined in multiple files in sync manually.
I am commenting once but recommend for anywhere `t()` is being called inside a function / functional component.
Example can be seen on line 149, 150, 151
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/DatasetPanel/index.tsx:
##########
@@ -110,7 +113,7 @@ const DatasetPanelWrapper = ({
if (tableName && schema && dbId) {
getTableMetadata({ tableName, dbId, schema });
}
- // getTableMetadata is a const and should not be independency array
+ // getTableMetadata is a const and should not be in dependency array
Review Comment:
nice catch!
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/LeftPanel/index.tsx:
##########
@@ -166,9 +182,7 @@ export default function LeftPanel({
setResetTables(false);
setRefresh(false);
})
- .catch(e => {
- console.log('error', e);
- });
+ .catch(error => console.log('There was an error fetching tables', error));
Review Comment:
Should this be an error instead of log? Also there is actually a logging utility in Superset-frontend that I was introduced to lately that should be best practice instead of calling console.log | .error | etc directly
You can: `import { logging } from '@superset-ui/core';`
Then call the logging methods defined at: https://github.com/apache/superset/blob/3c41ff68a43b5ab6b871226a73de9f2129d64766/superset-frontend/packages/superset-ui-core/src/utils/logging.ts
This lets us avoid ts-ignore comments anywhere we legitimately need to log something, and will allow for enriching what we do with logging calls in future, such as send some of the errors for backend logging to capture in usability metrics
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/index.tsx:
##########
@@ -73,6 +75,38 @@ export default function AddDataset() {
Reducer<Partial<DatasetObject> | null, DSReducerActionType>
>(datasetReducer, null);
const [hasColumns, setHasColumns] = useState(false);
+ const [linkedDatasets, setLinkedDatasets] = useState<DatasetObject[]>([]);
+ const linkedDatasetNames = linkedDatasets.map(dataset => dataset.table_name);
+ const encodedSchema = dataset?.schema
+ ? encodeURIComponent(dataset?.schema)
+ : undefined;
+
+ const queryParams = dataset?.schema
+ ? rison.encode_uri({
+ filters: [
+ { col: 'schema', opr: 'eq', value: encodedSchema },
+ { col: 'sql', opr: 'dataset_is_null_or_empty', value: '!t' },
+ ],
+ })
+ : undefined;
+
+ const getDatasetsList = () => {
+ SupersetClient.get({
+ endpoint: `/api/v1/dataset/?q=${queryParams}`,
+ })
+ .then(({ json }) => {
+ json.result.map((dataset: any) =>
+ setLinkedDatasets(linkedDatasets => [...linkedDatasets, dataset]),
+ );
+ })
+ .catch(error =>
+ console.log('There was an error fetching dataset', error),
Review Comment:
Please see other comment about using logging utility
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/Footer/index.tsx:
##########
@@ -113,7 +115,11 @@ function Footer({
<Button onClick={cancelButtonOnClick}>Cancel</Button>
<Button
buttonStyle="primary"
- disabled={!datasetObject?.table_name || !hasColumns}
+ disabled={
Review Comment:
This is a nit, but since this disabled check is getting kind of long it may be worth defining a variable doing this check before the return JSX block which will make the code easier to debug in future and keep the actual JSX portion cleaner.
Example:
```
const disabled = !datasetObject?.table_name ||
!hasColumns ||
linkedDatasets?.includes(datasetObject?.table_name)
return (
<>
<Button onClick={cancelButtonOnClick}>Cancel</Button>
<Button
buttonStyle="primary"
disabled
tooltip={!datasetObject?.table_name ? tooltipText : undefined}
onClick={onSave}
>
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/index.tsx:
##########
@@ -73,6 +75,38 @@ export default function AddDataset() {
Reducer<Partial<DatasetObject> | null, DSReducerActionType>
>(datasetReducer, null);
const [hasColumns, setHasColumns] = useState(false);
+ const [linkedDatasets, setLinkedDatasets] = useState<DatasetObject[]>([]);
+ const linkedDatasetNames = linkedDatasets.map(dataset => dataset.table_name);
+ const encodedSchema = dataset?.schema
+ ? encodeURIComponent(dataset?.schema)
+ : undefined;
+
+ const queryParams = dataset?.schema
+ ? rison.encode_uri({
+ filters: [
+ { col: 'schema', opr: 'eq', value: encodedSchema },
+ { col: 'sql', opr: 'dataset_is_null_or_empty', value: '!t' },
+ ],
+ })
+ : undefined;
+
+ const getDatasetsList = () => {
+ SupersetClient.get({
+ endpoint: `/api/v1/dataset/?q=${queryParams}`,
+ })
+ .then(({ json }) => {
+ json.result.map((dataset: any) =>
+ setLinkedDatasets(linkedDatasets => [...linkedDatasets, dataset]),
+ );
+ })
+ .catch(error =>
+ console.log('There was an error fetching dataset', error),
+ );
+ };
+
+ useEffect(() => {
+ if (dataset?.schema) getDatasetsList();
Review Comment:
Although JavaScript allows for omitting curly braces around one line conditionals and functions, over the years it has become a widely recommended practice to always include curly braces for following reasons:
1. It is a common area bug bugs to appear when refactoring is done from single line to multiple lines
2. It can make code ambiguous based on line spacing
` if (myVar > 3) myVar = 0;
doSomething(myVar);
`
The above code can be confusing to identify if the author intended to always run doSomething(myVar); after the if statement, or they made a code mistake and only want that to run in context of the if statement and forgot to add braces. This can get a lot more confusing than this simple example when this syntax gets used frequently.
4. Later if additional logic is added the curly braces will need to be added back. If dev forgets this the logic will run, but second line with run outside of the conditional
5. Because it has been recommended to be avoided, this syntax is not common to see in JavasScript code and some developers may be confused by and /or misinterpret the code and change / refactor incorrectly.
Many tools have rules defined to track this syntax as a bug, for example: This is a common SonarQube rule: https://3layer.com.br/sonar/rules/show/javascript:CurlyBraces?layout=false
Consistency, code clarity, and avoiding bugs typically trumps saving extra characters. This may be worth discussing adding as a lint rule with wider group
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eric-briscoe commented on a diff in pull request #22136: feat: Flow for tables that already have a dataset
Posted by GitBox <gi...@apache.org>.
eric-briscoe commented on code in PR #22136:
URL: https://github.com/apache/superset/pull/22136#discussion_r1025798342
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/DatasetPanel/DatasetPanel.tsx:
##########
@@ -123,6 +128,24 @@ const StyledTable = styled(Table)`
right: 0;
`;
+const StyledAlert = styled(Alert)`
Review Comment:
Related to coment on line 20, we shuld repelase use of supertThem with:
`const StyledAlert = styled(Alert)(
({ theme }) => `
and replace all the `supersetTheme` with `theme`
for example:
`border: 1px solid ${theme.colors.info.base};
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/DatasetPanel/DatasetPanel.tsx:
##########
@@ -19,10 +19,12 @@
import React from 'react';
import { supersetTheme, t, styled } from '@superset-ui/core';
Review Comment:
@lyndsiWilliams I missed this when we originally worked on DatasetPanel but using supersetTheme directly is an anti-pattern. We should remove this import, and instead bring in useTheme
`import { useTheme, t, styled } from '@superset-ui/core';`
At line 131-148, and line 288, anywhere else we directly use supersetTheme should be changed.
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/DatasetPanel/DatasetPanel.tsx:
##########
@@ -217,17 +273,23 @@ const DatasetPanel = ({
return (
<>
{tableName && (
- <StyledHeader
- position={
- !loading && hasColumns ? EPosition.RELATIVE : EPosition.ABSOLUTE
- }
- title={tableName || ''}
- >
- {tableName && (
- <Icons.Table iconColor={supersetTheme.colors.grayscale.base} />
- )}
- {tableName}
- </StyledHeader>
+ <>
+ {linkedDatasetNames?.includes(tableName) &&
+ renderExistingDatasetAlert(
+ linkedDatasets?.find(dataset => dataset.table_name === tableName),
+ )}
+ <StyledHeader
+ position={
+ !loading && hasColumns ? EPosition.RELATIVE : EPosition.ABSOLUTE
+ }
+ title={tableName || ''}
+ >
+ {tableName && (
+ <Icons.Table iconColor={supersetTheme.colors.grayscale.base} />
Review Comment:
This is where I commented earlier use of supersetTheme directly is an anit pattern we did not catch in initial creation of this file.
Above in the functional component we should have
`const theme = useTheme();`
Then here use
`<Icons.Table` iconColor={theme.colors.grayscale.base} />`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] lyndsiWilliams commented on a diff in pull request #22136: feat: Flow for tables that already have a dataset
Posted by GitBox <gi...@apache.org>.
lyndsiWilliams commented on code in PR #22136:
URL: https://github.com/apache/superset/pull/22136#discussion_r1028720164
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/index.tsx:
##########
@@ -73,6 +75,38 @@ export default function AddDataset() {
Reducer<Partial<DatasetObject> | null, DSReducerActionType>
>(datasetReducer, null);
const [hasColumns, setHasColumns] = useState(false);
+ const [linkedDatasets, setLinkedDatasets] = useState<DatasetObject[]>([]);
+ const linkedDatasetNames = linkedDatasets.map(dataset => dataset.table_name);
+ const encodedSchema = dataset?.schema
+ ? encodeURIComponent(dataset?.schema)
+ : undefined;
+
+ const queryParams = dataset?.schema
+ ? rison.encode_uri({
+ filters: [
+ { col: 'schema', opr: 'eq', value: encodedSchema },
+ { col: 'sql', opr: 'dataset_is_null_or_empty', value: '!t' },
+ ],
+ })
+ : undefined;
+
+ const getDatasetsList = () => {
Review Comment:
Moved the API call into a hook in [`this commit`](https://github.com/apache/superset/pull/22136/commits/ad13ff4dbb6c72d647397d3b2d106c6fc3dbbe2c).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] lyndsiWilliams commented on a diff in pull request #22136: feat: Flow for tables that already have a dataset
Posted by GitBox <gi...@apache.org>.
lyndsiWilliams commented on code in PR #22136:
URL: https://github.com/apache/superset/pull/22136#discussion_r1028713541
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/index.tsx:
##########
@@ -73,6 +75,38 @@ export default function AddDataset() {
Reducer<Partial<DatasetObject> | null, DSReducerActionType>
>(datasetReducer, null);
const [hasColumns, setHasColumns] = useState(false);
+ const [linkedDatasets, setLinkedDatasets] = useState<DatasetObject[]>([]);
+ const linkedDatasetNames = linkedDatasets.map(dataset => dataset.table_name);
+ const encodedSchema = dataset?.schema
+ ? encodeURIComponent(dataset?.schema)
+ : undefined;
+
+ const queryParams = dataset?.schema
+ ? rison.encode_uri({
+ filters: [
+ { col: 'schema', opr: 'eq', value: encodedSchema },
+ { col: 'sql', opr: 'dataset_is_null_or_empty', value: '!t' },
+ ],
+ })
+ : undefined;
+
+ const getDatasetsList = () => {
+ SupersetClient.get({
+ endpoint: `/api/v1/dataset/?q=${queryParams}`,
+ })
+ .then(({ json }) => {
+ json.result.map((dataset: any) =>
+ setLinkedDatasets(linkedDatasets => [...linkedDatasets, dataset]),
+ );
+ })
+ .catch(error =>
+ console.log('There was an error fetching dataset', error),
+ );
+ };
+
+ useEffect(() => {
+ if (dataset?.schema) getDatasetsList();
Review Comment:
Oh wow I didn't realize this was so risky! I added brackets in [`this commit`](https://github.com/apache/superset/pull/22136/commits/86549660bd4d6ac53e0b7723ab3e9370e37d2a28#diff-91bd0e303322c58c459c6d51e9182cd0e03f865ab1c7ca849df92da84752f9b6R108-R110) and will avoid this in the future.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] lyndsiWilliams commented on a diff in pull request #22136: feat: Flow for tables that already have a dataset
Posted by GitBox <gi...@apache.org>.
lyndsiWilliams commented on code in PR #22136:
URL: https://github.com/apache/superset/pull/22136#discussion_r1028361120
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/index.tsx:
##########
@@ -73,6 +75,38 @@ export default function AddDataset() {
Reducer<Partial<DatasetObject> | null, DSReducerActionType>
>(datasetReducer, null);
const [hasColumns, setHasColumns] = useState(false);
+ const [linkedDatasets, setLinkedDatasets] = useState<DatasetObject[]>([]);
+ const linkedDatasetNames = linkedDatasets.map(dataset => dataset.table_name);
+ const encodedSchema = dataset?.schema
+ ? encodeURIComponent(dataset?.schema)
+ : undefined;
+
+ const queryParams = dataset?.schema
+ ? rison.encode_uri({
+ filters: [
+ { col: 'schema', opr: 'eq', value: encodedSchema },
+ { col: 'sql', opr: 'dataset_is_null_or_empty', value: '!t' },
+ ],
+ })
+ : undefined;
+
+ const getDatasetsList = () => {
+ SupersetClient.get({
+ endpoint: `/api/v1/dataset/?q=${queryParams}`,
+ })
+ .then(({ json }) => {
+ json.result.map((dataset: any) =>
+ setLinkedDatasets(linkedDatasets => [...linkedDatasets, dataset]),
Review Comment:
When I initially set it to the entire response, it was only setting one dataset at a time for some reason. Looping and adding forces it to make an array of the datasets.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] github-actions[bot] commented on pull request #22136: feat: Flow for tables that already have a dataset
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #22136:
URL: https://github.com/apache/superset/pull/22136#issuecomment-1323839199
@AAfghahi Ephemeral environment spinning up at http://52.42.126.171:8080. Credentials are `admin`/`admin`. Please allow several minutes for bootstrapping and startup.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] AAfghahi commented on a diff in pull request #22136: feat: Flow for tables that already have a dataset
Posted by GitBox <gi...@apache.org>.
AAfghahi commented on code in PR #22136:
URL: https://github.com/apache/superset/pull/22136#discussion_r1029703633
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/DatasetPanel/DatasetPanel.tsx:
##########
@@ -168,15 +202,52 @@ export interface IDatasetPanelProps {
* Boolean indicating if the component is in a loading state
*/
loading: boolean;
+ datasets?: DatasetObject[] | undefined;
Review Comment:
Makes sense!
Does this need to be a list or can it be boolean? From my, limited, understanding of this feature you're seeing if there already is a dataset linked, so we don't need to be passing in a list of datasets, we could just check to see if there is an associated one.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] lyndsiWilliams commented on pull request #22136: feat: Flow for tables that already have a dataset
Posted by GitBox <gi...@apache.org>.
lyndsiWilliams commented on PR #22136:
URL: https://github.com/apache/superset/pull/22136#issuecomment-1316225405
/testenv up
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org