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: Handle python_date_format in ExploreMixin #24068

Merged
merged 1 commit into from
May 16, 2023

Conversation

john-bodley
Copy link
Member

@john-bodley john-bodley commented May 15, 2023

SUMMARY

This PR fixes an issue introduced in #20938 however it didn't surface until #22853 which resulted in a somewhat bad correctness regression as the resulting generated SQL was incorrect when filtering the temporal column, i.e., rather than queries being generated of the form,

WHERE
    ds >= '2023-01-01'

they were being generated as:

WHERE
    ds >= '2023-01-01 00:00:00.000000'

which due to lexicographical ordering would wrongfully exclude any records where ds = 2023-01-01.

The TL;DR is #20938 replicated logic from the TableColumn class however it did not include the logic which adhered to the Python date/time format meaning that columns which weren't handled by the DB engine spec convert_dttm method were cast to an ISO 8601 formatted string adhering to the Y-%m-%d %H:%M:%S.%f format.

This PR adds the missing logic and removes the now unused (per #22853) TableColumn methods to help improve the code hygiene and readability—having two or more instances of the same function makes the code very difficult to grok. @hughhhh I would suggest you perform a second pass to make sure all the legacy logic which was ported to the ExploreMixin has been removed.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

Added unit tests which would have hopefully helped discovered the issue.

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

@john-bodley
Copy link
Member Author

john-bodley commented May 15, 2023

Snap @betodealmeida this is the same logic as #24062. I've rebased my logic on your PR. I also added some unit tests as I realized these were missing.

@john-bodley john-bodley marked this pull request as ready for review May 15, 2023 22:57
@john-bodley john-bodley changed the title fix: Convert datetime object to a SQL expression string fix: Handle python_date_format in ExploreMixin May 15, 2023
@john-bodley john-bodley force-pushed the john-bodley-fix-20938 branch 2 times, most recently from c4db792 to 6498420 Compare May 15, 2023 23:13
"""Convert datetime object to a SQL expression string"""

sql = (
self.db_engine_spec.convert_dttm(col_type, dttm, db_extra=None)
if col_type
self.db_engine_spec.convert_dttm(col.type, dttm, db_extra=col.db_extra)
Copy link
Member Author

Choose a reason for hiding this comment

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

In the TableColumn.dttm_sql_literal method this the db_extra is set. I gather it might have been set to None previously for connivence/ease.

@betodealmeida
Copy link
Member

Snap @betodealmeida this is the same logic as #24062. I've rebased my logic on your PR. I also added some unit tests as I realized these were missing.

Sorry for not adding tests, this was blocking a release and we needed a quick fix.

@codecov
Copy link

codecov bot commented May 15, 2023

Codecov Report

Merging #24068 (ce17c70) into master (6baa552) will increase coverage by 0.02%.
The diff coverage is 100.00%.

❗ Current head ce17c70 differs from pull request most recent head 3e36553. Consider uploading reports for the commit 3e36553 to get more accurate results

@@            Coverage Diff             @@
##           master   #24068      +/-   ##
==========================================
+ Coverage   68.22%   68.25%   +0.02%     
==========================================
  Files        1942     1942              
  Lines       75215    75192      -23     
  Branches     8145     8145              
==========================================
+ Hits        51318    51322       +4     
+ Misses      21812    21785      -27     
  Partials     2085     2085              
Flag Coverage Δ
hive 53.19% <100.00%> (+0.02%) ⬆️
mysql 78.96% <100.00%> (+0.04%) ⬆️
postgres 79.03% <100.00%> (+0.04%) ⬆️
presto 53.12% <100.00%> (+0.02%) ⬆️
python 82.83% <100.00%> (+0.06%) ⬆️
sqlite 77.55% <100.00%> (+0.04%) ⬆️
unit 53.11% <100.00%> (+0.07%) ⬆️

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

Impacted Files Coverage Δ
superset/connectors/sqla/models.py 89.77% <ø> (+2.53%) ⬆️
superset/models/helpers.py 69.73% <100.00%> (+0.61%) ⬆️

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

Copy link
Member

@michael-s-molina michael-s-molina left a comment

Choose a reason for hiding this comment

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

LGTM

@john-bodley john-bodley merged commit 2f0caf8 into apache:master May 16, 2023
29 checks passed
@john-bodley john-bodley deleted the john-bodley-fix-20938 branch May 16, 2023 13:54
@john-bodley john-bodley mentioned this pull request May 18, 2023
9 tasks
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 3.0.0 labels Mar 8, 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 🚢 3.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants