zhuqi-lucas commented on code in PR #10901:
URL: https://github.com/apache/arrow-rs/pull/10901#discussion_r4119104400
##########
parquet/src/arrow/array_reader/cached_array_reader.rs:
##########
@@ -168,22 +172,33 @@ impl CachedArrayReader {
/// Remove batches from cache that have been completely consumed
/// This is only called for Consumer role readers
- fn cleanup_consumed_batches(&self) {
+ fn cleanup_consumed_batches(&mut self) {
let current_batch_id =
self.get_batch_id_from_position(self.outer_position);
// Remove batches that are at least one batch behind the current
position
// This ensures we don't remove batches that might still be needed for
the current batch
// We can safely remove batch_id if current_batch_id > batch_id + 1
- if current_batch_id.val > 1 {
- let mut cache = self.shared_cache.write().unwrap();
- for batch_id_to_remove in 0..(current_batch_id.val - 1) {
- cache.remove(
- self.column_idx,
- BatchID {
- val: batch_id_to_remove,
- },
- );
- }
+ if current_batch_id.val <= 1 {
+ return;
+ }
+ let end = current_batch_id.val - 1;
Review Comment:
Left as is — this changes behaviour rather than simplifies. `end =
current_batch_id.val` removes up to `current - 1`; the `-1` keeps it at
`current - 2`, which is what `main` does today (`for batch_id_to_remove in
0..(current_batch_id.val - 1)`). This PR only makes that same range incremental.
Widening it asserts that no consumer can still ask for `current - 1`.
Plausible — the `local_cache.retain` just above keeps `>= current` — but worth
its own PR. Happy to open one.
--
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]
