-
Notifications
You must be signed in to change notification settings - Fork 7.1k
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 adding sub-second intervals to DateTime #53309
Conversation
This is an automated comment for commit 690b0e9 with description of existing statuses. It's updated for the latest CI running
|
chassert(false); | ||
return 0; |
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.
maybe throw a logical error in cases like this?
Returning 0 seems strange on something that shouldn't be called + it could make it harder to debug.
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.
Yeah, that sounds better, changing.
Stress test (debug): #53454 |
Wow, a simple |
Maybe this is the culprit: ClickHouse/tests/clickhouse-test Lines 613 to 624 in 16cbda1
|
Ah, turns out the real timezone randomization is in |
Backport #53309 to 23.3: Fix adding sub-second intervals to DateTime
…54983) Co-authored-by: robot-clickhouse <[email protected]>
…54981) Co-authored-by: robot-clickhouse <[email protected]>
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Fixed adding intervals of a fraction of a second to DateTime producing incorrect result.
Closes #45779
Before:
After:
It was a simple bug in AddNanosecondsImpl/etc - it was returning a wrong type.
Along the way added a check for this overflow (very incomplete, other overflows remain unchecked):
Before:
After:
And made incompatible types get rejected during typechecking rather than execution:
Before:
After: