fix: honor Parquet byte-array statistics ordering - #24525
Open
sunchao wants to merge 1 commit into
Open
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #24525 +/- ##
==========================================
+ Coverage 81.27% 81.31% +0.04%
==========================================
Files 1116 1117 +1
Lines 395017 396039 +1022
Branches 395017 396039 +1022
==========================================
+ Hits 321055 322056 +1001
+ Misses 55166 55159 -7
- Partials 18796 18824 +28 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Member
Author
|
cc @alamb |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why are the changes needed?
Which issue does this PR close?
Part of #10586. This fixes the unsigned string/binary byte-array case; it does not claim to resolve every Parquet ordering issue. It is also the correctness prerequisite for #24526.
Rationale for this change
A Parquet scan can skip a file, row group, or page when its statistics prove that no row can satisfy the filter. If those statistics use a different comparison order from the query, that proof is invalid: DataFusion can silently discard a row that should be returned.
For example, suppose a Parquet row group contains
'aé','az', and'b':Parquet's deprecated byte-array
min/maxfields use signed comparison, whereas Arrow compares strings using unsigned UTF-8 bytes. The same values therefore have two different orders:aé < az < baé/baz < aé < baz/bThe first UTF-8 byte of
éis0xC3: it sorts beforez's0x7Aas a signed byte, but after it as an unsigned byte. If DataFusion interprets the legacy interval['aé', 'b']using Arrow's ordering, it sees'az' < 'aé'and can incorrectly skip the row group. Merely checking thatmin <= maxdoes not help: the two reported endpoints are still in ascending order.The newer
min_value/max_valuefields and page-index bounds also need an ordering declaration that the reader understands. A missing or unknowncolumn_ordersentry is not enough to justify assuming Arrow's ordering. The Parquet statistics definition and logical-type ordering rules describe these distinctions.This bug is independently observable with an ordinary equality filter; it does not require a large
INlist.What changes were proposed in this PR?
What changes are included in this PR?
The change makes a recognized comparison order a prerequisite for using unsigned byte-array bounds. At the point where Parquet metadata becomes Arrow statistics, DataFusion checks whether the column's physical/logical type and footer establish the expected unsigned order. It also rejects row-group bounds taken from the deprecated signed-order fields. When that evidence is missing, min/max is reported as unknown, so pruning keeps potentially matching data instead of guessing.
The rule is applied at the granularity where the statistics are used. An unsafe row group prevents DataFusion from claiming a trustworthy bound for the whole file, but it does not make other row groups' valid bounds unusable. Page-index bounds are checked against the footer independently. The same safeguards cover static and runtime row-group pruning, as well as the inverse predicates used to decide whether every row already satisfies a filter. Null counts and unrelated columns' statistics remain available.
The row-group pruning API gains a metadata-aware entry point so callers can supply the footer needed for this decision. The existing entry point remains source-compatible and behaves conservatively when that information is unavailable. Signed logical types such as decimals retain their existing behavior.
Are there any user-facing changes?
Queries no longer discard matching data because of these untrustworthy byte-array bounds. Older files, or files with an unrecognized ordering, may require more scanning. Modern files with trustworthy bounds retain min/max pruning. There is no file-format change or breaking public API change.
How was this PR tested?
Are these changes tested?
The regression uses actual serialized Parquet files containing the example above, with modern, deprecated, missing-order, and unknown-order metadata. It checks that the matching
azrow survives file, row-group, runtime, and page pruning, while valid statistics can still eliminate unrelated data. On unchanged Apachef1f0449a, the adapted equality regression fails because the deprecated-order case losesaz; the modern-statistics control passes.The dedicated Parquet-crate run passed 231 unit tests and four doctests. Its seven focused ordering tests also cover mixed safe/unsafe row groups, null counts, fixed-length binary and UUID, signed decimal, and logical types with undefined ordering. Formatting, all-targets/all-features Clippy with warnings denied, and
./dev/rust_lint.shpassed. The extended workspace run passed 10,666 Rust tests, with eight ignored, and all 503 SQL-logic files.The existing metadata benchmark was run on Apache
f1f0449aand this patch using the same valid modern-footer fixture. Across nine full-statistics cases there was no material regression; the largest case, with 256 columns and 128 row groups, measured 1.330 ms before and 1.332 ms after. Both runs used Rust 1.97.0,release-nonlto, 20 samples, and separate build directories on an Apple M5 Max.Validation commands