You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "zhuqi-lucas (via GitHub)" <gi...@apache.org> on 2026/06/18 12:28:39 UTC
[PR] feat(parquet): add ParquetPushDecoder::peek_next_row_group() [arrow-rs]
zhuqi-lucas opened a new pull request, #10158:
URL: https://github.com/apache/arrow-rs/pull/10158
# Which issue does this PR close?
Closes #10148.
# Rationale for this change
Adaptive callers that maintain per-row-group state in lock-step with the decoder — e.g. dynamic row-group pruners that re-evaluate row-group statistics mid-scan, or per-RG \`RowFilter\` toggles that skip per-row evaluation when stats prove every row matches — currently have no way to know which row group the next reader will correspond to. \`try_next_reader\` can silently advance past row groups whose row selection is empty under the current \`with_row_selection\`, breaking the assumption that the queue of indices passed to \`with_row_groups\` maps 1:1 to the readers handed back.
This is the API DataFusion's [#22450](https://github.com/apache/datafusion/pull/22450) (TopK runtime row-group pruning) needs to enable a per-RG fully-matched \`RowFilter\` skip optimization that the old \`split_runs\` design previously provided.
# What changes are included in this PR?
A new public method on \`ParquetPushDecoder\`:
\`\`\`rust
pub fn peek_next_row_group(&self) -> Option<usize>
\`\`\`
Returns the file-level row-group index that the next call to \`try_next_reader\` will yield a reader for, after applying any internal skipping (row selection emptiness, exhausted offset/limit budget). Returns \`None\` when no row groups remain, when the decoder sits inside a row group, or when every remaining row group would be skipped.
# Implementation
\`RowGroupFrontier::peek_next_row_group\` clones the offset/limit budget and the row-selection, then runs the same \`split_off\` walk that \`next_readable_row_group\` performs internally — returning the first row-group index whose simulated selection is non-empty (or, with predicates, the first index whose selection is non-empty regardless of budget). The clone keeps the call read-only; the cost is a single extra \`RowSelection::clone\` per peek.
# Are these changes tested?
Yes — four new lib tests:
- \`test_peek_next_row_group_basic\` — peek before / between / after readers on the 2-RG fixture.
- \`test_peek_next_row_group_respects_with_row_groups\` — explicit \`with_row_groups([1])\` reports \`Some(1)\`.
- \`test_peek_next_row_group_skips_empty_selection\` — a \`RowSelection\` that skips all of RG 0 + part of RG 1 makes peek report \`Some(1)\`, mirroring \`next_readable_row_group\`'s skip behavior.
- \`test_peek_next_row_group_finished\` — an empty \`with_row_groups\` (or a finished decoder) returns \`None\`.
All 1219 existing parquet lib tests still pass.
# Are there any user-facing changes?
One new public method on \`ParquetPushDecoder\`. No existing API is changed; nothing breaks.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(parquet): add ParquetPushDecoder::peek_next_row_group() [arrow-rs]
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on code in PR #10158:
URL: https://github.com/apache/arrow-rs/pull/10158#discussion_r3457001607
##########
parquet/src/arrow/push_decoder/mod.rs:
##########
@@ -840,6 +865,19 @@ impl ParquetDecoderState {
}
}
+ fn peek_next_row_group(&self) -> Result<Option<usize>, ParquetError> {
+ match self {
+ ParquetDecoderState::ReadingRowGroup {
+ remaining_row_groups,
+ } => remaining_row_groups.peek_next_row_group(),
+ // We only expose a meaningful answer at row-group boundaries.
+ // Mid-row-group there is no "next" — the active reader is
+ // tied to the current row group.
+ ParquetDecoderState::DecodingRowGroup { .. } => Ok(None),
Review Comment:
This is a great idea, addressed it in latest 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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(parquet): add ParquetPushDecoder::peek_next_row_group() [arrow-rs]
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on code in PR #10158:
URL: https://github.com/apache/arrow-rs/pull/10158#discussion_r3457000656
##########
parquet/src/arrow/push_decoder/mod.rs:
##########
@@ -840,6 +865,19 @@ impl ParquetDecoderState {
}
}
+ fn peek_next_row_group(&self) -> Result<Option<usize>, ParquetError> {
Review Comment:
Good suggestion, addressed in latest 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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(parquet): add ParquetPushDecoder::peek_next_row_group() [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #10158:
URL: https://github.com/apache/arrow-rs/pull/10158#issuecomment-4833213695
Rhank you for your patience @zhuqi-lucas
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(parquet): add ParquetPushDecoder::peek_next_row_group() [arrow-rs]
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on PR #10158:
URL: https://github.com/apache/arrow-rs/pull/10158#issuecomment-4817520384
> I cherry-picked [zhuqi-lucas#4](https://github.com/zhuqi-lucas/arrow-rs/pull/4) to this PR and merged up from main and will merge when CI passes. Thank you @zhuqi-lucas
Thanks @alamb !
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(parquet): add ParquetPushDecoder::peek_next_row_group() [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #10158:
URL: https://github.com/apache/arrow-rs/pull/10158#discussion_r3454695210
##########
parquet/src/arrow/push_decoder/mod.rs:
##########
@@ -538,6 +538,31 @@ impl ParquetPushDecoder {
self.state.row_groups_remaining()
}
+ /// Returns the file-level row-group index that the next call to
Review Comment:
I think `file-level` is somewhat redundant here -- the PushDecoder is always in teh context of a single file
```suggestion
/// Returns the row-group index that the next call to
```
##########
parquet/src/arrow/push_decoder/mod.rs:
##########
@@ -538,6 +538,31 @@ impl ParquetPushDecoder {
self.state.row_groups_remaining()
}
+ /// Returns the file-level row-group index that the next call to
+ /// [`Self::try_next_reader`] will yield a reader for, after applying
+ /// any internal skipping (row selection emptiness, exhausted budget,
+ /// finished state). Returns `Ok(None)` when:
Review Comment:
```suggestion
/// finished state).
///
/// Returns `Ok(None)` when:
```
##########
parquet/src/arrow/push_decoder/remaining.rs:
##########
@@ -93,6 +93,63 @@ impl RowGroupFrontier {
self.budget = budget;
}
+ /// Peek at the next row-group index `next_readable_row_group` would
+ /// hand out, without mutating any state. Returns `None` if every
+ /// remaining row group would be skipped under the current
+ /// selection/budget, or if the queue is empty.
+ ///
+ /// Mirrors the structure of `next_readable_row_group` but only walks
+ /// borrowed state — used by [`super::ParquetPushDecoder::peek_next_row_group`]
+ /// to let adaptive callers (e.g. dynamic row-group pruners or per-RG
+ /// `RowFilter` toggles) keep their per-RG state in lock-step with
+ /// the reader the decoder is about to emit.
+ fn peek_next_row_group(&self) -> Result<Option<usize>, ParquetError> {
+ // Short-circuit: budget exhausted or selection drained ⇒ same
+ // outcome as `next_readable_row_group`'s early return.
+ if self.budget.is_exhausted()
+ || self
+ .selection
+ .as_ref()
+ .is_some_and(|selection| selection.row_count() == 0)
+ {
+ return Ok(None);
+ }
+
+ // We may have to walk past row groups whose split selection is
+ // empty. Cloning the selection lets us run the same `split_off`
+ // logic without disturbing the real one.
+ let mut selection = self.selection.clone();
Review Comment:
😬 it would be nice to avoid cloning the selectin
##########
parquet/src/arrow/push_decoder/mod.rs:
##########
@@ -538,6 +538,31 @@ impl ParquetPushDecoder {
self.state.row_groups_remaining()
}
+ /// Returns the file-level row-group index that the next call to
+ /// [`Self::try_next_reader`] will yield a reader for, after applying
+ /// any internal skipping (row selection emptiness, exhausted budget,
+ /// finished state). Returns `Ok(None)` when:
+ /// - the decoder has no more row groups to read,
+ /// - the decoder is currently inside a row group (consumers should
+ /// call [`Self::is_at_row_group_boundary`] first), or
+ /// - every remaining row group would be skipped.
+ ///
+ /// Returns `Err` when reading row-group metadata fails (e.g.
+ /// `usize` overflow on 32-bit targets), matching the error surface
+ /// of `try_next_reader` so peek and read paths report errors
+ /// consistently.
+ ///
+ /// This is a read-only peek: it does not mutate decoder state. It is
+ /// useful for adaptive callers that maintain per-row-group state in
+ /// lock-step with the decoder (e.g. dynamic row-group pruners or
+ /// per-RG `RowFilter` toggles): without this peek the caller has no
+ /// way to know which row group the next reader actually corresponds
+ /// to, because [`Self::try_next_reader`] may silently advance past
+ /// row groups whose row selection is empty.
Review Comment:
Here is a suggestion to make this more concise for your consideration
```suggestion
/// This method not mutate decoder state. It is
/// useful for callers that maintain per-row-group state in
/// lock-step with the decoder (e.g. dynamic row-group pruners)
/// to determine which row group the next reader corresponds
/// to as [`Self::try_next_reader`] may silently advance past
/// row groups based on filtering and other criteria
```
##########
parquet/src/arrow/push_decoder/remaining.rs:
##########
@@ -93,6 +93,63 @@ impl RowGroupFrontier {
self.budget = budget;
}
+ /// Peek at the next row-group index `next_readable_row_group` would
+ /// hand out, without mutating any state. Returns `None` if every
+ /// remaining row group would be skipped under the current
+ /// selection/budget, or if the queue is empty.
+ ///
+ /// Mirrors the structure of `next_readable_row_group` but only walks
Review Comment:
is it possible to avoid the duplication with `next_readable_row_group`? I worry the two could drift out of sync
##########
parquet/src/arrow/push_decoder/mod.rs:
##########
@@ -538,6 +538,31 @@ impl ParquetPushDecoder {
self.state.row_groups_remaining()
}
+ /// Returns the file-level row-group index that the next call to
+ /// [`Self::try_next_reader`] will yield a reader for, after applying
+ /// any internal skipping (row selection emptiness, exhausted budget,
+ /// finished state). Returns `Ok(None)` when:
+ /// - the decoder has no more row groups to read,
+ /// - the decoder is currently inside a row group (consumers should
+ /// call [`Self::is_at_row_group_boundary`] first), or
+ /// - every remaining row group would be skipped.
+ ///
+ /// Returns `Err` when reading row-group metadata fails (e.g.
+ /// `usize` overflow on 32-bit targets), matching the error surface
+ /// of `try_next_reader` so peek and read paths report errors
+ /// consistently.
Review Comment:
This seems overly detailed (it probably isn't necessary to say that reading will fail when there is an overflo
```suggestion
```
##########
parquet/src/arrow/push_decoder/mod.rs:
##########
@@ -840,6 +865,19 @@ impl ParquetDecoderState {
}
}
+ fn peek_next_row_group(&self) -> Result<Option<usize>, ParquetError> {
Review Comment:
maybe you could add a comment with a link to ParquetPushDecoder::peek_next_row_group?
##########
parquet/src/arrow/push_decoder/mod.rs:
##########
@@ -538,6 +538,31 @@ impl ParquetPushDecoder {
self.state.row_groups_remaining()
}
+ /// Returns the file-level row-group index that the next call to
+ /// [`Self::try_next_reader`] will yield a reader for, after applying
+ /// any internal skipping (row selection emptiness, exhausted budget,
+ /// finished state). Returns `Ok(None)` when:
+ /// - the decoder has no more row groups to read,
+ /// - the decoder is currently inside a row group (consumers should
+ /// call [`Self::is_at_row_group_boundary`] first), or
+ /// - every remaining row group would be skipped.
+ ///
+ /// Returns `Err` when reading row-group metadata fails (e.g.
+ /// `usize` overflow on 32-bit targets), matching the error surface
+ /// of `try_next_reader` so peek and read paths report errors
+ /// consistently.
+ ///
+ /// This is a read-only peek: it does not mutate decoder state. It is
+ /// useful for adaptive callers that maintain per-row-group state in
+ /// lock-step with the decoder (e.g. dynamic row-group pruners or
+ /// per-RG `RowFilter` toggles): without this peek the caller has no
+ /// way to know which row group the next reader actually corresponds
+ /// to, because [`Self::try_next_reader`] may silently advance past
+ /// row groups whose row selection is empty.
+ pub fn peek_next_row_group(&self) -> Result<Option<usize>, ParquetError> {
+ self.state.peek_next_row_group()
+ }
+
Review Comment:
Are you suggesting it actually return more than one row group? Something like
```rust
pub fn peek_next_row_groups(&self) -> Result<Option<&[usize]>, ParquetError> {
```
or just rename it?
##########
parquet/src/arrow/push_decoder/mod.rs:
##########
@@ -840,6 +865,19 @@ impl ParquetDecoderState {
}
}
+ fn peek_next_row_group(&self) -> Result<Option<usize>, ParquetError> {
+ match self {
+ ParquetDecoderState::ReadingRowGroup {
+ remaining_row_groups,
+ } => remaining_row_groups.peek_next_row_group(),
+ // We only expose a meaningful answer at row-group boundaries.
+ // Mid-row-group there is no "next" — the active reader is
+ // tied to the current row group.
+ ParquetDecoderState::DecodingRowGroup { .. } => Ok(None),
Review Comment:
I think there is a well defined next row group even in the middle of reading a row group. Why not return the remaining row groups here? Something like
```rust
ParquetDecoderState::DecodingRowGroup {
remaining_row_groups ,
..
} => remaining_row_groups.peek_next_row_group(),
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(parquet): add ParquetPushDecoder::peek_next_row_group() [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #10158:
URL: https://github.com/apache/arrow-rs/pull/10158
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(parquet): add ParquetPushDecoder::peek_next_row_group() [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #10158:
URL: https://github.com/apache/arrow-rs/pull/10158#issuecomment-4817012693
I cherry-picked https://github.com/zhuqi-lucas/arrow-rs/pull/4 to this PR and merged up from main and will merge when CI passes. Thank you @zhuqi-lucas
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(parquet): add ParquetPushDecoder::peek_next_row_group() [arrow-rs]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on code in PR #10158:
URL: https://github.com/apache/arrow-rs/pull/10158#discussion_r3453537189
##########
parquet/src/arrow/push_decoder/mod.rs:
##########
@@ -538,6 +538,31 @@ impl ParquetPushDecoder {
self.state.row_groups_remaining()
}
+ /// Returns the file-level row-group index that the next call to
+ /// [`Self::try_next_reader`] will yield a reader for, after applying
+ /// any internal skipping (row selection emptiness, exhausted budget,
+ /// finished state). Returns `Ok(None)` when:
+ /// - the decoder has no more row groups to read,
+ /// - the decoder is currently inside a row group (consumers should
+ /// call [`Self::is_at_row_group_boundary`] first), or
+ /// - every remaining row group would be skipped.
+ ///
+ /// Returns `Err` when reading row-group metadata fails (e.g.
+ /// `usize` overflow on 32-bit targets), matching the error surface
+ /// of `try_next_reader` so peek and read paths report errors
+ /// consistently.
+ ///
+ /// This is a read-only peek: it does not mutate decoder state. It is
+ /// useful for adaptive callers that maintain per-row-group state in
+ /// lock-step with the decoder (e.g. dynamic row-group pruners or
+ /// per-RG `RowFilter` toggles): without this peek the caller has no
+ /// way to know which row group the next reader actually corresponds
+ /// to, because [`Self::try_next_reader`] may silently advance past
+ /// row groups whose row selection is empty.
+ pub fn peek_next_row_group(&self) -> Result<Option<usize>, ParquetError> {
+ self.state.peek_next_row_group()
+ }
+
Review Comment:
Should we make this `peek_remaining_row_groups()`? That might enable more usages in the future.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(parquet): add ParquetPushDecoder::peek_next_row_group() [arrow-rs]
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on PR #10158:
URL: https://github.com/apache/arrow-rs/pull/10158#issuecomment-4815750465
> Thank you @zhuqi-lucas -- I have thought more about this PR and API and the more I think about it the more I think it would be better if we just went with the clone of the selection approach initially
>
> I made a PR to update this PR with the proposal, for your consideration
>
> * [zhuqi-lucas#4](https://github.com/zhuqi-lucas/arrow-rs/pull/4)
>
> What I suggest we do is:
>
> 1. Simplify this PR and merge in the inefficient implementation
> 2. Work on some ways to improve the implementation's performance as a follow on / when we have additional evidence that the `peek_next_row_group` is too slow
>
> Some other ideas about improving the performance without a second copy of the code:
>
> 1. Maybe we can split the "decide how to advance the selection / row group" logic from the "actually mutate the state" - then we would call the decision logic in peek_next_row_group 🤔
Thank you @alamb , i approved the [zhuqi-lucas#4](https://github.com/zhuqi-lucas/arrow-rs/pull/4) , but i have no permission to merge it.
I agree we can optimize performance as a folllow up.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(parquet): add ParquetPushDecoder::peek_next_row_group() [arrow-rs]
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on code in PR #10158:
URL: https://github.com/apache/arrow-rs/pull/10158#discussion_r3457002178
##########
parquet/src/arrow/push_decoder/remaining.rs:
##########
@@ -93,6 +93,63 @@ impl RowGroupFrontier {
self.budget = budget;
}
+ /// Peek at the next row-group index `next_readable_row_group` would
+ /// hand out, without mutating any state. Returns `None` if every
+ /// remaining row group would be skipped under the current
+ /// selection/budget, or if the queue is empty.
+ ///
+ /// Mirrors the structure of `next_readable_row_group` but only walks
+ /// borrowed state — used by [`super::ParquetPushDecoder::peek_next_row_group`]
+ /// to let adaptive callers (e.g. dynamic row-group pruners or per-RG
+ /// `RowFilter` toggles) keep their per-RG state in lock-step with
+ /// the reader the decoder is about to emit.
+ fn peek_next_row_group(&self) -> Result<Option<usize>, ParquetError> {
+ // Short-circuit: budget exhausted or selection drained ⇒ same
+ // outcome as `next_readable_row_group`'s early return.
+ if self.budget.is_exhausted()
+ || self
+ .selection
+ .as_ref()
+ .is_some_and(|selection| selection.row_count() == 0)
+ {
+ return Ok(None);
+ }
+
+ // We may have to walk past row groups whose split selection is
+ // empty. Cloning the selection lets us run the same `split_off`
+ // logic without disturbing the real one.
+ let mut selection = self.selection.clone();
Review Comment:
Good point!
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(parquet): add ParquetPushDecoder::peek_next_row_group() [arrow-rs]
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on PR #10158:
URL: https://github.com/apache/arrow-rs/pull/10158#issuecomment-4786279276
Heard you @alamb — the PR did drift into "let's also add a more general API and a visitor framework" territory during the back-and-forth. Pushed `4cdc6d6eb` to trim back to the actual primitive the dynamic-pruner follow-up needs:
**Kept**
* Single-value `peek_next_row_group()`
* `PeekSelectionCursor` (no-clone walker over `RowSelection`)
* `DecodingRowGroup` arm returns the next row group rather than `None`
* Doc rewordings applied earlier via GitHub suggestions
* 8 peek tests covering basic flow, `with_row_groups`, selection-skip, finished state, OFFSET single-RG skip, OFFSET drains all, mid-row-group `try_decode`, and the read-only invariant
**Dropped**
* `peek_remaining_row_groups` at every layer (no consumer for it)
* `walk_peekable` + `PeekStep` visitor framework (only existed to share a loop body with the multi-value variant)
* `RowGroupAction` + `classify_row_group` shared classifier — restored the original `QueuedRowGroupDecision` + `plan_selected_row_group` on `next_readable_row_group`. The Read/Skip rule in `peek_next_row_group` is now a 5-line inline mirror of `plan_selected_row_group` with a comment asking future edits to keep them in lock-step. The existing `test_peek_next_row_group_basic` drives peek and `try_next_reader` interleaved across both row groups, so any drift surfaces immediately.
* Eight multi-value tests that only mattered for the now-removed API.
Net change vs the previous head: `−280 lines`. 1224 / 1224 parquet tests pass, clippy clean. Happy to cut further if anything still feels like scope creep.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(parquet): add ParquetPushDecoder::peek_next_row_group() [arrow-rs]
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on code in PR #10158:
URL: https://github.com/apache/arrow-rs/pull/10158#discussion_r3456922369
##########
parquet/src/arrow/push_decoder/mod.rs:
##########
@@ -538,6 +538,31 @@ impl ParquetPushDecoder {
self.state.row_groups_remaining()
}
+ /// Returns the file-level row-group index that the next call to
+ /// [`Self::try_next_reader`] will yield a reader for, after applying
+ /// any internal skipping (row selection emptiness, exhausted budget,
+ /// finished state). Returns `Ok(None)` when:
+ /// - the decoder has no more row groups to read,
+ /// - the decoder is currently inside a row group (consumers should
+ /// call [`Self::is_at_row_group_boundary`] first), or
+ /// - every remaining row group would be skipped.
+ ///
+ /// Returns `Err` when reading row-group metadata fails (e.g.
+ /// `usize` overflow on 32-bit targets), matching the error surface
+ /// of `try_next_reader` so peek and read paths report errors
+ /// consistently.
+ ///
+ /// This is a read-only peek: it does not mutate decoder state. It is
+ /// useful for adaptive callers that maintain per-row-group state in
+ /// lock-step with the decoder (e.g. dynamic row-group pruners or
+ /// per-RG `RowFilter` toggles): without this peek the caller has no
+ /// way to know which row group the next reader actually corresponds
+ /// to, because [`Self::try_next_reader`] may silently advance past
+ /// row groups whose row selection is empty.
+ pub fn peek_next_row_group(&self) -> Result<Option<usize>, ParquetError> {
+ self.state.peek_next_row_group()
+ }
+
Review Comment:
Good point, future work may dependent on this.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(parquet): add ParquetPushDecoder::peek_next_row_group() [arrow-rs]
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on PR #10158:
URL: https://github.com/apache/arrow-rs/pull/10158#issuecomment-4775559633
Addressed in 74435caef (force-push pending if you want to squash). Summary of what changed and how each comment was handled:
**alamb's doc comments** (`mod.rs:541-561`) — applied via GitHub suggestions, then I tightened the rewording slightly to call out the new mid-row-group semantics. Public doc now reads "Safe to call at any time" and explicitly states what mid-row-group means.
**alamb @ `mod.rs:868`** (link inner helper to public doc) — done.
**alamb @ `mod.rs:876`** (return next RG in `DecodingRowGroup` state) — done. The `DecodingRowGroup` arm now delegates to `remaining_row_groups.peek_next_row_group()` and returns the row group `try_next_reader` will produce *after* the active one. New test `test_peek_next_row_group_during_decoding_row_group` covers this — drives `try_decode` mid-RG with a small batch size and asserts peek reports the next RG, not `None`.
**alamb @ `remaining.rs:101`** (drift concern with `next_readable_row_group`) — addressed by extracting `classify_row_group` (predicate-and-budget Read/Skip rule) as a private helper that both walkers call. The duplicated branch is gone; new test `test_peek_next_row_group_does_not_mutate_state` calls peek repeatedly between reads and asserts each pair agrees, locking in the lock-step contract.
**alamb @ `remaining.rs:121`** (avoid the selection clone) — replaced with a borrowed `PeekSelectionCursor` that walks `RowSelection::iter` with a cursor and counts selected rows per row group without disturbing the real selection. No more `selection.clone()` in peek.
**adriangb @ `mod.rs:565`** (rename to `peek_remaining_row_groups`?) — keeping the single-value `peek_next_row_group` for now. The only consumer I have (DataFusion runtime RG pruning follow-up) needs exactly the next index, and you said *"maybe it's worth doing the simpler API for now and we can always add another one later"* — taking you up on that. Happy to add a separate `peek_remaining_row_groups` returning `&[usize]` in a follow-up when there's a concrete second use case.
Existing 6 peek tests + 2 new tests all pass (46 push_decoder tests, 1223 parquet tests, clippy clean).
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(parquet): add ParquetPushDecoder::peek_next_row_group() [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #10158:
URL: https://github.com/apache/arrow-rs/pull/10158#discussion_r3481220941
##########
parquet/src/arrow/push_decoder/remaining.rs:
##########
@@ -37,6 +37,58 @@ enum QueuedRowGroupDecision {
Skip { remaining_budget: RowBudget },
}
+/// Borrowed cursor over a [`RowSelection`] that counts selected rows in
+/// each row group's slice without mutating the selection.
Review Comment:
this is a neat idea, but the more I see the more I think it may be overly fancy / premature optimization
##########
parquet/src/arrow/push_decoder/remaining.rs:
##########
@@ -93,6 +145,62 @@ impl RowGroupFrontier {
self.budget = budget;
}
+ /// Peek at the next row-group index `next_readable_row_group` would
+ /// hand out, without mutating any state. Returns `None` if every
+ /// remaining row group would be skipped under the current
+ /// selection/budget, or if the queue is empty.
+ ///
+ /// Walks the queued frontier via [`PeekSelectionCursor`] so the
+ /// real `RowSelection` is not cloned. The Read/Skip rule inlined
+ /// below is intentionally kept in lock-step with
+ /// [`Self::plan_selected_row_group`]; both touch a small enough
+ /// set of decisions that a shared helper would obscure more than
+ /// it saves. The `peek_matches_next_readable_first_hit` test
+ /// asserts the lock-step on the head element across a range of
+ /// inputs.
+ fn peek_next_row_group(&self) -> Result<Option<usize>, ParquetError> {
+ // Short-circuit: budget exhausted or selection drained ⇒ same
+ // outcome as `next_readable_row_group`'s early return.
+ if self.budget.is_exhausted()
Review Comment:
I worry that this is very similar to next_readeable_row_group and that the implementations can drift out of sync over time
https://github.com/apache/arrow-rs/blob/fe0b993a14ddb963dcf6698ec4d90d9ec1911483/parquet/src/arrow/push_decoder/remaining.rs#L126-L125
##########
parquet/src/arrow/push_decoder/mod.rs:
##########
@@ -538,6 +538,31 @@ impl ParquetPushDecoder {
self.state.row_groups_remaining()
}
+ /// Returns the file-level row-group index that the next call to
+ /// [`Self::try_next_reader`] will yield a reader for, after applying
+ /// any internal skipping (row selection emptiness, exhausted budget,
+ /// finished state). Returns `Ok(None)` when:
+ /// - the decoder has no more row groups to read,
+ /// - the decoder is currently inside a row group (consumers should
+ /// call [`Self::is_at_row_group_boundary`] first), or
+ /// - every remaining row group would be skipped.
+ ///
+ /// Returns `Err` when reading row-group metadata fails (e.g.
+ /// `usize` overflow on 32-bit targets), matching the error surface
+ /// of `try_next_reader` so peek and read paths report errors
+ /// consistently.
+ ///
+ /// This is a read-only peek: it does not mutate decoder state. It is
+ /// useful for adaptive callers that maintain per-row-group state in
+ /// lock-step with the decoder (e.g. dynamic row-group pruners or
+ /// per-RG `RowFilter` toggles): without this peek the caller has no
+ /// way to know which row group the next reader actually corresponds
+ /// to, because [`Self::try_next_reader`] may silently advance past
+ /// row groups whose row selection is empty.
+ pub fn peek_next_row_group(&self) -> Result<Option<usize>, ParquetError> {
+ self.state.peek_next_row_group()
+ }
+
Review Comment:
Let's file a follow on ticket to consider adding additional APIs that will expose more information. For now, the next row group is a step forward
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(parquet): add ParquetPushDecoder::peek_next_row_group() [arrow-rs]
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on code in PR #10158:
URL: https://github.com/apache/arrow-rs/pull/10158#discussion_r3457002963
##########
parquet/src/arrow/push_decoder/remaining.rs:
##########
@@ -93,6 +93,63 @@ impl RowGroupFrontier {
self.budget = budget;
}
+ /// Peek at the next row-group index `next_readable_row_group` would
+ /// hand out, without mutating any state. Returns `None` if every
+ /// remaining row group would be skipped under the current
+ /// selection/budget, or if the queue is empty.
+ ///
+ /// Mirrors the structure of `next_readable_row_group` but only walks
Review Comment:
Good point, addressed in latest 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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(parquet): add ParquetPushDecoder::peek_next_row_group() [arrow-rs]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on code in PR #10158:
URL: https://github.com/apache/arrow-rs/pull/10158#discussion_r3455440703
##########
parquet/src/arrow/push_decoder/mod.rs:
##########
@@ -538,6 +538,31 @@ impl ParquetPushDecoder {
self.state.row_groups_remaining()
}
+ /// Returns the file-level row-group index that the next call to
+ /// [`Self::try_next_reader`] will yield a reader for, after applying
+ /// any internal skipping (row selection emptiness, exhausted budget,
+ /// finished state). Returns `Ok(None)` when:
+ /// - the decoder has no more row groups to read,
+ /// - the decoder is currently inside a row group (consumers should
+ /// call [`Self::is_at_row_group_boundary`] first), or
+ /// - every remaining row group would be skipped.
+ ///
+ /// Returns `Err` when reading row-group metadata fails (e.g.
+ /// `usize` overflow on 32-bit targets), matching the error surface
+ /// of `try_next_reader` so peek and read paths report errors
+ /// consistently.
+ ///
+ /// This is a read-only peek: it does not mutate decoder state. It is
+ /// useful for adaptive callers that maintain per-row-group state in
+ /// lock-step with the decoder (e.g. dynamic row-group pruners or
+ /// per-RG `RowFilter` toggles): without this peek the caller has no
+ /// way to know which row group the next reader actually corresponds
+ /// to, because [`Self::try_next_reader`] may silently advance past
+ /// row groups whose row selection is empty.
+ pub fn peek_next_row_group(&self) -> Result<Option<usize>, ParquetError> {
+ self.state.peek_next_row_group()
+ }
+
Review Comment:
I am suggesting it return more than one. I'm not *sure* if that's helpful, but I think we have that information and if we do we might as well return it. It would avoid adding that API later and then there being overlap, but maybe it's worth doing the simpler API for now and we can always add another one later.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(parquet): add ParquetPushDecoder::peek_next_row_group() [arrow-rs]
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on PR #10158:
URL: https://github.com/apache/arrow-rs/pull/10158#issuecomment-4765640772
cc @adriangb @alamb
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(parquet): add ParquetPushDecoder::peek_next_row_group() [arrow-rs]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #10158:
URL: https://github.com/apache/arrow-rs/pull/10158#discussion_r3435772847
##########
parquet/src/arrow/push_decoder/mod.rs:
##########
@@ -1743,6 +1776,93 @@ mod test {
expect_finished(decoder.try_decode());
}
+ /// `peek_next_row_group` reports the index of the row group the
+ /// next `try_next_reader` call will hand back, matching the
+ /// frontier's internal skip logic.
+ #[test]
+ fn test_peek_next_row_group_basic() {
+ let mut decoder = ParquetPushDecoderBuilder::try_new_decoder(test_file_parquet_metadata())
+ .unwrap()
+ .build()
+ .unwrap();
+
+ // Two row groups (0, 1). At boundary before any read, peek should
+ // see RG 0.
+ assert_eq!(decoder.peek_next_row_group(), Some(0));
+ assert!(decoder.is_at_row_group_boundary());
+
+ let ranges = expect_needs_data(decoder.try_next_reader());
+ push_ranges_to_decoder(&mut decoder, ranges);
+ let reader = expect_data(decoder.try_next_reader());
+ // Once the reader for RG 0 has been handed off, the decoder is
+ // back at a boundary waiting for RG 1 — peek must reflect that
+ // (the active reader lives outside the decoder).
+ assert!(decoder.is_at_row_group_boundary());
+ assert_eq!(decoder.peek_next_row_group(), Some(1));
+
+ // Drain RG 0's reader and consume RG 1.
+ for batch in reader {
+ let _ = batch.unwrap();
+ }
+ let ranges = expect_needs_data(decoder.try_next_reader());
+ push_ranges_to_decoder(&mut decoder, ranges);
+ let reader = expect_data(decoder.try_next_reader());
+ for batch in reader {
+ let _ = batch.unwrap();
+ }
+
+ // No row groups left.
+ assert_eq!(decoder.peek_next_row_group(), None);
+ }
+
+ /// `peek_next_row_group` honors `with_row_groups` — restricting the
+ /// scan to a single row group means peek reports only that one and
+ /// then `None`.
+ #[test]
+ fn test_peek_next_row_group_respects_with_row_groups() {
+ let decoder = ParquetPushDecoderBuilder::try_new_decoder(test_file_parquet_metadata())
+ .unwrap()
+ .with_row_groups(vec![1])
+ .build()
+ .unwrap();
+
+ assert_eq!(decoder.peek_next_row_group(), Some(1));
+ }
+
+ /// When a row-selection segment leaves the next row group with zero
+ /// selected rows, `peek_next_row_group` mirrors
+ /// `next_readable_row_group`'s skip: it returns the *following*
+ /// row group instead of the empty one.
+ #[test]
Review Comment:
The new API documents that peeking simulates offset/limit budget skipping, but the added tests don't cover budget-driven row-group skipping. Adding a small test for `with_offset` (or `with_limit`) would help ensure `peek_next_row_group` stays aligned with `next_readable_row_group`'s budget logic.
##########
parquet/src/arrow/push_decoder/remaining.rs:
##########
@@ -93,6 +93,62 @@ impl RowGroupFrontier {
self.budget = budget;
}
+ /// Peek at the next row-group index `next_readable_row_group` would
+ /// hand out, without mutating any state. Returns `None` if every
+ /// remaining row group would be skipped under the current
+ /// selection/budget, or if the queue is empty.
+ ///
+ /// Mirrors the structure of `next_readable_row_group` but only walks
+ /// borrowed state — used by [`super::ParquetPushDecoder::peek_next_row_group`]
+ /// to let adaptive callers (e.g. dynamic row-group pruners or per-RG
+ /// `RowFilter` toggles) keep their per-RG state in lock-step with
+ /// the reader the decoder is about to emit.
+ fn peek_next_row_group(&self) -> Option<usize> {
+ // Short-circuit: budget exhausted or selection drained ⇒ same
+ // outcome as `next_readable_row_group`'s early return.
+ if self.budget.is_exhausted()
+ || self
+ .selection
+ .as_ref()
+ .is_some_and(|selection| selection.row_count() == 0)
+ {
+ return None;
+ }
+
+ // We may have to walk past row groups whose split selection is
+ // empty. Cloning the selection lets us run the same `split_off`
+ // logic without disturbing the real one.
+ let mut selection = self.selection.clone();
+ let mut budget = self.budget;
+ for &row_group_idx in &self.row_groups {
+ let row_count = self.row_group_num_rows(row_group_idx).ok()?;
+ let selected_rows = match selection.as_mut() {
Review Comment:
`peek_next_row_group` currently converts `row_group_num_rows` errors into `None` via `.ok()?`. This can mask real metadata problems (e.g. row-count overflow on 32-bit targets), making the decoder look "finished" even though `try_next_reader` would return `Err`. Consider returning `Result<Option<usize>, ParquetError>` (and plumbing it through `RemainingRowGroups`/`ParquetPushDecoder`) so peek and read paths report errors consistently.
##########
parquet/src/arrow/push_decoder/remaining.rs:
##########
@@ -93,6 +93,62 @@ impl RowGroupFrontier {
self.budget = budget;
}
+ /// Peek at the next row-group index `next_readable_row_group` would
+ /// hand out, without mutating any state. Returns `None` if every
+ /// remaining row group would be skipped under the current
+ /// selection/budget, or if the queue is empty.
+ ///
+ /// Mirrors the structure of `next_readable_row_group` but only walks
+ /// borrowed state — used by [`super::ParquetPushDecoder::peek_next_row_group`]
+ /// to let adaptive callers (e.g. dynamic row-group pruners or per-RG
+ /// `RowFilter` toggles) keep their per-RG state in lock-step with
+ /// the reader the decoder is about to emit.
+ fn peek_next_row_group(&self) -> Option<usize> {
+ // Short-circuit: budget exhausted or selection drained ⇒ same
+ // outcome as `next_readable_row_group`'s early return.
+ if self.budget.is_exhausted()
+ || self
+ .selection
+ .as_ref()
+ .is_some_and(|selection| selection.row_count() == 0)
+ {
+ return None;
+ }
+
+ // We may have to walk past row groups whose split selection is
+ // empty. Cloning the selection lets us run the same `split_off`
+ // logic without disturbing the real one.
+ let mut selection = self.selection.clone();
+ let mut budget = self.budget;
+ for &row_group_idx in &self.row_groups {
+ let row_count = self.row_group_num_rows(row_group_idx).ok()?;
+ let selected_rows = match selection.as_mut() {
+ Some(remaining) => {
+ let rg_segment = remaining.split_off(row_count);
+ rg_segment.row_count()
+ }
+ None => row_count,
+ };
+ if selected_rows == 0 {
+ // Same skip path as `next_readable_row_group`: row
+ // selection drained for this RG, move on.
+ continue;
+ }
+ if self.has_predicates {
+ // Predicates → always read, regardless of budget.
+ return Some(row_group_idx);
+ }
Review Comment:
The comment "Predicates → always read, regardless of budget" is slightly misleading since the function still returns `None` when the budget is exhausted. Rewording this makes the intended meaning ("budget-based row-group skipping is disabled when predicates are present") 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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(parquet): add ParquetPushDecoder::peek_next_row_group() [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #10158:
URL: https://github.com/apache/arrow-rs/pull/10158#issuecomment-4783663896
I will try and review this again tomorrow -- it seems to me like we maybe have given you bad advice and this pR has now grown to be quite complicated 🤔
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(parquet): add ParquetPushDecoder::peek_next_row_group() [arrow-rs]
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on PR #10158:
URL: https://github.com/apache/arrow-rs/pull/10158#issuecomment-4776388089
Thank you @alamb and @adriangb for review, addressed review comments and also added more test cases in latest 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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(parquet): add ParquetPushDecoder::peek_next_row_group() [arrow-rs]
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on PR #10158:
URL: https://github.com/apache/arrow-rs/pull/10158#issuecomment-4776378780
Pushed `c64c08906` with four additional combination tests to lock down the interactions DataFusion's runtime row-group pruning path will rely on:
* `test_peek_remaining_row_groups_during_decoding_row_group` — multi-value peek mid-`try_decode` iteration (symmetric to the single-value DecodingRowGroup test).
* `test_peek_remaining_row_groups_with_offset_and_limit` — OFFSET + LIMIT both push down; verifies the simulated budget chains correctly across Read decisions.
* `test_peek_remaining_row_groups_offset_skips_then_limit_caps` — OFFSET silent-skips RG 0, LIMIT then caps RG 1; verifies the Skip path shrinks offset rather than treating budget as exhausted.
* `test_peek_remaining_row_groups_selection_then_budget_skip` — composes both silent-skip paths (selection-empty RG + budget-skip RG) in one walk; expected empty result.
12 peek tests total (6 original + 6 added), 1232 parquet tests pass, clippy clean. Ready for another look whenever you have a moment.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org