-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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 arrow 37 #5782
Update arrow 37 #5782
Conversation
}) | ||
.collect(); | ||
.collect::<Vec<_>>(); | ||
let arrays = arrays.iter().collect(); |
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 is temporary - see #5894
.collect::<Vec<_>>(); | ||
let arrays = arrays.iter().collect(); |
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 is temporary - see #5851
@@ -176,10 +176,10 @@ drop table foo | |||
|
|||
statement ok | |||
create table foo as select | |||
arrow_cast(100, 'Decimal128(3,2)') as col_d128 | |||
arrow_cast(100, 'Decimal128(5,2)') as col_d128 |
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 results from - apache/arrow-rs#3996
Which now complains (correctly) that 100 cannot be represented in an decimal with scale 2 and precision 3
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.
cc @viirya
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 looks good to me -- I haven't quite made it through this PR yet but what I have seen so far is good.
I plan to run the planning benchmark against this PR later today to see if it improves things
@@ -176,10 +176,10 @@ drop table foo | |||
|
|||
statement ok | |||
create table foo as select | |||
arrow_cast(100, 'Decimal128(3,2)') as col_d128 | |||
arrow_cast(100, 'Decimal128(5,2)') as col_d128 |
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.
cc @viirya
df_schema.fields.into_iter().map(|f| f.field).collect(), | ||
df_schema.metadata, | ||
) | ||
let fields: Fields = df_schema.fields.into_iter().map(|f| f.field).collect(); |
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.
🤮 at least this should be cheaper at runtime
I took the liberty of merging up from main to resolve a conflict |
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 it looks great. Thank you @tustvold
I also ran the sql_planning
benchmark, and sadly this branch doesn't show any improvements.
Given that this is a non trivial change that will take time to percolate through the ecosystem I plan to merge this shortly
* Update arrow 37 * Fix avro * Fix sql_planner benchmark * Update arrow pin * Format * Fix test * Remove pin * Update version * Fix logical merge conflicts * Pyarrow clippy * More clippy * fixup --------- Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
Which issue does this PR close?
Closes #.
Rationale for this change
Arrow 37 has some non-trivial breaking changes to schema handling, lets get ahead of this
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?