-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Update to arrow, parquet to 57.1.0
#18820
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
base: main
Are you sure you want to change the base?
Conversation
| | alltypes_plain.parquet | 1851 | 6957 | 2 | page_index=false | | ||
| | alltypes_tiny_pages.parquet | 454233 | 267014 | 2 | page_index=true | | ||
| | lz4_raw_compressed_larger.parquet | 380836 | 996 | 2 | page_index=false | | ||
| | alltypes_plain.parquet | 1851 | 8882 | 2 | page_index=false | |
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.
the metadata didn't actually get bigger, we just actually included the encryption information (better reporting)
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.
Actually I looked into it more and I think the size growth is a bug. See
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.
Update: the size is correct. As @etseidl says "the truth hurts"
|
🤖 |
|
🤖: Benchmark completed Details
|
|
🤖 |
| query TTT | ||
| select arrow_typeof(column1), arrow_typeof(column2), arrow_typeof(column3) from arrays; | ||
| ---- | ||
| List(nullable List(nullable Int64)) List(nullable Float64) List(nullable Utf8) |
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.
Previously the DataType parsing code did not handle this syntax (it only supported List(Float64)). We have now made the display and parsing consistent, see apache/arrow-rs#8649 (comment) for background and details
| | alltypes_plain.parquet | 1851 | 6957 | 2 | page_index=false | | ||
| | alltypes_tiny_pages.parquet | 454233 | 267014 | 2 | page_index=true | | ||
| | lz4_raw_compressed_larger.parquet | 380836 | 996 | 2 | page_index=false | | ||
| | alltypes_plain.parquet | 1851 | 8882 | 2 | page_index=false | |
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.
Actually I looked into it more and I think the size growth is a bug. See
|
🤖: Benchmark completed Details
|
fafb102 to
191db07
Compare
191db07 to
5a91551
Compare
5a91551 to
9eab2c3
Compare
| | alltypes_plain.parquet | 1851 | 6957 | 2 | page_index=false | | ||
| | alltypes_tiny_pages.parquet | 454233 | 267014 | 2 | page_index=true | | ||
| | lz4_raw_compressed_larger.parquet | 380836 | 996 | 2 | page_index=false | | ||
| | alltypes_plain.parquet | 1851 | 8882 | 2 | page_index=false | |
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.
Update: the size is correct. As @etseidl says "the truth hurts"
| /// the filters are applied in the same order as written in the query | ||
| pub reorder_filters: bool, default = false | ||
|
|
||
| /// (reading) Force the use of RowSelections for filter results, when |
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 an escape valve if we find some issue when using the new adaptive filter from @hhhizzz
| assert_contains!( | ||
| &e, | ||
| r#"Error during planning: Can not find compatible types to compare Boolean with [Struct("foo": Boolean), Utf8]"# | ||
| r#"Error during planning: Can not find compatible types to compare Boolean with [Struct("foo": non-null Boolean), Utf8]"# |
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.
these are due the changes from apache/arrow-rs#8648 to clean up datatype display. It is a nice improvement in my mind
| // The cache is on by default, and used when filter pushdown is enabled | ||
| PredicateCacheTest { | ||
| expected_inner_records: 8, | ||
| expected_records: 7, // reads more than necessary from the cache as then another bitmap is applied |
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 behavior changed due to adaptive filtering. I added a new test that turns off adaptive filtering to show doing so restores the old behavior
It seems to be quite a bit faster even without filter pushdown 🚀 |
It is like someone has been optimizing low level filter kernels 😆 (but seriously I think major credit is due to you and @rluvaton ) |
|
Thank you, I have some more in my sleeve. |
9eab2c3 to
a81c0d3
Compare
a81c0d3 to
eda1b53
Compare
arrow, parquet 57.1.0arrow, parquet 57.1.0
arrow, parquet 57.1.0arrow, parquet to 57.1.0
| [1, 2, 3, 4, 5] [h, e, l, l, o] | ||
|
|
||
| # TODO: Enable once arrow_cast supports ListView types. | ||
| # TODO: Enable once array_slice supports LargeListView types. |
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.
It seems like you copy pasted from below, but this test ListView and not LargeListView
| # TODO: Enable once array_slice supports LargeListView types. | |
| # TODO: Enable once array_slice supports ListView types. |
| # ---- | ||
| # [1, 2, 3, 4, 5] [h, e, l, l, o] | ||
| query error DataFusion error: Execution error: Unsupported type 'ListView\(Int64\)'. Must be a supported arrow type name such as 'Int32' or 'Timestamp\(ns\)'. Error unknown token: ListView | ||
| query error Failed to coerce arguments to satisfy a call to 'array_slice' function: |
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.
Might not be related to this PR but now I don't know what arguments are the problematic ones with this change
before I could see it's ListView but now I have no idea what arguments is invalid and what their type that is not supported
| /// Should we force the reader to use RowSelections for filtering | ||
| pub force_filter_selections: bool, |
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 do you think of making it an enum instead to allow for future additions without breaking changes?
(that enum should also be non exhaustive to avoid adding a variant a breaking change)
I also see that the with_row_selection_policy already accept enum.
making it an enum also allow to force mask or configure the threshold in the auto policy. this is also useful for testing to force specific path when creating a reproduction test for a bug
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 a good idea -- I was secretly hoping no one would use this flag and added it as an "escape" valve to go back to the arrow 57.0.0 reader behavior
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.
left some comments, other than that LGTM
what do you think of splitting to 2 PRs
one for the actual upgrade and one for the new flag in parquet reader
not because the PR are large but because they are not required to be in the same PR for the upgrade to be made.
If you decide not to, please update the title and the description so the commit message will include that change making it easier to find later
I will do this. In my mind the config flag is required in order to allow people to opt out of the new behavior |
so if the behavior changed, we want by default to opt out of it for this release, no? or only for this pr, |
|
if you split the PR it would also be easier for others to create PR with the new arrow version while we discuss this |
I am not sure The default behavior of the parquet reader has changed in arrow-rs (in theory it will always be better). The only usecase I have is adding an "escape valve" so that if someone hit an issue with the new code, there was a way to turn if off without requiring a fork I don't (yet) have any reason to believe the new behavior isn't always better nor any usecase for tuning the row selector policy from DataFusion |
rluvaton
left a comment
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.
LGTM, I'm eager to create PR that using the new version so approving, if you decide to merge without the enum change
Which issue does this PR close?
57.1.0(November 2025) arrow-rs#8464Rationale for this change
Get latest and greatest code from arrow
What changes are included in this PR?
Are these changes tested?
Yes, by CI
Are there any user-facing changes?
No