hhhizzz commented on code in PR #10288:
URL: https://github.com/apache/arrow-rs/pull/10288#discussion_r3610661674
##########
parquet/src/arrow/arrow_reader/mod.rs:
##########
@@ -1424,33 +1429,33 @@ impl ParquetRecordBatchReader {
read
));
}
-
- let array = self.array_reader.consume_batch()?;
- // The column reader exposes the projection as a struct
array; convert this
- // into a record batch before applying the boolean filter
mask.
- let struct_array = array.as_struct_opt().ok_or_else(|| {
- ArrowError::ParquetError(
- "Struct array reader should return struct
array".to_string(),
- )
- })?;
-
- let filtered_batch =
- filter_record_batch(&RecordBatch::from(struct_array),
&mask)?;
-
- if filtered_batch.num_rows() != mask_chunk.selected_rows {
- return Err(general_err!(
- "filtered rows mismatch selection - expected {},
got {}",
- mask_chunk.selected_rows,
- filtered_batch.num_rows()
- ));
- }
-
- if filtered_batch.num_rows() == 0 {
- continue;
+ match combined_filter_mask.as_mut() {
+ Some(combined) =>
combined.append_buffer(mask.values()),
+ None => match first_filter_mask.take() {
+ Some(first) => {
+ let mut combined =
+ BooleanBufferBuilder::new(first.len() +
mask.len());
+ combined.append_buffer(&first);
+ combined.append_buffer(mask.values());
+ combined_filter_mask = Some(combined);
+ }
+ None => first_filter_mask =
Some(mask.values().clone()),
+ },
}
+ read_records += mask_chunk.selected_rows;
+ }
- return Ok(Some(filtered_batch));
+ if read_records == 0 {
+ return Ok(None);
}
+
+ let filter_mask = match combined_filter_mask {
Review Comment:
I looked into using `BatchCoalescer`, but kept the current approach for this
PR.
`ArrayReader` already buffers multiple `read_records` calls across
`skip_records`, which lets this implementation cross several loaded ranges,
call `consume_batch` once, and apply the combined filter once. Using
`BatchCoalescer` here would require materializing and filtering a `RecordBatch`
for each loaded range before coalescing them, introducing intermediate batches
and additional copying.
I think that trade-off would be better evaluated separately with dedicated
benchmarks rather than expanding this correctness-focused PR.
--
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]
