Update to arrow 36 #5685
Update to arrow 36 #5685
Conversation
| @@ -3437,10 +3437,6 @@ mod tests { | |||
| ScalarValue::Decimal128(None, 10, 2), | |||
| ScalarValue::try_from_array(&array, 3).unwrap() | |||
| ); | |||
| assert_eq!( | |||
There was a problem hiding this comment.
See #5441 (comment)
This test was wrong
tustvold
force-pushed
the
update-arrow-36
branch
from
d73f165 to
7f16660
Compare
github-actions
bot
added
core
| "| [] |", | ||
| "| [] |", | ||
| "| [] |", | ||
| "| [a] |", |
There was a problem hiding this comment.
Bug fix -- see apache/arrow-rs#3803
| @@ -1390,7 +1390,7 @@ as values | |||
| ('2021-1-1T05:11:10.432', 'Row 3'); | |||
|
|
|||
|
|
|||
| statement ok | |||
| statement error DataFusion error: Arrow error: Parser error: Error parsing timestamp from '2021-1-1T05:11:10.432': error parsing date | |||
There was a problem hiding this comment.
I think we should change the data in this test so it passes
Also, it looks like a slight regression to me in that parsing 2021-1-1 used to parse and now doesn't.
So I suggest:
1. Change this test from
2021-1-1T05:11:10.432
to something like
2021-01-01T05:11:10.432
2 file an upstream ticket in arrow-rs to be more lenient
Aka to accept 2021-1-1T05:11:10.432 as valid -- I can do this if you like
There was a problem hiding this comment.
- Change this test from
Will do
2 file an upstream ticket in arrow-rs to be more lenient
I would strongly request we do not do this, it will severely hurt performance for an incredibly esoteric use-case. It isn't actually documented that chrono supports this, and we have never intentionally supported it either
| @@ -1387,7 +1387,7 @@ as values | |||
| ('2018-11-13T17:11:10.011375885995', 'Row 0'), | |||
| ('2011-12-13T11:13:10.12345', 'Row 1'), | |||
| (null, 'Row 2'), | |||
| ('2021-1-1T05:11:10.432', 'Row 3'); | |||
| ('2021-01-01T05:11:10.432', 'Row 3'); | |||
There was a problem hiding this comment.
Arrow no longer supports timestamps of this shortened form, it has never been documented to support them, and it is an accidental undocumented quirk of chrono that it did. I would very strongly resist changing this, being able to predict the digit locations ahead of time is critical to avoiding a data dependency when parsing.
There was a problem hiding this comment.
Filed apache/arrow-rs#3969 to track (I don't think we necessarily need to change arrow to support these timestamps, it but it will reduce confusion to have the change documented)
Which issue does this PR close?
Closes #.
Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?