fix(clickhouse): time filters use server timezone for toDateTime literals#41579
Conversation
Code Review Agent Run #7aff25Actionable Suggestions - 0Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #41579 +/- ##
==========================================
- Coverage 64.42% 64.41% -0.01%
==========================================
Files 2668 2668
Lines 147183 147186 +3
Branches 33947 33948 +1
==========================================
- Hits 94822 94817 -5
- Misses 50646 50652 +6
- Partials 1715 1717 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Code Review Agent Run #683152Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help |
rusackas
left a comment
There was a problem hiding this comment.
Thanks @innovark37, pinning the toDateTime literal to 'UTC' is the right call given our filter datetimes are UTC-naive, and normalizing tz-aware inputs first is a nice touch. Your rebuttal on the Top-N/prequery thread reads right to me too... that path already leans on the same UTC-naive convention. LGTM!
SUMMARY
Fixes ClickHouse temporal filter SQL generation for deployments where the ClickHouse server/session timezone is not UTC.
Superset computes time filter bounds in UTC, but ClickHouse
toDateTime('YYYY-MM-DD HH:mm:ss')interprets the string in the server/session timezone when no timezone argument is provided. This can shift the effective filter bounds and cause expected rows to be excluded.This change renders ClickHouse
DateTimeliterals with an explicit UTC timezone:BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Setup
Europe/Moscow)Before:
toDateTime('...', )without an explicit timezone:After:
TESTING INSTRUCTIONS
Europe/Moscow.tz_bug_test.events.createdAtis marked as temporal.id,name, andcreatedAt.createdAtas the time column.event 12:30 Moscowis returned.ADDITIONAL INFORMATION