-
Notifications
You must be signed in to change notification settings - Fork 13.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: remove unused time column when update dataset #14969
Conversation
Codecov Report
@@ Coverage Diff @@
## master #14969 +/- ##
==========================================
- Coverage 77.08% 76.84% -0.24%
==========================================
Files 984 984
Lines 51787 51796 +9
Branches 7036 7041 +5
==========================================
- Hits 39920 39804 -116
- Misses 11642 11767 +125
Partials 225 225
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
Awesome PR description! thanks for the fix 🙏 |
@junlincc Ephemeral environment spinning up at http://52.42.220.175:8080. Credentials are |
ba74605
to
c60d9a8
Compare
Ephemeral environment shutdown and build artifacts deleted. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few minor ideas/comments
superset-frontend/src/explore/components/controls/DatasourceControl/index.jsx
Outdated
Show resolved
Hide resolved
// 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); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 --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);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for your comments! This has the side effect of leaving an unexpected column to do the filtering and grain(in other words, first datetime column depends on the order of the columns in the database.). What do you think about?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done this change. Thanks for the comment!
c60d9a8
to
37c00eb
Compare
37c00eb
to
d6a8a5a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
* fix: remove unused time column when update dataset * fix lint * set dttm to first datatime column
* fix: remove unused time column when update dataset * fix lint * set dttm to first datatime column
* fix: remove unused time column when update dataset * fix lint * set dttm to first datatime column
* fix: remove unused time column when update dataset * fix lint * set dttm to first datatime column
SUMMARY
closes: #14683
superset-ui related PR: apache-superset/superset-ui#1148
Since the early Superset TableSource references to Druid table structure, there was always a time dimension(time partition column) in all tables(as known as, datasource or dataset). But this concept is not suitable for other data warehouses or databases.
There is a
main_dttm_col
field in the superset dataset, service time dimension. Now if a time column is not passed to superset backend, themain_dttm_col
will be used in the time column.This change
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before
before.mp4
After
after.mp4
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION