[GitHub] [superset] villebro commented on a change in pull request #14969: fix: remove unused time column when update dataset

[GitBox <gi...@apache.org>]

villebro commented on a change in pull request #14969:
URL: https://github.com/apache/superset/pull/14969#discussion_r647332159



##########
File path: superset-frontend/src/explore/components/controls/DatasourceControl/index.jsx
##########
@@ -117,6 +118,15 @@ class DatasourceControl extends React.PureComponent {
 
   onDatasourceSave(datasource) {
     this.props.actions.setDatasource(datasource);
+    // remove time column in the form_data
+    const timeCol = this.props.form_data?.granularity_sqla; // eslint-disable-line camelcase

Review comment:
       As there are multiple references to snake case variables, maybe we could just disable it for the whole file:
   ```js
   /* eslint-disable-line camelcase */
   ```

##########
File path: superset-frontend/src/explore/components/controls/DatasourceControl/index.jsx
##########
@@ -117,6 +118,15 @@ class DatasourceControl extends React.PureComponent {
 
   onDatasourceSave(datasource) {
     this.props.actions.setDatasource(datasource);
+    // remove time column in the form_data
+    const timeCol = this.props.form_data?.granularity_sqla; // eslint-disable-line camelcase
+    const { columns } = this.props.datasource;
+    if (
+      datasource.type === 'table' &&
+      !columns.find(({ column_name }) => column_name === timeCol)?.is_dttm // eslint-disable-line camelcase
+    ) {
+      this.props.actions.setControlValue('granularity_sqla', null);
+    }

Review comment:
       When the time column control is empty and a column is made `is_dttm`, the time column control stays empty, despite being required (not possible to remove value once set). Would it make sense to default to the first temporal column if the value is unset? Something like this:
   ```diff
   diff --git a/superset-frontend/src/explore/components/controls/DatasourceControl/index.jsx b/superset-frontend/src/explore/components/controls/DatasourceControl/index.jsx
   index 873067565..44452fb0c 100644
   --- a/superset-frontend/src/explore/components/controls/DatasourceControl/index.jsx
   +++ b/superset-frontend/src/explore/components/controls/DatasourceControl/index.jsx
   @@ -121,12 +121,20 @@ class DatasourceControl extends React.PureComponent {
        // remove time column in the form_data
        const timeCol = this.props.form_data?.granularity_sqla; // eslint-disable-line camelcase
        const { columns } = this.props.datasource;
   +    const firstDttmCol = columns.find(column => column.is_dttm);
        if (
          datasource.type === 'table' &&
   +      timeCol &&
          !columns.find(({ column_name }) => column_name === timeCol)?.is_dttm // eslint-disable-line camelcase
        ) {
          this.props.actions.setControlValue('granularity_sqla', null);
   +    } else if (datasource.type === 'table' && !timeCol && firstDttmCol) {
   +      this.props.actions.setControlValue(
   +        'granularity_sqla',
   +        firstDttmCol.column_name,
   +      );
        }
   +
        if (this.props.onDatasourceSave) {
          this.props.onDatasourceSave(datasource);
        }
   ```




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

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