-
Notifications
You must be signed in to change notification settings - Fork 810
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
Improve error message for timestamp queries outside supported range #5730
Conversation
4cc559d
to
4b65f4e
Compare
Hi @tustvold, |
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.
Thank you @Abdi-29 -- this looks good to me ❤️ Nice work on the tests
I left a few comments, but I think we could merge this as is as well
Just let me know
T::make_value(naive).ok_or_else(|| { | ||
ArrowError::CastError(format!( | ||
T::make_value(naive).ok_or_else(|| match T::UNIT { | ||
TimeUnit::Nanosecond => ArrowError::CastError(format!( |
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 wonder if you would be willing to improve the messages for the other types (like Microsecond, etc)?
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.
Yes sure, I can work on it. Thanks for the feedback
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.
@alamb I did some research about the range of the other types but I couldn't find anything. Do you have any tips for me on where I can search for them?
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 you could create an array with T::Min
of the underlying type
something like (untested):
let min_date = DateTimeArray::from(vec![i32::MIN, i32:MAX]));
pretty_print(min_date)
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, I'll try it now
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 tried
let a = Arc::new(TimestampMicrosecondArray::from(vec![Some(i64::MIN), Some(i64::MAX)]));
println!("{}", pretty_format_columns("foo", &[a]).unwrap());
And I got
+--------------------------------------------------------------------------------------------------------+
| foo |
+--------------------------------------------------------------------------------------------------------+
| ERROR: Cast error: Failed to convert -9223372036854775808 to datetime for Timestamp(Microsecond, None) |
| ERROR: Cast error: Failed to convert 9223372036854775807 to datetime for Timestamp(Microsecond, None) |
+--------------------------------------------------------------------------------------------------------+
So if you can figure out the min/max values that were allowed for that type and then that code would show you the string value I think
However, to be honest what we have in this PR is better than main, I think we could merge it as is
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.
Ah I see, thanks. I'll open another issue to fix this
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 for the effort @Abdi-29 -- I think this PR is better than what is on main, even though it could be improved for other types as well
Thanks again
Which issue does this PR close?
Closes #5581
Rationale for this change
This change improves the error message handling for timestamp queries outside the supported range in Datafusion. Currently, users receive confusing error messages about nanosecond overflow, which do not clearly explain the limitation of timestamp queries. This change enhances the user experience and reduces confusion by providing clearer guidance.
What changes are included in this PR?
This PR updates the error message handling for timestamp queries to provide clearer guidance when timestamps fall outside the supported range.
Are there any user-facing changes?
Users will receive more informative error messages when executing timestamp queries outside the supported range.