zhuqi-lucas commented on code in PR #23696:
URL: https://github.com/apache/datafusion/pull/23696#discussion_r3766671273
##########
datafusion/datasource-parquet/src/opener/mod.rs:
##########
@@ -1435,18 +1434,26 @@ impl RowGroupsPrunedParquetOpen {
prepared.virtual_state.as_deref(),
)?;
- let (decoder, rg_plan) = {
+ let (decoder, rg_plan, filter_installed, row_filter_context) = {
Review Comment:
Good idea — will factor the initial decoder / rg_plan / filter setup into a
helper returning a named struct (e.g. `InitialDecoderState`) as a follow-up,
keeping this PR focused on the correctness fixes. Filed #24286 to track it.
##########
datafusion/datasource-parquet/src/access_plan.rs:
##########
@@ -571,12 +571,81 @@ impl ParquetAccessPlan {
row_group_meta_data: &[RowGroupMetaData],
) -> Result<PreparedAccessPlan> {
let row_group_indexes = self.row_group_indexes();
+ // Carry `fully_matched` flags in the same order as
+ // `row_group_indexes` so downstream code (per-RG `RowFilter` skip)
+ // can look them up positionally.
+ let fully_matched: Vec<bool> = row_group_indexes
+ .iter()
+ .map(|&idx| self.fully_matched[idx])
+ .collect();
let row_selection =
self.into_overall_row_selection(row_group_meta_data)?;
- PreparedAccessPlan::new(row_group_indexes, row_selection)
+ let (row_group_indexes, fully_matched, row_selection) =
strip_empty_row_groups(
+ row_group_indexes,
+ fully_matched,
+ row_selection,
+ row_group_meta_data,
+ );
+
+ PreparedAccessPlan::new(row_group_indexes, fully_matched,
row_selection)
}
}
+/// Strip row groups whose post-pruning `RowSelection` selects zero rows.
+///
+/// arrow-rs's push decoder silently advances past such row groups inside
+/// `try_next_reader`, but the rest of DataFusion (per-RG metadata maps,
+/// the runtime dynamic-pruner, the per-RG `RowFilter` toggle) assumes a
+/// 1:1 correspondence between the prepared plan and the readers the
+/// decoder hands back. Removing these empty entries here keeps that
+/// invariant and lets downstream code consult per-RG state — like
+/// [`PreparedAccessPlan::fully_matched`] — without going out of sync.
+///
+/// The flat `RowSelection` is split per row group with
+/// [`RowSelection::split_off`] (mirroring arrow-rs's own logic) and the
+/// surviving segments are concatenated back into the result selection.
+/// When `row_selection` is `None` (no page-index pruning, no
+/// user-supplied selection) no row group can be empty and the inputs are
+/// returned unchanged.
Review Comment:
Correction to my earlier reply here — I got this wrong:
`strip_empty_row_groups` is added by *this* PR (commit df4c62e), not #22450
(which merged back in June), and it is not on `main`. It is currently coupled
to `fully_matched` — it keeps `row_group_indexes`, `fully_matched`, and the
`RowSelection` aligned while stripping empty RGs. You are right that the
empty-RG stripping itself is generic, so I filed #24287 to extract that generic
part into its own follow-up PR once this lands.
--
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]
