alamb commented on code in PR #11510:
URL: https://github.com/apache/arrow-rs/pull/11510#discussion_r4247601099
##########
parquet/src/arrow/arrow_reader/selection/cursor.rs:
##########
@@ -238,45 +239,73 @@ impl MaskCursor {
/// Produces the next chunk for a non-empty, trailing-skip-free mask.
fn next_mask_chunk_non_empty(&mut self, batch_size: usize) -> MaskChunk {
debug_assert!(!self.is_empty());
+ let mask_start = self.next_selected_row().unwrap_or(self.mask.len());
+ debug_assert!(
+ mask_start < self.mask.len(),
+ "ReadPlan must remove trailing skips from Mask selections"
+ );
+ self.scan_chunk(mask_start, self.mask.len(), batch_size)
+ }
- let (initial_skip, chunk_rows, selected_rows, mask_start,
end_position) = {
- let mask = &self.mask;
- let start_position = self.position;
- let mut cursor = start_position;
- let mut initial_skip = 0;
+ /// Find the first selected row without inspecting every skipped bit.
+ fn next_selected_row(&self) -> Option<usize> {
+ let chunks = BitChunks::new(
+ self.mask.values(),
+ self.mask.offset() + self.position,
+ self.mask.len() - self.position,
+ );
+ chunks.iter_padded().enumerate().find_map(|(idx, word)| {
+ (word != 0).then(|| self.position + idx * 64 +
word.trailing_zeros() as usize)
Review Comment:
I was confused at first if the 64 would break on 32 bit architectures, but
since BitChunks is explicitly using u64 (and not usize) I think it is good
##########
parquet/src/arrow/arrow_reader/selection/cursor.rs:
##########
@@ -238,45 +239,73 @@ impl MaskCursor {
/// Produces the next chunk for a non-empty, trailing-skip-free mask.
fn next_mask_chunk_non_empty(&mut self, batch_size: usize) -> MaskChunk {
debug_assert!(!self.is_empty());
+ let mask_start = self.next_selected_row().unwrap_or(self.mask.len());
+ debug_assert!(
+ mask_start < self.mask.len(),
+ "ReadPlan must remove trailing skips from Mask selections"
+ );
+ self.scan_chunk(mask_start, self.mask.len(), batch_size)
+ }
- let (initial_skip, chunk_rows, selected_rows, mask_start,
end_position) = {
- let mask = &self.mask;
- let start_position = self.position;
- let mut cursor = start_position;
- let mut initial_skip = 0;
+ /// Find the first selected row without inspecting every skipped bit.
+ fn next_selected_row(&self) -> Option<usize> {
+ let chunks = BitChunks::new(
+ self.mask.values(),
+ self.mask.offset() + self.position,
+ self.mask.len() - self.position,
+ );
+ chunks.iter_padded().enumerate().find_map(|(idx, word)| {
Review Comment:
It also feels to me this is a optimization opportunity -- both
`next_selected_row` and `scan_chunk` seem to iterate though the same bits 🤔
##########
parquet/src/arrow/arrow_reader/selection/cursor.rs:
##########
@@ -238,45 +239,73 @@ impl MaskCursor {
/// Produces the next chunk for a non-empty, trailing-skip-free mask.
fn next_mask_chunk_non_empty(&mut self, batch_size: usize) -> MaskChunk {
debug_assert!(!self.is_empty());
+ let mask_start = self.next_selected_row().unwrap_or(self.mask.len());
+ debug_assert!(
+ mask_start < self.mask.len(),
+ "ReadPlan must remove trailing skips from Mask selections"
+ );
+ self.scan_chunk(mask_start, self.mask.len(), batch_size)
+ }
- let (initial_skip, chunk_rows, selected_rows, mask_start,
end_position) = {
- let mask = &self.mask;
- let start_position = self.position;
- let mut cursor = start_position;
- let mut initial_skip = 0;
+ /// Find the first selected row without inspecting every skipped bit.
+ fn next_selected_row(&self) -> Option<usize> {
+ let chunks = BitChunks::new(
Review Comment:
💯 for using BitChunks
--
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]
