Repository navigation
fix(arrow): decode short BYTE_ARRAY decimal row-group stats - #3364
Merged
blackmwk merged 2 commits intoOct 12, 2026
Merged
Conversation
get_parquet_stat_min_as_datum / get_parquet_stat_max_as_datum decoded BYTE_ARRAY decimal bounds with a strict 16-byte conversion. Parquet stores these as minimal-length big-endian two's-complement values, so any bound shorter than 16 bytes failed to convert and aborted the row-group read. Use the variable-length, sign-extending i128_from_be_bytes helper, as the FIXED_LEN_BYTE_ARRAY arm already does. Bounds longer than 16 bytes still return DataInvalid. Empty bounds also return DataInvalid instead of decoding as zero, which could otherwise prune matching row groups. Closes apache#3280
DeviousCardi
marked this pull request as ready for review
October 8, 2026 03:13
…bytearray-stats # Conflicts: # crates/iceberg/src/arrow/schema.rs
blackmwk
approved these changes
Oct 12, 2026
blackmwk
left a comment
Contributor
There was a problem hiding this comment.
Thanks @DeviousCardi for this fix!
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?
What changes are included in this PR?
get_parquet_stat_min_as_datumandget_parquet_stat_max_as_datum(crates/iceberg/src/arrow/schema.rs) decoded BYTE_ARRAY decimal statistics withi128::from_be_bytes(bytes.try_into()?). That only works when the bound is exactly 16 bytes. Parquet writes these bounds as minimal-length big-endian two's-complement values, so a shorter bound madetry_into()fail, and the?aborted the row-group read.This PR switches both the min and max BYTE_ARRAY arms to
i128_from_be_bytes, the variable-length, sign-extending helper the FIXED_LEN_BYTE_ARRAY arm already uses. Now:DataInvalid.i128_from_be_bytesmaps empty input to0, and a made-up0bound could wrongly prune row groups that do match. Rejecting it keeps the behaviourmainalready has for empty bounds, which also fail today.DataInvalid, with the same message as the FIXED_LEN_BYTE_ARRAY arm.BYTE_ARRAY is not a spec-conformant Iceberg decimal encoding (the spec allows int32, int64 or fixed), so this is a defensive fix. It is the row-group counterpart of #3247.
Open question for reviewers
For malformed bounds (empty, or longer than 16 bytes), this PR keeps the existing row-group behaviour: return an error, which aborts the read. #3247 handled the same kind of malformed bound at the page-index layer differently: it logs at debug level and skips pruning for that column. Should the row-group path do the same?
RowGroupMetricsEvaluatortreats a missing stat (Ok(None)) as "cannot prune", so that would be safe. However,get_parquet_stat_*_as_datumis also used by the Parquet writer's min/max aggregation, which turnsNoneinto anUnexpectederror. A change there would need to be made at the evaluator, not in these functions. I left it out of scope to keep this PR minimal and happy to follow up.Are these changes tested?
Yes. New unit tests in
arrow::schema::tests:test_byte_array_decimal_stats_short_bounds: 2-byte bounds (-1234/1234) and 1-byte bounds (0x80->-128,0x7F->127). These check that negative values are sign-extended. This test fails onmain.test_byte_array_decimal_stats_full_width_bounds: 16-bytei128::MIN/i128::MAX.test_byte_array_decimal_stats_empty_bound_errors: an empty min or max returnsErrorKind::DataInvalid.test_byte_array_decimal_stats_oversized_bound_errors: a 17-byte min or max returnsErrorKind::DataInvalidand does not panic.These pass locally:
cargo test -p icebergcargo clippy --all-targets --all-features --workspace -- -D warningscargo fmt --all -- --checkcargo machetedev/check_license_notice.shAI Disclosure
This change was developed with AI assistance (Claude Code): investigating the issue, writing the fix and unit tests, and drafting this description. The fix reuses the existing
i128_from_be_byteshelper and was checked with the tests listed above, including confirming the short-bounds test fails without the fix. The area most worth a reviewer's attention is the choice to keep erroring on empty/oversized bounds (see the open question above).🤖 Generated with Claude Code