Skip to content

Commit dbdfcee

Browse files
committed
Address review: infallible RowFilterContext::build_row_filter, lazy initial filter, doc name
- RowFilterContext::try_new only constructs a context when the prebuilt list is non-empty, so build_row_filter now returns RowFilter directly and the unreachable empty arm (and PrebuiltRowFilterCandidateList::is_empty) is gone. - The opener no longer builds the initial RowFilter eagerly just to discard it when the first row group is fully matched; it is built lazily in the non-fully-matched branch only. - Fix doc reference: RowFilterContext::build -> build_row_filter.
1 parent 5a173c9 commit dbdfcee

3 files changed

Lines changed: 16 additions & 32 deletions

File tree

‎datafusion/datasource-parquet/src/opener/mod.rs‎

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1552,26 +1552,25 @@ impl RowGroupsPrunedParquetOpen {
15521552
// installed filter is owned by the decoder and is not
15531553
// recoverable once replaced.
15541554
let first_rg_fully_matched = rg_plan.front().is_some_and(|e| e.fully_matched);
1555-
let initial_filter = precomputed_context
1556-
.as_ref()
1557-
.and_then(|ctx| ctx.build_row_filter());
15581555
let row_filter_context = precomputed_context;
15591556

15601557
let mut builder =
15611558
decoder_config.build(prepared_access_plan, reader_metadata.clone());
15621559
let mut filter_installed = false;
1563-
if let Some(row_filter) = initial_filter {
1560+
if let Some(ctx) = row_filter_context.as_ref() {
15641561
if first_rg_fully_matched {
15651562
// The first RG is fully matched: install an empty filter
15661563
// and count the suppression, exactly as the per-RG toggle
15671564
// does mid-scan, so the metric is consistent whether the
1568-
// skip happens at open time or at a later boundary.
1565+
// skip happens at open time or at a later boundary. The
1566+
// real filter is only built (lazily, from the prebuilt
1567+
// candidates) at the first non-fully-matched boundary.
15691568
builder = builder.with_row_filter(
15701569
parquet::arrow::arrow_reader::RowFilter::new(vec![]),
15711570
);
15721571
row_filter_skipped_fully_matched.add_one();
15731572
} else {
1574-
builder = builder.with_row_filter(row_filter);
1573+
builder = builder.with_row_filter(ctx.build_row_filter());
15751574
filter_installed = true;
15761575
if let Some(max_predicate_cache_size) =
15771576
prepared.max_predicate_cache_size

‎datafusion/datasource-parquet/src/push_decoder.rs‎

Lines changed: 10 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -340,10 +340,6 @@ impl PrebuiltRowFilterCandidateList {
340340
}
341341
}
342342

343-
fn is_empty(&self) -> bool {
344-
self.inner.is_empty()
345-
}
346-
347343
fn as_slice(&self) -> &[PrebuiltRowFilterCandidate] {
348344
&self.inner
349345
}
@@ -397,15 +393,15 @@ impl RowFilterContext {
397393
/// Build a fresh [`RowFilter`] for the next non-fully-matched run using
398394
/// the cached candidates. Cheap: no tree walks, only counter allocation
399395
/// and (optionally) a sort by `required_bytes`.
400-
pub(crate) fn build_row_filter(&self) -> Option<RowFilter> {
401-
if self.prebuilt.is_empty() {
402-
return None;
403-
}
404-
Some(row_filter_from_prebuilt(
396+
///
397+
/// Infallible by construction: [`Self::try_new`] only produces a context
398+
/// when the prebuilt candidate list is non-empty.
399+
pub(crate) fn build_row_filter(&self) -> RowFilter {
400+
row_filter_from_prebuilt(
405401
self.prebuilt.as_slice(),
406402
self.reorder_predicates,
407403
&self.file_metrics,
408-
))
404+
)
409405
}
410406
}
411407

@@ -667,22 +663,11 @@ impl PushDecoderStreamState {
667663
.row_filter_context
668664
.as_ref()
669665
.expect("filter_needs_toggle ⇒ context set");
670-
match ctx.build_row_filter() {
671-
Some(filter) => {
672-
builder = builder.with_row_filter(filter);
673-
if let Some(cap) = ctx.max_predicate_cache_size {
674-
builder = builder.with_max_predicate_cache_size(cap);
675-
}
676-
self.filter_installed = true;
677-
}
678-
None => {
679-
// Filter could not be rebuilt; install an empty filter
680-
// so the decoder runs unfiltered for this run rather
681-
// than failing.
682-
builder = builder.with_row_filter(RowFilter::new(vec![]));
683-
self.filter_installed = false;
684-
}
666+
builder = builder.with_row_filter(ctx.build_row_filter());
667+
if let Some(cap) = ctx.max_predicate_cache_size {
668+
builder = builder.with_max_predicate_cache_size(cap);
685669
}
670+
self.filter_installed = true;
686671
} else {
687672
// Skip per-row filtering for the upcoming fully-matched RG.
688673
builder = builder.with_row_filter(RowFilter::new(vec![]));

‎datafusion/datasource-parquet/src/row_filter.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -428,7 +428,7 @@ pub fn build_row_filter(
428428
/// Extracting this from [`build_row_filter`] lets callers pay the tree-walk +
429429
/// column-resolution + `reassign_expr_columns` cost **once per file** instead
430430
/// of once per row group, which is the hot path for
431-
/// [`RowFilterContext::build`](crate::push_decoder::RowFilterContext) rebuilds
431+
/// [`RowFilterContext::build_row_filter`](crate::push_decoder::RowFilterContext) rebuilds
432432
/// on `fully_matched → not-fully-matched` boundaries.
433433
#[derive(Clone, Debug)]
434434
pub(crate) struct PrebuiltRowFilterCandidate {

0 commit comments

Comments
 (0)