DeviousCardi opened a new pull request, #3364: URL: https://github.com/apache/iceberg-rust/pull/3364
## Which issue does this PR close? - Closes #3280. ## What changes are included in this PR? `get_parquet_stat_min_as_datum` and `get_parquet_stat_max_as_datum` (`crates/iceberg/src/arrow/schema.rs`) decoded BYTE_ARRAY decimal statistics with `i128::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 made `try_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: - Bounds of 1 to 16 bytes decode correctly, and negative values are sign-extended. - Empty (0-byte) bounds return `DataInvalid`. `i128_from_be_bytes` maps empty input to `0`, and a made-up `0` bound could wrongly prune row groups that do match. Rejecting it keeps the behaviour `main` already has for empty bounds, which also fail today. - Bounds longer than 16 bytes still return `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? `RowGroupMetricsEvaluator` treats a missing stat (`Ok(None)`) as "cannot prune", so that would be safe. However, `get_parquet_stat_*_as_datum` is also used by the Parquet writer's min/max aggregation, which turns `None` into an `Unexpected` error. 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 on `main`. - `test_byte_array_decimal_stats_full_width_bounds`: 16-byte `i128::MIN` / `i128::MAX`. - `test_byte_array_decimal_stats_empty_bound_errors`: an empty min or max returns `ErrorKind::DataInvalid`. - `test_byte_array_decimal_stats_oversized_bound_errors`: a 17-byte min or max returns `ErrorKind::DataInvalid` and does not panic. These pass locally: - `cargo test -p iceberg` - `cargo clippy --all-targets --all-features --workspace -- -D warnings` - `cargo fmt --all -- --check` - `cargo machete` - `dev/check_license_notice.sh` ## AI Disclosure <!-- https://iceberg.apache.org/contribute/#guidelines-for-ai-assisted-contributions --> 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_bytes` helper 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](https://claude.com/claude-code) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
