fix: can't sync temporal flag on virtual table #19366

zhaoyongjie · 2022-03-25T10:58:03Z

SUMMARY

Currently, the virtual table is unable to sync is temporal flag. This is due to the SupersetResultSet response is_date instead of is_dttm. We should keep the physical table and virtual table have the same interface.

In this PR also introduce a new type superset.superset_typing.ResultSetColumnType to guarantee type checking.

I will address a separate PR to finish PhysicalColumnType.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before

be.unable.to.sync.temporal.mov

TESTING INSTRUCTIONS

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-03-26T05:28:37Z

Codecov Report

Merging #19366 (964f87e) into master (eab9388) will decrease coverage by 0.01%.
The diff coverage is 95.23%.

❗ Current head 964f87e differs from pull request most recent head e6b5307. Consider uploading reports for the commit e6b5307 to get more accurate results

@@            Coverage Diff             @@
##           master   #19366      +/-   ##
==========================================
- Coverage   66.58%   66.56%   -0.02%     
==========================================
  Files        1676     1676              
  Lines       64176    64186      +10     
  Branches     6525     6525              
==========================================
- Hits        42732    42728       -4     
- Misses      19745    19759      +14     
  Partials     1699     1699

Flag	Coverage Δ
javascript	`51.31% <ø> (ø)`
mysql	`?`

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

Impacted Files	Coverage Δ
...frontend/src/SqlLab/components/ResultSet/index.tsx	`50.49% <ø> (ø)`
superset-frontend/src/SqlLab/fixtures.ts	`100.00% <ø> (ø)`
superset/views/core.py	`76.54% <ø> (-0.46%)`	⬇️
superset/db_engine_specs/presto.py	`89.00% <90.00%> (-0.12%)`	⬇️
superset/connectors/sqla/models.py	`89.31% <100.00%> (ø)`
superset/connectors/sqla/utils.py	`94.56% <100.00%> (+0.05%)`	⬆️
superset/db_engine_specs/base.py	`88.33% <100.00%> (+0.01%)`	⬆️
superset/result_set.py	`98.38% <100.00%> (ø)`
superset/superset_typing.py	`98.11% <100.00%> (+0.15%)`	⬆️
superset/common/utils/dataframe_utils.py	`85.71% <0.00%> (-7.15%)`	⬇️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eab9388...e6b5307. Read the comment docs.

jinghua-qa · 2022-03-29T04:26:28Z

/testenv up

github-actions · 2022-03-29T04:34:53Z

@jinghua-qa Ephemeral environment spinning up at http://34.217.79.231:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

jinghua-qa · 2022-03-30T20:42:35Z

I tested the PR locally, is temporal can be sync as expected, but when i save the changes, i saw error msg

Screen.Recording.2022-03-30.at.1.34.07.PM.mov

zhaoyongjie · 2022-03-31T02:36:45Z

I tested the PR locally, is temporal can be sync as expected, but when i save the changes, i saw error msg
Screen.Recording.2022-03-30.at.1.34.07.PM.mov

This is an unrelated issue with current PR. I have opened a separated PR to fix it.

jinghua-qa

Work as expected, LGTM

zhaoyongjie · 2022-04-01T05:58:21Z

superset/result_set.py

