-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
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"
datafusion/common/src/config.rs
Outdated
| /// 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 🚀 |
| /// 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
I pull the flag into its own PR where we can discuss using an enum: #19003 (review) |
Co-authored-by: Raz Luvaton <[email protected]>
|
run benchmarks |
|
🤖 |
|
🤖: Benchmark completed Details
|
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
|
Thanks again @rluvaton |
## Which issue does this PR close? - related to apache/arrow-rs#8464 ## Rationale for this change Get latest and greatest code from arrow ## What changes are included in this PR? 1. Update to Arrow 57.1.0 2. Update for API changes (comments inline) ## Are these changes tested? Yes, by CI ## Are there any user-facing changes? No --------- Co-authored-by: Raz Luvaton <[email protected]>
…prior to parquet 57.1.0 upgrade (#19003) ~Draft until #18820 is merged~ ## Which issue does this PR close? - Follow on to #18820 ## Rationale for this change The parquet 57.1.0 upgrade includes a new adaptive filter from @hhhizzz : - apache/arrow-rs#8733 Our testing shows this is faster in all cases, but I want to have an escape valve for people to turn it off if they hit some issue. I had originally included this in #18820 but @rluvaton suggested it would be easier to understand as its own PR in #18820 (review) ## What changes are included in this PR? 1. Add a `force_filter_selections` config setting 2. Add configuration guide 3. Add tests ## Are these changes tested? Yes ## Are there any user-facing changes? A new boolean flag
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