adriangb opened a new pull request, #11685: URL: https://github.com/apache/arrow-rs/pull/11685
Stacked on https://github.com/apache/arrow-rs/pull/11549 and https://github.com/apache/arrow-rs/pull/11641. Review only the last commit: https://github.com/pydantic/arrow-rs/compare/bba50bfeb...ce9cad991 ## Summary https://github.com/apache/arrow-rs/pull/11641 adds a second copy of the Arrow to Parquet value conversion for `Sbbf::hash_array`, `insert_array` and `check_array`. That copy is different from what `ArrowWriter` writes: - It hashes `Date64` as `INT64`, but with `coerce_types` the writer stores `INT32` days. - It hashes decimals at the array's precision, not at the column's width. This PR removes that copy. All Arrow hashing goes through the writer's `visit_i32`, `visit_i64` and `visit_fixed_len_byte_array`. `ArrowSbbf` becomes a thin wrapper. ```text ArrowWriter::write_leaf ──┐ ├─> visit_i32 / visit_i64 / visit_fixed_len_byte_array (one conversion) bloom_filter::arrow │ visit_hashes(target) ───┘ Sbbf::hash_array / insert_array / check_array target = arrow_to_parquet_type(data_type) ArrowSbbf::check target = column of the file ``` Other changes: - A decimal that does not fit in a `FIXED_LEN_BYTE_ARRAY` column is now truncated, as the writer does. Before, `ArrowSbbf::check` reported it as absent, which is a false negative for a value that was written. The writer no longer computes an overflow flag that it does not use. - `visit_i64` accepts `Decimal32`. Before, `ArrowSbbf::check` with a `Decimal32` array against an `INT64` decimal column returned an error. ## Evidence - **Before:** `ArrowSbbf::check(Decimal32(9, 2) [12345])` against a `Decimal64(12, 2)` column gives `Err("Cannot coerce Decimal32(9, 2) to I64")`. **After:** gives `[true]` (new test `decimal32_against_int64_column`). - **Before:** a `Decimal128(38, 2)` value wider than 13 bytes, checked against a precision 30 column, gives `false`. The writer truncates the same value and stores it. **After:** gives `true`, the same as the writer (updated test `decimal_precision_differs_from_column`). - All existing tests from both PRs pass, including https://github.com/apache/arrow-rs/pull/11641's `compatible_with_arrow_writer`, which compares the bitset of `insert_array` with the bitset that `ArrowWriter` writes. Clippy is clean with default, `arrow` and all features. <details> <summary>Benchmark (laptop, noisy)</summary> `cargo bench -p parquet --features arrow --bench bloom_filter_arrow -- arrow_check`, A/B/A. A = https://github.com/apache/arrow-rs/pull/11641, B = this PR. | Case | A | B | A | | --- | --- | --- | --- | | `check_array` i64 | 3.37 ms | 2.44 ms | 2.31 ms | | `hash_array` + `check_hash` i64 | 5.28 ms | 5.74 ms | 5.10 ms | | `check_array` utf8 | 4.63 ms | 4.28 ms | 3.96 ms | | `hash_array` + `check_hash` utf8 | 7.08 ms | 6.61 ms | 6.27 ms | The two A runs differ by up to 30%, so the B results are in the noise. B can be up to about 13% slower than the second A run. This needs a run on a quiet machine. </details> ## Merge Danger **Door:** two-way Only `pub(crate)` code and the behavior of `ArrowSbbf` for decimals that do not fit change. Neither PR below this one is released yet. **Blast Radius:** bloom-filters `ArrowWriter` output does not change: the FLBA path still truncates, and the only writer edit is removing an unused flag. `ArrowSbbf::check` can now give a false positive (never a false negative) for decimals too wide for the column. AI usage: Claude Code wrote this change. It is a draft until I finish my own review. 🤖 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]
