hhhizzz commented on code in PR #10288:
URL: https://github.com/apache/arrow-rs/pull/10288#discussion_r3610655563
##########
parquet/src/arrow/arrow_reader/mod.rs:
##########
@@ -1381,13 +1382,17 @@ impl ParquetRecordBatchReader {
if batch_size == 0 {
return Ok(None);
}
- match self.read_plan.row_selection_cursor_mut() {
+ let filter_mask = match self.read_plan.row_selection_cursor_mut() {
RowSelectionCursor::Mask(mask_cursor) => {
- // Stream the record batch reader using contiguous segments of
the selection
- // mask, avoiding the need to materialize intermediate
`RowSelector` ranges.
- while !mask_cursor.is_empty() {
- let Some(mask_chunk) =
mask_cursor.next_mask_chunk(batch_size) else {
- return Ok(None);
+ let mut first_filter_mask: Option<BooleanBuffer> = None;
+ let mut combined_filter_mask: Option<BooleanBufferBuilder> =
None;
+
+ // Each chunk stays within loaded pages, while the in-progress
array and
+ // filter mask accumulate chunks until the logical batch is
full.
+ while read_records < batch_size && !mask_cursor.is_empty() {
+ let remaining = batch_size - read_records;
+ let Some(mask_chunk) = mask_cursor.next_chunk(remaining)?
else {
Review Comment:
I traced this back to an invariant established by `ReadPlanBuilder::build`:
trailing skips are trimmed before constructing a `MaskCursor`. Since the reader
only calls `next_chunk` for a non-empty cursor with a non-zero batch size, a
selected row must remain and `next_chunk` cannot return `None` here.
I made that invariant explicit by changing the crate-private `next_chunk`
contract to return `MaskChunk` directly, removed the unreachable branch, and
added a unit test covering a selection with trailing skips.
--
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]
