You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "Abdi-29 (via GitHub)" <gi...@apache.org> on 2024/05/06 21:37:03 UTC
[PR] Improve error message for timestamp queries outside supported range [arrow-rs]
Abdi-29 opened a new pull request, #5730:
URL: https://github.com/apache/arrow-rs/pull/5730
# 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Improve error message for timestamp queries outside supported range [arrow-rs]
Posted by "Abdi-29 (via GitHub)" <gi...@apache.org>.
Abdi-29 commented on code in PR #5730:
URL: https://github.com/apache/arrow-rs/pull/5730#discussion_r1603588931
##########
arrow-cast/src/cast/string.rs:
##########
@@ -112,8 +112,11 @@ fn cast_string_to_timestamp_impl<O: OffsetSizeTrait, T: ArrowTimestampType, Tz:
.map(|v| {
v.map(|v| {
let naive = string_to_datetime(tz, v)?.naive_utc();
- 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!(
Review Comment:
Ah I see, thanks. I'll open another issue to fix this
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Improve error message for timestamp queries outside supported range [arrow-rs]
Posted by "Abdi-29 (via GitHub)" <gi...@apache.org>.
Abdi-29 commented on code in PR #5730:
URL: https://github.com/apache/arrow-rs/pull/5730#discussion_r1600538937
##########
arrow-cast/src/cast/string.rs:
##########
@@ -112,8 +112,11 @@ fn cast_string_to_timestamp_impl<O: OffsetSizeTrait, T: ArrowTimestampType, Tz:
.map(|v| {
v.map(|v| {
let naive = string_to_datetime(tz, v)?.naive_utc();
- 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!(
Review Comment:
thanks, I'll try it now
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Improve error message for timestamp queries outside supported range [arrow-rs]
Posted by "Abdi-29 (via GitHub)" <gi...@apache.org>.
Abdi-29 commented on PR #5730:
URL: https://github.com/apache/arrow-rs/pull/5730#issuecomment-2100288299
Hi @tustvold,
do you know what the next step it's going to be?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Improve error message for timestamp queries outside supported range [arrow-rs]
Posted by "Abdi-29 (via GitHub)" <gi...@apache.org>.
Abdi-29 commented on code in PR #5730:
URL: https://github.com/apache/arrow-rs/pull/5730#discussion_r1597420193
##########
arrow-cast/src/cast/string.rs:
##########
@@ -112,8 +112,11 @@ fn cast_string_to_timestamp_impl<O: OffsetSizeTrait, T: ArrowTimestampType, Tz:
.map(|v| {
v.map(|v| {
let naive = string_to_datetime(tz, v)?.naive_utc();
- 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!(
Review Comment:
Yes sure, I can work on it. Thanks for the feedback
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Improve error message for timestamp queries outside supported range [arrow-rs]
Posted by "Abdi-29 (via GitHub)" <gi...@apache.org>.
Abdi-29 commented on code in PR #5730:
URL: https://github.com/apache/arrow-rs/pull/5730#discussion_r1597420306
##########
arrow-cast/src/cast/mod.rs:
##########
@@ -8057,6 +8057,35 @@ mod tests {
test_cast_string_to_decimal256_overflow(overflow_array);
}
+ #[test]
+ fn test_cast_outside_supported_range_for_nanoseconds() {
+ const EXPECTED_ERROR_MESSAGE: &str = "The dates that can be represented as nanoseconds have to be between 1677-09-21T00:12:44.0 and 2262-04-11T23:47:16.854775804";
+
+ let array = StringArray::from(vec![Some("1650-01-01 01:01:01.000001")]);
+
+ let cast_options = CastOptions {
+ safe: false,
+ format_options: FormatOptions::default(),
+ };
+
+ let result = cast_string_to_timestamp::<i32, TimestampNanosecondType>(
+ &array,
+ &None::<Arc<str>>,
+ &cast_options,
+ );
+
+ assert!(result.is_err());
Review Comment:
I'll remove that part, Thanks alamb for the feedback
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Improve error message for timestamp queries outside supported range [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #5730:
URL: https://github.com/apache/arrow-rs/pull/5730#discussion_r1597412412
##########
arrow-cast/src/cast/string.rs:
##########
@@ -112,8 +112,11 @@ fn cast_string_to_timestamp_impl<O: OffsetSizeTrait, T: ArrowTimestampType, Tz:
.map(|v| {
v.map(|v| {
let naive = string_to_datetime(tz, v)?.naive_utc();
- 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!(
Review Comment:
I wonder if you would be willing to improve the messages for the other types (like Microsecond, etc)?
##########
arrow-cast/src/cast/mod.rs:
##########
@@ -8057,6 +8057,35 @@ mod tests {
test_cast_string_to_decimal256_overflow(overflow_array);
}
+ #[test]
+ fn test_cast_outside_supported_range_for_nanoseconds() {
+ const EXPECTED_ERROR_MESSAGE: &str = "The dates that can be represented as nanoseconds have to be between 1677-09-21T00:12:44.0 and 2262-04-11T23:47:16.854775804";
+
+ let array = StringArray::from(vec![Some("1650-01-01 01:01:01.000001")]);
+
+ let cast_options = CastOptions {
+ safe: false,
+ format_options: FormatOptions::default(),
+ };
+
+ let result = cast_string_to_timestamp::<i32, TimestampNanosecondType>(
+ &array,
+ &None::<Arc<str>>,
+ &cast_options,
+ );
+
+ assert!(result.is_err());
Review Comment:
I think this assert is unecessary (as `unwrap_err()` will panic if `result` is not an `Err`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Improve error message for timestamp queries outside supported range [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #5730:
URL: https://github.com/apache/arrow-rs/pull/5730#discussion_r1600533138
##########
arrow-cast/src/cast/string.rs:
##########
@@ -112,8 +112,11 @@ fn cast_string_to_timestamp_impl<O: OffsetSizeTrait, T: ArrowTimestampType, Tz:
.map(|v| {
v.map(|v| {
let naive = string_to_datetime(tz, v)?.naive_utc();
- 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!(
Review Comment:
I think you could create an array with `T::Min` of the underlying type
something like (untested):
```rust
let min_date = DateTimeArray::from(vec![i32::MIN, i32:MAX]));
pretty_print(min_date)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Improve error message for timestamp queries outside supported range [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #5730:
URL: https://github.com/apache/arrow-rs/pull/5730
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Improve error message for timestamp queries outside supported range [arrow-rs]
Posted by "Abdi-29 (via GitHub)" <gi...@apache.org>.
Abdi-29 commented on code in PR #5730:
URL: https://github.com/apache/arrow-rs/pull/5730#discussion_r1599857786
##########
arrow-cast/src/cast/string.rs:
##########
@@ -112,8 +112,11 @@ fn cast_string_to_timestamp_impl<O: OffsetSizeTrait, T: ArrowTimestampType, Tz:
.map(|v| {
v.map(|v| {
let naive = string_to_datetime(tz, v)?.naive_utc();
- 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!(
Review Comment:
@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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Improve error message for timestamp queries outside supported range [arrow-rs]
Posted by "Abdi-29 (via GitHub)" <gi...@apache.org>.
Abdi-29 commented on code in PR #5730:
URL: https://github.com/apache/arrow-rs/pull/5730#discussion_r1599857786
##########
arrow-cast/src/cast/string.rs:
##########
@@ -112,8 +112,11 @@ fn cast_string_to_timestamp_impl<O: OffsetSizeTrait, T: ArrowTimestampType, Tz:
.map(|v| {
v.map(|v| {
let naive = string_to_datetime(tz, v)?.naive_utc();
- 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!(
Review Comment:
@alamb I did some research about the range of the other types but I couldn't find anything. Do you perhaps have some tips for me on where I can search for them?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Improve error message for timestamp queries outside supported range [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #5730:
URL: https://github.com/apache/arrow-rs/pull/5730#discussion_r1603491847
##########
arrow-cast/src/cast/string.rs:
##########
@@ -112,8 +112,11 @@ fn cast_string_to_timestamp_impl<O: OffsetSizeTrait, T: ArrowTimestampType, Tz:
.map(|v| {
v.map(|v| {
let naive = string_to_datetime(tz, v)?.naive_utc();
- 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!(
Review Comment:
I tried
```rust
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org