bharadwaj-pendyala opened a new pull request, #11021: URL: https://github.com/apache/arrow-rs/pull/11021
# Which issue does this PR close? - Closes #11018. # Rationale for this change `DeltaBitPackDecoder::skip` sized its scratch buffer with a `match` on `values_per_mini_block` that only accepted 32 and 64, and returned `cannot skip miniblock of size N` for anything else. The spec only requires a multiple of 32, and Databricks Photon 0.2 writes 256, so valid files from that engine fail to read as soon as a `RowSelector::skip` is involved. `get` never had the restriction because it chunks against the caller's buffer. The scratch buffer only exists to walk `last_value` forward through the deltas. `BitReader::get_batch` carries its own `bit_offset` and `byte_offset`, so reading a 256 value miniblock as four calls of 64 leaves the reader exactly where one call of 256 would. That means the buffer can stay small and the miniblock can be consumed a chunk at a time, which was the concern that closed #9793 without a fix. # What changes are included in this PR? - Size `skip_buffer` at `values_per_mini_block.min(64)` and loop over the miniblock in chunks of it, instead of rejecting sizes other than 32 and 64. - Flip `non_standard_delta_blocks` in `bad_data.rs` from asserting the error to asserting the skip lands on the right rows. That test already carried a commented-out `skip should succeed` and a TODO for this change. For 32 and 64 the buffer is the same length it was before and the new loop runs exactly once, so the path this crate's own writer produces allocates and reads what it did before. # Are these changes tested? - New `test_delta_bit_packed_skip_wide_miniblocks` builds a hand-written page with `block_size = 1024` over 4 miniblocks, so 256 values each. It fails on `main` with `cannot skip miniblock of size 256` and passes here. It checks the values after a skip that stops mid-miniblock, and after one that crosses into a `bit_width = 0` miniblock, since both depend on `last_value` surviving the chunk boundary. - `non_standard_delta_blocks` now reads `bigdelta.parquet` twice, once with the row selection and once without, and compares the 5 selected rows against `all.slice(1000, 5)`. Asserting only `num_rows() == 5` would pass on wrong values. - `cargo test -p parquet --all-features`: 1426 lib tests and 456 integration tests pass, 0 failures. - `cargo clippy -p parquet --all-features --all-targets`: clean. - No benchmark numbers here because the change doesn't claim a speedup. The argument that the common path is untouched is the buffer length and loop count above, not a measurement. # Are there any user-facing changes? Reading a page with miniblocks wider than 64 values under a row selection now returns data instead of an error. No public API changes. There's also an existing overflow bug in `skip`. The `bit_width == 0` branch of `skip` computes `min_delta * n` in `i64` and then converts to `T::T`, so a valid INT32 progression can fail the conversion even though every decoded value fits. A page with `first_value = -2000000000`, `min_delta = 10000000` and zero-width miniblocks reads fine through `get`, giving 560000000 at index 256, but `skip(257)` returns `delta*n overflow in skip`. That's independent of miniblock width and predates this change, so I've left it for a separate issue and fix. # AI usage Claude wrote the change and the tests, and Codex reviewed the diff adversarially before it was pushed. I checked every finding myself: the split-read equivalence against `BitReader::get_batch`, the truncated-page error path, and the overflow above, which I reproduced before writing it down. The PR description is AI-assisted. -- 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]
