adriangb opened a new pull request, #11569: URL: https://github.com/apache/arrow-rs/pull/11569
> [!IMPORTANT] > This PR is stacked on https://github.com/apache/arrow-rs/pull/11459. Do not merge this PR first. To see only the changes of this PR, use https://github.com/pydantic/arrow-rs/compare/claude/push-buffers-release-binary-search...claude/push-buffers-skip-empty # Which issue does this PR close? - Follow-up to https://github.com/apache/arrow-rs/pull/11459, as suggested by @alamb in https://github.com/apache/arrow-rs/pull/11459#pullrequestreview-5523546294 # Rationale for this change In https://github.com/apache/arrow-rs/pull/11459, `PushBuffers` keeps an `empty` counter of the empty ranges that `push_range` accepts. When the counter is not zero, `release_ranges` scans all buffers to remove them. This adds state and a slow path for ranges that contain no bytes. It is not safe to only drop empty pushes. A lookup for an empty range `n..n` succeeds only if a buffer touches offset `n`. If the decoder requests such a range, the caller pushes `n..n`. If that push is dropped, the decoder requests the same range again on each call, and the decoder never makes progress. # What changes are included in this PR? - `has_range`, `get_bytes` and `Read` treat an empty request as always available. `get_bytes(_, 0)` returns `Bytes::new()`. - `push_range` does not store an empty (or inverted) range, because it adds no bytes. - Remove the `empty` counter, the full-scan path in `release_ranges`, and the `empty` bookkeeping in `clear_ranges` and `clear_all_ranges`. `release_ranges` now only looks at the buffers that overlap the released ranges. # Are these changes tested? Yes. - New unit test `empty_ranges` in `push_buffers.rs`: an empty request is available with no buffers, an empty push is a no-op, and reads of real data still work after an empty push. - New test `needed_ranges_never_contains_an_empty_range` in the push decoder (`reader_builder/data.rs`): a request with a zero-length range never lists that range as needed, also after the caller pushes it. - The fuzz test model now also skips empty pushes and expects empty requests to be available. The invariant check asserts that no empty buffer is stored. # Are there any user-facing changes? Yes, one small behaviour change. `ParquetPushDecoder` and `ParquetMetaDataPushDecoder` no longer list zero-length ranges in `DecodeResult::NeedsData`. Before this change, a zero-length range that no pushed buffer touched was listed as needed. With this PR, an empty push is not stored, so such a request would never be satisfied and the decoder would loop forever. Because an empty range is now always available, the decoders do not request it, and this risk is removed. Callers that push empty ranges still get `Ok(())`. There are no public API changes. 🤖 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]
