-
Notifications
You must be signed in to change notification settings - Fork 321
Fix: normalize datetime values to strings in unit tests #4696
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
Conversation
df850d4 to
bd49e17
Compare
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.
Don't really have a concrete comment, unless I'm downplaying the power of str(x), my hunch is this will cause false-negative comparisons again but I don't have a better solution at hand.
| query: | ||
| rows: | ||
| - id: id1 | ||
| agg_timestamp_col: ["2024-01-02T15:00:00.000000"] |
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.
Would the same output comparison work properly if we compared an equivalent datetime value from a different string? E.g `2024-01-02T15:00:00.00" or one based on a different timezone that results in the same time?
Ideally we'd normalize all datetime types into a common representation at a common timezone (?)
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 think so, there's a test for this:
sqlmesh/tests/core/test_test.py
Line 1220 in f87d3f0
| execution_time: "2023-01-01 12:05:03+00:00" |
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.
This looks like it's on the same timezone from a quick look, I was referring to a scenario such as :
inputs:
...
time_col: "2020-01-01 05:00:00+05"
outputs:
...
time_col: "2020-01-01 00:00:00+00"
this test should be correct, right? But if it's a string comparison (or at best, normalized to a certain precision with zeroes) then it'd fail
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 don't think that we encourage specifying time zones in the timestamp values (in the sense that it's unsupported).
The +zone syntax is only mentioned in relation to the execution_time variable in the docs, and I think that was intentional at the time.
Fixes the issue discussed in this Slack thread: https://tobiko-data.slack.com/archives/C044BRE5W4S/p1749046530513469