zhuqi-lucas commented on code in PR #24354:
URL: https://github.com/apache/datafusion/pull/24354#discussion_r3781606891
##########
datafusion/datasource-parquet/src/push_decoder.rs:
##########
@@ -342,6 +342,20 @@ impl PushDecoderStreamState {
.as_ref()
.expect("decoder present")
.is_at_row_group_boundary();
+ // Only the runtime pruner rebuilds the decoder from `rg_plan`, so
+ // only it needs `rg_plan` kept in sync with the decoder frontier.
+ // arrow-rs silently finishes row groups whose post-predicate
+ // selection is empty without handing back a reader, so without
this
+ // sync `rg_plan` trails the decoder by one and a rebuild re-reads
an
+ // already-delivered row group (#24352). Gating on the pruner also
+ // avoids the O(remaining row groups) cost of
`peek_next_row_group()`
Review Comment:
Right — gating the sync on `row_group_pruner.is_some()` is exactly to avoid
that `peek` clone on scans that never rebuild. Only the pruner rebuilds from
`rg_plan`, so only it needs `rg_plan` kept in sync.
##########
datafusion/datasource-parquet/src/push_decoder.rs:
##########
@@ -342,6 +342,20 @@ impl PushDecoderStreamState {
.as_ref()
.expect("decoder present")
.is_at_row_group_boundary();
+ // Only the runtime pruner rebuilds the decoder from `rg_plan`, so
+ // only it needs `rg_plan` kept in sync with the decoder frontier.
+ // arrow-rs silently finishes row groups whose post-predicate
+ // selection is empty without handing back a reader, so without
this
+ // sync `rg_plan` trails the decoder by one and a rebuild re-reads
an
+ // already-delivered row group (#24352). Gating on the pruner also
+ // avoids the O(remaining row groups) cost of
`peek_next_row_group()`
+ // on ordinary scans that never rebuild.
+ if at_boundary
+ && self.row_group_pruner.is_some()
Review Comment:
Fair — the gate does couple `rg_plan`'s correctness to `row_group_pruner`.
It is sound today because the pruner is the only thing that rebuilds from
`rg_plan`, but you are right that it is a fragile coupling. The clean fix is
the `remaining_row_groups()` restructure: with no parallel `rg_plan`, there is
no "correctness conditional on an unrelated field" to worry about.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
