-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Update to Arrow 35.0.0 #5441
Update to Arrow 35.0.0 #5441
Conversation
@@ -2979,10 +2979,6 @@ mod tests { | |||
ScalarValue::Decimal128(None, 10, 2), | |||
ScalarValue::try_from_array(&array, 3).unwrap() | |||
); | |||
assert_eq!( |
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 test was wrong, index 4 is beyond the bounds of the array. Previously this was fine as the null buffer only enforced the length for multiples of 8
"| [] |", | ||
"| [] |", | ||
"| [] |", | ||
"| [a] |", |
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.
🎉
@@ -1411,14 +1411,14 @@ async fn cast_timestamp_before_1970() -> Result<()> { | |||
|
|||
assert_batches_eq!(expected, &actual); | |||
|
|||
let sql = "select cast('1969-01-01T00:00:00.1Z' as timestamp);"; | |||
let sql = "select cast('1969-01-01T00:00:00.100Z' as timestamp);"; |
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.
what happens without this change?
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.
An error, it was a mis-reading of the RFC spec, I thought it restricted to multiples of 3 for subsecond precision, I'm working on a fix. It is somewhat amusing that chrono doesn't actually have any tests for 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.
Filed upstream issue: apache/arrow-rs#3859 (I think apache/arrow-rs#3858 fixes it)
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 would tend to think this regression should block the upgrade of datafusion, but would defer to others.
maybe we can mark this PR as "ready for review" to get some other opinions 🤔
@@ -414,7 +414,7 @@ async fn set_time_zone_bad_time_zone_format() { | |||
.await | |||
.unwrap(); | |||
let err = pretty_format_batches(&result).err().unwrap().to_string(); | |||
assert_eq!(err, "Parser error: Invalid timezone \"+08:00:00\": Expected format [+-]XX:XX, [+-]XX, or [+-]XXXX"); | |||
assert_eq!(err, "Parser error: Invalid timezone \"+08:00:00\": only offset based timezones supported without chrono-tz feature"); |
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.
perhaps we can improve the error message a bit in a future 😊
@@ -431,7 +431,7 @@ mod tests { | |||
.project(proj)? | |||
.build()?; | |||
|
|||
let expected = "Error parsing 'I'M NOT A TIMESTAMP' as timestamp"; | |||
let expected = "Error parsing timestamp from 'I'M NOT A TIMESTAMP'"; |
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 @comphead -- I think that is apache/arrow-rs#3859 There were a few regressions, in arrow 35.0.0 so I think we should probably skip arrow 35.0.0 and wait for arrow 36.0.0: apache/arrow-rs#3889 |
Do we have a list of regression to be fixed in arrow 36.0.0? |
The ones I know of are apache/arrow-rs#3889 (comment) |
Closing in favor of #5685 |
Which issue does this PR close?
Closes #.
Rationale for this change
Update to Arrow 35.0.0
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?