-
Notifications
You must be signed in to change notification settings - Fork 13.9k
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(sqla): Normalize prequery result type #17312
fix(sqla): Normalize prequery result type #17312
Conversation
4f9ceff
to
09c24dd
Compare
09c24dd
to
02eb304
Compare
Codecov Report
@@ Coverage Diff @@
## master #17312 +/- ##
=======================================
Coverage 77.09% 77.09%
=======================================
Files 1037 1037
Lines 55646 55650 +4
Branches 7607 7607
=======================================
+ Hits 42900 42905 +5
+ Misses 12496 12495 -1
Partials 250 250
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
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.
I agree with the functional change of using the native column type as the basis of the conversion, thanks for the improvement. However, AFAIK the walrus operator will break Python 3.7 compatibility, so we should not use it before we remove explicit support for that version from setup.py
.
superset/connectors/sqla/models.py
Outdated
column_ = columns_by_name[dimension] | ||
|
||
if column_.type and column_.is_temporal and isinstance(value, str): | ||
if result := self.db_engine_spec.convert_dttm( |
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.
As we still support Python 3.7, I don't think we can start using the walrus operator quite yet
02eb304
to
734de96
Compare
@villebro thanks for the review. I've addressed your comments. |
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 👍
Co-authored-by: John Bodley <john.bodley@airbnb.com>
SUMMARY
This PR fixes a regression introduced in #16795 as not all temporal columns are of type
TIMESTAMP
. The solution (as outlined in #16795 (comment)) uses the SQLA connector to convert the date time appropriately.TESTING INSTRUCTIONS
Added unit tests and tested locally.
ADDITIONAL INFORMATION