+            column: ResultSetColumnType = {
                "name": col.name,
                "type": db_type_str,
-                "is_date": self.is_temporal(db_type_str),
+                "is_dttm": self.is_temporal(db_type_str),


We should keep a consistent interface between the physical dataset and the virtual dataset.

villebro

Love it, thanks for this fix + nice cleanup! In the long run I feel we should start moving types from superset/superset_typing.py into the packages where they are most relevant (see e.g. superset/key_value/types.py), but that can be done in a follow up (e.g. the one planned for physical columns)

zhaoyongjie · 2022-04-01T08:35:58Z

Love it, thanks for this fix + nice cleanup! In the long run I feel we should start moving types from superset/superset_typing.py into the packages where they are most relevant (see e.g. superset/key_value/types.py), but that can be done in a follow up (e.g. the one planned for physical columns)

sounds good! I will follow this pattern in future.

github-actions · 2022-04-01T08:41:15Z

Ephemeral environment shutdown and build artifacts deleted.

(cherry picked from commit d954c3d)

sadpandajoe · 2022-04-06T18:17:36Z

🏷️ preset:2022.13

pull-request-size bot added the size/L label Mar 25, 2022

zhaoyongjie changed the title ~~fix: can't sync is_dttm in physical table and change is_date to is_dttm on resultSet~~ fix: can't sync is_dttm on virtual table and change is_date to is_dttm on resultSet Mar 25, 2022

zhaoyongjie changed the title ~~fix: can't sync is_dttm on virtual table and change is_date to is_dttm on resultSet~~ fix: can't sync is_dttm on virtual table and change is_date to is_dttm Mar 25, 2022

zhaoyongjie changed the title ~~fix: can't sync is_dttm on virtual table and change is_date to is_dttm~~ fix: can't sync temporal flag on virtual table Mar 25, 2022

zhaoyongjie requested review from villebro, john-bodley and a team March 28, 2022 01:47

jinghua-qa added the need:qa-review Requires QA review label Mar 29, 2022

zhaoyongjie force-pushed the fix_vds_sync branch from ea924e1 to d3cc6d6 Compare March 30, 2022 09:10

jinghua-qa approved these changes Mar 31, 2022

View reviewed changes

zhaoyongjie added 8 commits March 31, 2022 16:50

wip

6e9b557

update UT

ecc57fb

update UT

523ef2d

wip

b03ea30

update

e47a8ed

fix lint

a257938

fix Presto UT

62fda63

fix conflicts

40c48e8

zhaoyongjie force-pushed the fix_vds_sync branch from d3cc6d6 to 40c48e8 Compare March 31, 2022 08:51

Merge branch 'master' into fix_vds_sync

e6b5307

zhaoyongjie commented Apr 1, 2022

View reviewed changes

villebro approved these changes Apr 1, 2022

View reviewed changes

zhaoyongjie merged commit d954c3d into apache:master Apr 1, 2022

villebro added the lts-v1 label Apr 1, 2022

sadpandajoe pushed a commit to preset-io/superset that referenced this pull request Apr 1, 2022

fix: can't sync temporal flag on virtual table (apache#19366)

acb3439

(cherry picked from commit d954c3d)

villebro pushed a commit that referenced this pull request Apr 3, 2022

fix: can't sync temporal flag on virtual table (#19366)

468730a

(cherry picked from commit d954c3d)

villebro pushed a commit that referenced this pull request Apr 4, 2022

fix: can't sync temporal flag on virtual table (#19366)

3ff9cde

(cherry picked from commit d954c3d)

superset-github-bot bot added the preset:2022.13 label Apr 6, 2022

philipher29 pushed a commit to ValtechMobility/superset that referenced this pull request Jun 9, 2022

fix: can't sync temporal flag on virtual table (apache#19366)

33e3a73

mistercrunch added 🍒 1.5.0 🍒 1.5.1 🍒 1.5.2 🍒 1.5.3 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.0.0 labels Mar 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: can't sync temporal flag on virtual table #19366

fix: can't sync temporal flag on virtual table #19366

zhaoyongjie commented Mar 25, 2022 •

edited

Loading

codecov bot commented Mar 26, 2022 •

edited

Loading

jinghua-qa commented Mar 29, 2022

github-actions bot commented Mar 29, 2022

jinghua-qa commented Mar 30, 2022

zhaoyongjie commented Mar 31, 2022

jinghua-qa left a comment

zhaoyongjie Apr 1, 2022 •

edited

Loading

villebro left a comment

zhaoyongjie commented Apr 1, 2022

github-actions bot commented Apr 1, 2022

sadpandajoe commented Apr 6, 2022

fix: can't sync temporal flag on virtual table #19366

fix: can't sync temporal flag on virtual table #19366

Conversation

zhaoyongjie commented Mar 25, 2022 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

codecov bot commented Mar 26, 2022 • edited Loading

Codecov Report

jinghua-qa commented Mar 29, 2022

github-actions bot commented Mar 29, 2022

jinghua-qa commented Mar 30, 2022

zhaoyongjie commented Mar 31, 2022

jinghua-qa left a comment

Choose a reason for hiding this comment

zhaoyongjie Apr 1, 2022 • edited Loading

Choose a reason for hiding this comment

villebro left a comment

Choose a reason for hiding this comment

zhaoyongjie commented Apr 1, 2022

github-actions bot commented Apr 1, 2022

sadpandajoe commented Apr 6, 2022

zhaoyongjie commented Mar 25, 2022 •

edited

Loading

codecov bot commented Mar 26, 2022 •

edited

Loading

zhaoyongjie Apr 1, 2022 •

edited

Loading