Repository navigation
fix: discard parquet bounds when row group statistics are missing - #25228
Merged
jayzhan211 merged 6 commits intoSep 13, 2026
Merged
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #25228 +/- ##
==========================================
+ Coverage 81.93% 81.95% +0.01%
==========================================
Files 1133 1133
Lines 423529 423993 +464
Branches 423529 423993 +464
==========================================
+ Hits 347032 347495 +463
+ Misses 55907 55904 -3
- Partials 20590 20594 +4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
haohuaijin
marked this pull request as ready for review
September 12, 2026 12:48
jayzhan211
approved these changes
Sep 13, 2026
Contributor
There was a problem hiding this comment.
Thanks @haohuaijin Few non-blocking suggestions
Co-authored-by: Jay Zhan <[email protected]>
Contributor
Author
|
Thanks @jayzhan211, I applied the suggestion. |
Contributor
|
Thanks @haohuaijin |
Omega359
pushed a commit
to Omega359/arrow-datafusion
that referenced
this pull request
Oct 11, 2026
…ache#25228) ## Which issue does this PR close? Closes apache#25226. ## Rationale for this change MIN/MAX queries can return incorrect results when a Parquet file contains a mix of row groups with and without bounds. A missing bound is converted to a NULL in the per-row-group statistics array. Aggregating that array skips the NULL, allowing another group's endpoint to be reported as an exact bound for the entire file. The optimizer can then answer the aggregate from incomplete statistics. For example, a file contains `["a", "b"]` with statistics and two 8192-character strings starting with `z` without statistics. `SELECT LENGTH(MIN(a)), LENGTH(MAX(a))` previously returned `1, 1`; the correct result is `1, 8192`. ## What changes are included in this PR? - Invalidate a file-level bound when any row group has no converted bound and may contain non-NULL values. Return `Precision::Absent` so that incomplete statistics cannot establish the file's extrema. - Handle minimum and maximum independently, preserving a usable bound when only the other endpoint is missing. - Preserve bounds from other groups when the group with no bound is empty or is proven all NULL. The all-NULL check requires both the column value count and the explicitly recorded NULL count to equal the row count. - Express the active-accumulator precondition with `expect`: callers already check for `Some`, and the path that clears the accumulator returns immediately. This removes an unreachable fallback return. ## What is the testing strategy for this PR? `test_min_max_require_complete_row_group_bounds` covers absent statistics, independently missing minimum/maximum values, unknown and partial NULL counts, empty groups, all-NULL groups, and complete bounds. Every case runs in both row-group orders. `parquet_missing_bounds.slt` verifies both the stored string lengths and the aggregate results. Its fixture is generated at runtime using public Rust Parquet writer APIs, enabling chunk statistics for the first row group and disabling them for the second. Disabling statistics is deliberate: parquet-rs truncates long extrema rather than omitting them. No generated Parquet file is committed and no footer is manually edited. Local validation: - All 10 tests in `metadata::tests::statistics_tests` passed. - `parquet_missing_bounds.slt` passed with the fix. Running the same fixture and SQL test against unmodified main reproduced the incorrect `1, 1` result. - LLVM coverage confirmed that the removed fallback return had zero executions and that the replacement precondition assertion was exercised 36 times by the statistics tests. The updated Codecov patch percentage remains subject to the next CI run. ## Are there any user-facing changes? MIN/MAX queries return correct results for Parquet files with incomplete row-group bounds. Valid statistics remain usable for each independently complete endpoint, including when other groups are empty or all NULL. There are no public API changes. --------- Co-authored-by: Jay Zhan <[email protected]>
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.
Which issue does this PR close?
Closes #25226.
Rationale for this change
MIN/MAX queries can return incorrect results when a Parquet file contains a mix of row groups with and without bounds. A missing bound is converted to a NULL in the per-row-group statistics array. Aggregating that array skips the NULL, allowing another group's endpoint to be reported as an exact bound for the entire file. The optimizer can then answer the aggregate from incomplete statistics.
For example, a file contains
["a", "b"]with statistics and two 8192-character strings starting withzwithout statistics.SELECT LENGTH(MIN(a)), LENGTH(MAX(a))previously returned1, 1; the correct result is1, 8192.What changes are included in this PR?
Precision::Absentso that incomplete statistics cannot establish the file's extrema.expect: callers already check forSome, and the path that clears the accumulator returns immediately. This removes an unreachable fallback return.What is the testing strategy for this PR?
test_min_max_require_complete_row_group_boundscovers absent statistics, independently missing minimum/maximum values, unknown and partial NULL counts, empty groups, all-NULL groups, and complete bounds. Every case runs in both row-group orders.parquet_missing_bounds.sltverifies both the stored string lengths and the aggregate results. Its fixture is generated at runtime using public Rust Parquet writer APIs, enabling chunk statistics for the first row group and disabling them for the second. Disabling statistics is deliberate: parquet-rs truncates long extrema rather than omitting them. No generated Parquet file is committed and no footer is manually edited.Local validation:
metadata::tests::statistics_testspassed.parquet_missing_bounds.sltpassed with the fix. Running the same fixture and SQL test against unmodified main reproduced the incorrect1, 1result.Are there any user-facing changes?
MIN/MAX queries return correct results for Parquet files with incomplete row-group bounds. Valid statistics remain usable for each independently complete endpoint, including when other groups are empty or all NULL. There are no public API changes.