viirya commented on code in PR #11165:
URL: https://github.com/apache/arrow-rs/pull/11165#discussion_r4068973102
##########
parquet/src/arrow/push_decoder/mod.rs:
##########
@@ -605,6 +619,36 @@ impl ParquetPushDecoder {
self.state.peek_next_row_group()
}
+ /// Preview at most two filter-free row groups using the demand range
planner.
+ ///
+ /// Returns `None` outside a row-group boundary or when row predicates are
+ /// present; returns an empty vector when there is no selected work. This
+ /// never advances the decoder, evaluates predicates, reads data, or
creates
+ /// batch readers. Ranges fully covered by an input buffer are excluded;
Review Comment:
Could we make explicit that all preview entries use the same current buffer
state, without simulating buffer consumption by earlier entries? For example,
with a plan of `[B, B]` and B already buffered, both entries can have empty
ranges even though the first read may consume those buffers and the second read
subsequently requests B again. The duplicate-group test covers this, but the
distinction would also be useful in the public API docs.
Relatedly, did you consider returning all required ranges independently of
buffer coverage? The current behavior matches `NeedsData`, which is useful, but
I would like to understand why it is the preferred contract for a caller
managing its own read-ahead cache.
##########
parquet/src/arrow/push_decoder/mod.rs:
##########
@@ -605,6 +619,36 @@ impl ParquetPushDecoder {
self.state.peek_next_row_group()
}
+ /// Preview at most two filter-free row groups using the demand range
planner.
+ ///
+ /// Returns `None` outside a row-group boundary or when row predicates are
+ /// present; returns an empty vector when there is no selected work. This
+ /// never advances the decoder, evaluates predicates, reads data, or
creates
+ /// batch readers. Ranges fully covered by an input buffer are excluded;
+ /// partially covered ranges are returned unchanged, matching `NeedsData`.
+ ///
+ /// `max_row_groups` must be 1 or 2. Errors leave the decoder unchanged;
+ /// speculative callers should defer them to normal ordered demand. The
+ /// snapshot is only advisory: see [`RowGroupRangePreview`]. Cost includes
+ /// cloning the remaining row-group plan and selections.
+ pub fn preview_row_group_ranges(
+ &self,
+ max_row_groups: usize,
+ ) -> Result<Option<Vec<RowGroupRangePreview>>, ParquetError> {
+ if !(1..=2).contains(&max_row_groups) {
Review Comment:
What motivates restricting `max_row_groups` to exactly 1 or 2? The
implementation appears capable of walking more groups, and a scheduler reading
from high-latency storage may want a deeper window. Bounding each call makes
sense, but could the caller choose that bound through this parameter? If the
cap is intentional for the initial API, could we document the use case or
tradeoff behind it?
##########
parquet/src/arrow/async_reader/mod.rs:
##########
@@ -580,6 +580,15 @@ impl<T: AsyncFileReader + Send + 'static>
ParquetRecordBatchStreamBuilder<T> {
Self::new_builder(AsyncReader(input), metadata)
}
+ /// Consume this builder and return its underlying async reader.
Review Comment:
This ownership handoff looks useful and straightforward. One clarification
for the motivating workflow: metadata-only preparation can already use
`AsyncFileReader::get_metadata()` or `ArrowReaderMetadata::load_async(&mut
reader, ...)` without transferring ownership to a stream builder. The builder’s
Bloom-filter helper seems to be the stronger motivation here. As a possible
follow-up, would it make sense to expose that helper independently of the
builder as well? That need not block adding `into_inner()`.
##########
parquet/src/arrow/push_decoder/mod.rs:
##########
@@ -605,6 +619,36 @@ impl ParquetPushDecoder {
self.state.peek_next_row_group()
}
+ /// Preview at most two filter-free row groups using the demand range
planner.
+ ///
+ /// Returns `None` outside a row-group boundary or when row predicates are
+ /// present; returns an empty vector when there is no selected work. This
+ /// never advances the decoder, evaluates predicates, reads data, or
creates
+ /// batch readers. Ranges fully covered by an input buffer are excluded;
+ /// partially covered ranges are returned unchanged, matching `NeedsData`.
+ ///
+ /// `max_row_groups` must be 1 or 2. Errors leave the decoder unchanged;
+ /// speculative callers should defer them to normal ordered demand. The
+ /// snapshot is only advisory: see [`RowGroupRangePreview`]. Cost includes
Review Comment:
Thanks for documenting the cloning cost. Since this API is intended for
read-ahead, callers may invoke it at every row-group boundary. Cloning the
entire remaining queue on each call gives quadratic cumulative queue-copying
work across a scan, even with a preview depth of one or two. Have you evaluated
this for files with many row groups or large selections? A focused measurement
would help establish whether this is an acceptable initial tradeoff.
##########
parquet/src/arrow/push_decoder/mod.rs:
##########
@@ -605,6 +619,36 @@ impl ParquetPushDecoder {
self.state.peek_next_row_group()
}
+ /// Preview at most two filter-free row groups using the demand range
planner.
+ ///
+ /// Returns `None` outside a row-group boundary or when row predicates are
Review Comment:
Could we link to `is_at_row_group_boundary()` and add a short usage example
with `try_next_reader()`? A loop using only `try_decode()` does not expose an
intermediate previewable boundary: returned batches leave the decoder in
`DecodingRowGroup`, and subsequent calls advance internally into the next
group. An example showing when to preview, start speculative I/O, and reconcile
it with ordered demand would make the intended usage much clearer.
--
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]
