zhuqi-lucas commented on code in PR #7524:
URL: https://github.com/apache/arrow-rs/pull/7524#discussion_r2095546155
##########
parquet/src/arrow/arrow_reader/mod.rs:
##########
@@ -808,58 +841,92 @@ impl ParquetRecordBatchReader {
fn next_inner(&mut self) -> Result<Option<RecordBatch>> {
let mut read_records = 0;
let batch_size = self.batch_size();
+
+ let mut mask_builder = BooleanBufferBuilder::new(batch_size);
+
match self.read_plan.selection_mut() {
Some(selection) => {
- while read_records < batch_size && !selection.is_empty() {
- let front = selection.pop_front().unwrap();
- if front.skip {
- let skipped =
self.array_reader.skip_records(front.row_count)?;
+ while let Some(cur_selection) =
+ take_next_selection(selection, batch_size - read_records)
+ {
+ let mut total_read = 0;
+ let mut total_skip = 0;
+ for r in cur_selection.iter() {
+ if r.skip {
+ total_skip += r.row_count;
+ } else {
+ total_read += r.row_count;
+ }
+ }
+ let select_count = cur_selection.iter().count();
+ let total = total_skip + total_read;
- if skipped != front.row_count {
- return Err(general_err!(
+ if total < 10 * select_count {
+ let mut bitmap_builder =
BooleanBufferBuilder::new(total);
Review Comment:
> Since many of these read selections come from a BooleanBuffer originally
(the result of evaluating a ArrowPredicate), I wonder what you think about
potentially avoiding creating `RowSelection`s in the ReadPlan
>
> Something like
>
> ```rust
> enum DecodeRows {
> /// Decode exactly the rows accoding to the row selection
> RowSelection(RowSelection),
> /// Decode all rows, and then apply a filter to keep only the ones that
matter
> Bitmask(BooleanBuffer)
> }
>
> impl ReadPlan {
> // instead of Selection, have DecodeRows
> decode_plan: VecDeque<DecodeRows>;
> }
> ```
>
> And then the trick would be some sort of heuristic / adaptive approach to
turn the result of the ArrowPredicate into a `DecodeRows` plan
>
>
https://github.com/apache/arrow-rs/blob/7bab215351876ffbef8e4e5898bdc1bf766557f5/parquet/src/arrow/arrow_reader/mod.rs#L1013-L1017
>
> 🤔
Thank you @alamb for this suggestion, i was thinking use this way.
But i don't use it for this PR, because the adaptive level is batch level
for this PR, for example:
We have batch size 8192, we only do the adaptive for each batch level, and
the default is to use range selector, so we will be more accurate for each
small level to choose:
8192(2000read 2000skip 4192 read) => keep range
8192(200read 200 skip ....200read..) avg =200 => keep range
8192(5 read 10 skip 10 skip, 5 read, 5read...) => avg < 10 so nee to change
the 8192 small window to bitmap
....
So we only change for small window with default range selector to change to
bitmap, it's not heavy for most cases.
--
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]
