-
Notifications
You must be signed in to change notification settings - Fork 1.7k
fix: Add overflow checks to SparkDateAdd/Sub to avoid panics #18013
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
query D | ||
SELECT date_sub('2016-07-30'::date, 2147483647::int); | ||
---- | ||
ERROR: Cast error: Failed to convert -2147466635 to temporal for Date32 |
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.
ERROR: Cast error: Failed to convert -2147466635 to temporal for Date32
is it a result returned by date_sub
🤔 ?
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.
No. This error is unrelated to the changes in this PR, but I thought it was good to add the test.
I was expecting to see an overflow though. I will try and track down the root cause.
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 added a test to show negative overflow
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.
Thanks @andygrove lgtm
🚀 |
…18013) * fix * fix * fix * fix * fix * add negative overflow test * remove unrelated test * update test
FYI @andygrove - created a PR to cherry-pick this to branch-50 : #18131 |
Which issue does this PR close?
DateAdd
/DateSub
do not perform overflow checks in release builds datafusion-comet#2539Rationale for this change
Return errors rather than panicking.
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?