fix: Default temporal column in Datasource #21857

geido · 2022-10-18T14:05:33Z

SUMMARY

Fixes an issue for which the default temporal column wasn't picked up correctly

BEFORE

188827803-5e994ce4-0413-4e74-a22c-d9221934c378.mov

AFTER

DEV.Superset.2.mp4

TESTING INSTRUCTIONS

Edit a dataset with multiple temporal columns
Disable all temporal columns and save
Make sure the control is now empty
Edit the dataset again and add two temporal columns and make one the default and save
Make sure the default is picked in the control after saving
Edit the dataset again and remove the default from temporal columns and save
The control should have picked the first available temporal column

ADDITIONAL INFORMATION

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

codecov · 2022-10-18T21:44:29Z

Codecov Report

Merging #21857 (18dda9e) into master (df3b5a8) will decrease coverage by 0.11%.
The diff coverage is 88.88%.

@@            Coverage Diff             @@
##           master   #21857      +/-   ##
==========================================
- Coverage   66.85%   66.74%   -0.12%     
==========================================
  Files        1805     1810       +5     
  Lines       69061    71385    +2324     
  Branches     7369     8232     +863     
==========================================
+ Hits        46169    47644    +1475     
- Misses      20995    21703     +708     
- Partials     1897     2038     +141

Flag	Coverage Δ
hive	`52.92% <ø> (ø)`
javascript	`53.66% <88.88%> (+0.45%)`	⬆️
mysql	`78.24% <ø> (ø)`
postgres	`78.30% <ø> (ø)`
presto	`52.82% <ø> (ø)`
python	`81.45% <ø> (ø)`
sqlite	`76.79% <ø> (ø)`
unit	`51.06% <ø> (ø)`

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

Impacted Files	Coverage Δ
...re/components/controls/DatasourceControl/index.jsx	`84.61% <88.88%> (+9.36%)`	⬆️
...ls/DndColumnSelectControl/DndAdhocFilterOption.tsx	`66.66% <0.00%> (-33.34%)`	⬇️
...frontend/src/components/Chart/ChartContextMenu.tsx	`31.81% <0.00%> (-12.19%)`	⬇️
...gins/plugin-chart-echarts/src/Pie/controlPanel.tsx	`16.66% <0.00%> (-11.91%)`	⬇️
.../CRUD/data/database/DatabaseModal/ExtraOptions.tsx	`80.48% <0.00%> (-6.47%)`	⬇️
.../plugin-chart-echarts/src/Timeseries/buildQuery.ts	`66.66% <0.00%> (-4.77%)`	⬇️
...-chart-nvd3/src/vendor/superset/AnnotationTypes.js	`66.66% <0.00%> (-4.77%)`	⬇️
...plore/components/controls/OptionControls/index.tsx	`75.20% <0.00%> (-2.83%)`	⬇️
...e/components/controls/MetricControl/AdhocMetric.js	`93.65% <0.00%> (-2.01%)`	⬇️
...rc/SqlLab/components/ScheduleQueryButton/index.tsx	`18.84% <0.00%> (-1.92%)`	⬇️
... and 65 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

superset-frontend/src/explore/components/controls/DatasourceControl/index.jsx

zhaoyongjie · 2022-10-19T15:40:32Z

superset-frontend/src/explore/components/controls/DatasourceControl/index.jsx

-    const timeCol = this.props.form_data?.granularity_sqla;
    const { columns } = this.props.datasource;
-    const firstDttmCol = columns.find(column => column.is_dttm);
-    if (
-      datasource.type === 'table' &&
-      !columns.find(({ column_name }) => column_name === timeCol)?.is_dttm
-    ) {
-      // set `granularity_sqla` to first datatime column name or null
-      this.props.actions.setControlValue(
-        'granularity_sqla',
-        firstDttmCol ? firstDttmCol.column_name : null,
-      );
+    const timeCol = this.props.form_data?.granularity_sqla;
+    const timeColConf = columns.find(
+      ({ column_name }) => column_name === timeCol,
+    );
+    if (datasource.type === 'table') {
+      // resets new default time col or picks the first available one
+      if (!timeColConf?.is_dttm) {
+        const setDttmCol = getTemporalColumns(this.props.datasource);
+        this.props.actions.setControlValue(
+          'granularity_sqla',
+          setDttmCol.defaultTemporalColumn,
+        );
+      }
    }
+


The getTemporalColumns will return a well-designed data structure so the below snippet will handle it. this logic is same as dndGranularitySqlaControl. notice that the granularity_sqla default value is null.

const {temporalColumns, defaultTemporalColumn} = getTemporalColumns(this.props.datasource) this.props.actions.setControlValue( 'granularity_sqla', defaultTemporalColumn ?? temporalColumns?.[0] ?? null, );

@zhaoyongjie I still need to check if the current time col is null, because I only want to replace the existing value if that is null, not otherwise

@geido timeColConf must be a time column, otherwise it wouldn't be set to granularity_sqla.

@geido i jsut reread this codes. the datasource is new datasource setting, so we should use the new datasouce instead of this.props.datasouce.

const {temporalColumns, defaultTemporalColumn} = getTemporalColumns(datasource) this.props.actions.setControlValue( 'granularity_sqla', defaultTemporalColumn ?? temporalColumns?.[0] ?? null, );

@zhaoyongjie granularity_sqla can indeed return also a column that is not a time column anymore after the user has unselected the "Is temporal" from the dataset. To try, just select a default time column for the time column control, then edit the dataset and keep the column as default but remove the "Is temporal" flag, you will see that granularity_sqla will still return that value as if it was temporal as indeed that's the last value known when saving the dataset.

That's useful because we want to change the control value only when it's empty. This is a product requirement that I confirmed with Kasia as well.

I also found out that the getTemporalColumns util isn't working properly. It's setting as default temporal column also columns that are not temporal anymore. In fact, the datasource.main_dttm_col can hold a column that was kept as default but removed from "Is temporal". I think this requires a fix in the util itself, but for now I am just making an additional check in my code to account for this issue as I don't want to potentially cause troubles to other parts of the application that are using that util.

I made some more improvements to the code but I believe this should be ready to go. I will spin up a testenv if you want to manual test it.

The product requirements for manual testing are the following:

If the control is empty, change it with the new default or the first time column found if no default is there

If the control is not empty, changing the default will only affect the dataset but not the control which will keep the existing value

If the column that is currently used in the control is removed from the dataset as time column and if there is no other time column available, the control should be emptied

Thanks

@geido Thank you for the explanation. I understand what you are saying. This has been a long-time-coming issue in the Superset.
The main_dttm_col is saved in the tables table on the Superset metadata. According to Superset's original design(for Druid native query), each dataset must have a main_dttm_col, it is granularity_sqla in the frontend, AKA time column.

Every column in the datasource is saved in the table_columns table, it has a column is_dttm for is_temporal usages.

the time column query order is

main_dttm_col.

Arbitrary column is in table_columns and is_dttm is true.

The issue should be that even if we unselected the default column's is_dttm attribute, it is still saved in the main_dttm_col column. We should fix this logic --- remove a non-temporal recorded from main_dttm_col, and not just ignore main_dttm_col on the frontend, which would cause inconsistencies between the front and backend.

@zhaoyongjie I understand that but the fix would be out of scope right now. I need to get the fix for the control out first and I can open a separate issue to investigate the core problem that you are describing. Thanks

geido · 2022-10-21T10:56:38Z

/testenv up

github-actions · 2022-10-21T11:08:32Z

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

jinghua-qa

LGTM!

github-actions · 2022-10-25T11:42:43Z

Ephemeral environment shutdown and build artifacts deleted.

geido added 2 commits October 18, 2022 14:30

WIP

5bf6059

Fix and add tests

0dcac0a

pull-request-size bot added the size/L label Oct 18, 2022

geido requested review from zhaoyongjie and jinghua-qa October 18, 2022 16:04

zhaoyongjie reviewed Oct 19, 2022

View reviewed changes

superset-frontend/src/explore/components/controls/DatasourceControl/index.jsx Show resolved Hide resolved

Use util

3fdca22

jinghua-qa added the need:qa-review Requires QA review label Oct 19, 2022

zhaoyongjie reviewed Oct 19, 2022

View reviewed changes

Fix default temporal

18dda9e

zhaoyongjie self-requested a review October 20, 2022 14:01

zhaoyongjie approved these changes Oct 20, 2022

View reviewed changes

jinghua-qa approved these changes Oct 24, 2022

View reviewed changes

geido merged commit fa67315 into master Oct 25, 2022

mistercrunch added the 🚢 2.1.3 label Feb 18, 2024

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

mistercrunch deleted the fix/default-temporal-column branch March 26, 2024 16:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Default temporal column in Datasource #21857

fix: Default temporal column in Datasource #21857

geido commented Oct 18, 2022

codecov bot commented Oct 18, 2022 •

edited

zhaoyongjie Oct 19, 2022 •

edited

geido Oct 20, 2022

zhaoyongjie Oct 20, 2022

zhaoyongjie Oct 20, 2022

geido Oct 20, 2022

zhaoyongjie Oct 20, 2022 •

edited

geido Oct 20, 2022

geido commented Oct 21, 2022

github-actions bot commented Oct 21, 2022

jinghua-qa left a comment

github-actions bot commented Oct 25, 2022

fix: Default temporal column in Datasource #21857

fix: Default temporal column in Datasource #21857

Conversation

geido commented Oct 18, 2022

SUMMARY

BEFORE

AFTER

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

codecov bot commented Oct 18, 2022 • edited

Codecov Report

zhaoyongjie Oct 19, 2022 • edited

Choose a reason for hiding this comment

geido Oct 20, 2022

Choose a reason for hiding this comment

zhaoyongjie Oct 20, 2022

Choose a reason for hiding this comment

zhaoyongjie Oct 20, 2022

Choose a reason for hiding this comment

geido Oct 20, 2022

Choose a reason for hiding this comment

zhaoyongjie Oct 20, 2022 • edited

Choose a reason for hiding this comment

geido Oct 20, 2022

Choose a reason for hiding this comment

geido commented Oct 21, 2022

github-actions bot commented Oct 21, 2022

jinghua-qa left a comment

Choose a reason for hiding this comment

github-actions bot commented Oct 25, 2022

codecov bot commented Oct 18, 2022 •

edited

zhaoyongjie Oct 19, 2022 •

edited

zhaoyongjie Oct 20, 2022 •

edited