Skip to content
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: Time Offset in SQLite and refine logic in Date Type conversion #21378

Merged
merged 15 commits into from
Sep 16, 2022

Conversation

zhaoyongjie
Copy link
Member

@zhaoyongjie zhaoyongjie commented Sep 8, 2022

SUMMARY

Currently, The Time Offset can't be used in SQLite as datasouce, it's because the Pandas + SQLAlchemy query from SQLite will return datetime column as object rather than datetime.

... from sqlalchemy import create_engine
... import pandas as pd

... sqlite_engine = create_engine("sqlite:////some-path/examples_sqlite.db")
... sql_sqlite = '''
SELECT * FROM main.birth_names_sqlite
limit 1
'''
... df = pd.read_sql(sql_sqlite, sqlite_engine)
... df.dtypes
...
ds        object
gender    object
name      object
num        int64
state     object
dtype: object

This PR uses the original normalize_dttm_col function but adds a new argument for it. the normalize_dttm_col will convert the old __timestamp column or the new BASE_AXIS to the Timestamp series in pandas.

In addition, added a new unit test for Time Grain and Time Offset in the QueryObject.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

After

FF Enabled
image

FF Disabled

image

Before

FF Enabled
image

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

@zhaoyongjie zhaoyongjie changed the title fix: Time Grain in SQLite and Time Shift decopule from granularity [wip]fix: Time Grain in SQLite and Time Shift decopule from granularity Sep 8, 2022
@codecov
Copy link

codecov bot commented Sep 8, 2022

Codecov Report

Merging #21378 (4387f29) into master (1c0bff3) will increase coverage by 0.01%.
The diff coverage is 93.47%.

@@            Coverage Diff             @@
##           master   #21378      +/-   ##
==========================================
+ Coverage   66.67%   66.68%   +0.01%     
==========================================
  Files        1791     1791              
  Lines       68458    68476      +18     
  Branches     7270     7270              
==========================================
+ Hits        45642    45661      +19     
+ Misses      20955    20954       -1     
  Partials     1861     1861              
Flag Coverage Δ
hive 53.08% <71.73%> (+0.04%) ⬆️
mysql 78.20% <91.30%> (+0.01%) ⬆️
postgres 78.27% <91.30%> (+0.01%) ⬆️
presto 52.98% <71.73%> (+0.04%) ⬆️
python 81.41% <93.47%> (+0.01%) ⬆️
sqlite 76.76% <91.30%> (+0.01%) ⬆️
unit 51.05% <36.95%> (+0.01%) ⬆️

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

Impacted Files Coverage Δ
superset/viz.py 58.17% <ø> (ø)
superset/common/query_context_processor.py 88.51% <90.90%> (-0.29%) ⬇️
superset/utils/core.py 90.32% <94.28%> (+0.38%) ⬆️

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

@zhaoyongjie zhaoyongjie force-pushed the fix/fix_datetime_sqlite branch from 69f2687 to 5df12e0 Compare September 14, 2022 08:37
@zhaoyongjie zhaoyongjie changed the title [wip]fix: Time Grain in SQLite and Time Shift decopule from granularity [wip]fix: Time Grain and Time Offset in SQLite Sep 14, 2022
@zhaoyongjie zhaoyongjie changed the title [wip]fix: Time Grain and Time Offset in SQLite fix: Time Grain and Time Offset in SQLite Sep 14, 2022
@pull-request-size pull-request-size bot added size/L and removed size/M labels Sep 14, 2022
@zhaoyongjie zhaoyongjie changed the title fix: Time Grain and Time Offset in SQLite fix: Time Offset in SQLite Sep 14, 2022
Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a few comments

superset/utils/core.py Outdated Show resolved Hide resolved
superset/utils/core.py Outdated Show resolved Hide resolved
superset/common/query_context_processor.py Outdated Show resolved Hide resolved
Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested to work as expected. I left one last comment that I'd be curious to hear your thoughts on, but this is probably already ok for merging to unblock the fix which is more serious than the remaining comment.

superset/common/query_context_processor.py Outdated Show resolved Hide resolved
@zhaoyongjie zhaoyongjie changed the title fix: Time Offset in SQLite fix: Time Offset in SQLite and refactor logic in Date type conversion Sep 15, 2022
@zhaoyongjie zhaoyongjie changed the title fix: Time Offset in SQLite and refactor logic in Date type conversion fix: Time Offset in SQLite and refine logic in Date type conversion Sep 15, 2022
@zhaoyongjie zhaoyongjie changed the title fix: Time Offset in SQLite and refine logic in Date type conversion fix: Time Offset in SQLite and refine logic in Date Type conversion Sep 15, 2022
Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for all the iterations on this one!

@zhaoyongjie zhaoyongjie force-pushed the fix/fix_datetime_sqlite branch from e1e8daf to d3518c2 Compare September 16, 2022 03:09
@zhaoyongjie zhaoyongjie merged commit 2dfcba0 into apache:master Sep 16, 2022
kgopal492 pushed a commit to kgopal492/superset that referenced this pull request Feb 1, 2023
kgopal492 added a commit to pinterest/superset that referenced this pull request Feb 2, 2023
* fix(chart): fix time comparison error (#13)

* fix: use dttm_col name to normalize dttm_col to fix time comparison error

* Revert "fix(chart): fix time comparison error (#13)"

This reverts commit b3dacc1.

* fix: Time Offset in SQLite and refine logic in Date Type conversion (apache#21378)

* fix another bug in cherry-pick

* fix test

---------

Co-authored-by: Yongjie Zhao <yongjiezhao@apache.org>
@mistercrunch 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/L 🚢 2.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants