perf(parquet): skip RowFilter on statically fully-matched row groups - #23696
Open
zhuqi-lucas wants to merge 11 commits into
Open
perf(parquet): skip RowFilter on statically fully-matched row groups#23696zhuqi-lucas wants to merge 11 commits into
zhuqi-lucas wants to merge 11 commits into
Conversation
github-actions
Bot
added
core
zhuqi-lucas
marked this pull request as draft
July 19, 2026 03:28
Contributor
Author
|
run benchmarks |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing perf/parquet-skip-filter-for-full-match (a3047ed) to 67947b6 (merge-base) diff using: tpcds File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing perf/parquet-skip-filter-for-full-match (a3047ed) to 67947b6 (merge-base) diff using: tpch File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing perf/parquet-skip-filter-for-full-match (a3047ed) to 67947b6 (merge-base) diff using: clickbench_partitioned File an issue against this benchmark runner |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves Parquet scan performance by avoiding per-row RowFilter evaluation for row groups proven (via statistics/static pruning) to be fully matched, and by enabling cheap per-row-group rebuilds of the row filter using prebuilt candidates. This builds on arrow-rs’s peek_next_row_group() to keep DataFusion’s row-group plan aligned with the decoder’s actual frontier.
Changes:
- Track per-row-group
fully_matchedstate in the Parquet access plan and preserve it through reorder/reverse operations. - Toggle the decoder’s
RowFilterat row-group boundaries (empty filter for fully-matched RGs; real filter otherwise) and add a new metric for observability. - Refactor row-filter construction to prebuild filter candidates once per file and rebuild cheaply per row group; update integration + SQL logic tests accordingly.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| datafusion/datasource-parquet/src/access_plan.rs | Carries per-RG fully_matched flags into the prepared plan and strips empty-selection RGs to keep plan/decoder alignment. |
| datafusion/datasource-parquet/src/metrics.rs | Adds row_filter_skipped_fully_matched metric to Parquet file metrics. |
| datafusion/datasource-parquet/src/push_decoder.rs | Adds per-RG plan entries and implements boundary-time RowFilter toggling using peek_next_row_group(). |
| datafusion/datasource-parquet/src/row_filter.rs | Splits row-filter building into “prebuild once per file” + “instantiate per RG” and expands nested/struct field planning logic. |
| datafusion/datasource-parquet/src/opener/mod.rs | Wires the prebuilt row-filter context into decoder construction and propagates new metric/state into the stream. |
| datafusion/datasource-parquet/src/sort.rs | Updates prepared-plan reverse test to reflect stripping of all-empty selections. |
| datafusion/core/tests/parquet/dynamic_row_group_pruning.rs | Adds an end-to-end integration test asserting fully-matched RGs trigger row-filter skipping behavior. |
| datafusion/sqllogictest/test_files/push_down_filter_parquet.slt | Updates EXPLAIN ANALYZE expectations to include the new metric. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+64
to
+68
| /// Number of row groups for which the per-row | ||
| /// [`RowFilter`](parquet::arrow::arrow_reader::RowFilter) was skipped | ||
| /// because the static stats proved every row of the RG satisfies the | ||
| /// predicate. The decoder is rebuilt at the boundary with an empty | ||
| /// row filter so the upcoming RG decodes without per-row evaluation. |
Contributor
Author
There was a problem hiding this comment.
Fixed in fea0264 — the doc now states explicitly that this counts suppression events, not row groups: a run of consecutive fully-matched RGs shares a single toggle.
| && (!DataType::is_nested(return_type) | ||
| || self.is_nested_type_supported(return_type)) | ||
| { | ||
| // try to resolve all field name arguments to strinrg literals |
Comment on lines
+192
to
+196
| /// The result of resolving which Parquet leaf columns and Arrow schema fields | ||
| /// are needed to evaluate an expression against a Parquet file | ||
| /// | ||
| /// This is the shared output of the column resolution pipeline used by both | ||
| /// the row filter to build `ArrowPredicate`s and the opener to build `ProjectionMask`s |
Contributor
Author
There was a problem hiding this comment.
Fixed in fea0264 — row_filter.rs now reuses the shared ParquetReadPlan / PushdownChecker machinery from projection_read_plan.rs instead of carrying its own copy (~700 lines removed).
| .map(|p| replace_columns_with_literals(p, &literal_columns)) | ||
| .transpose()?; | ||
| } | ||
|
|
Contributor
Author
There was a problem hiding this comment.
Good catch — this was real: the rebase had silently dropped the input_file_name() rewrite. Restored in fea0264 along with a regression test (test_input_file_name_projection).
Contributor
Author
|
run benchmarks env:
DATAFUSION_EXECUTION_PARQUET_PUSHDOWN_FILTERS: true |
|
Thank you for opening this pull request! Reviewer note: cargo-semver-checks reported the current version number is not SemVer-compatible with the changes in this pull request (compared against the base branch). Details |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing perf/parquet-skip-filter-for-full-match (a3047ed) to 67947b6 (merge-base) diff using: clickbench_partitioned File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing perf/parquet-skip-filter-for-full-match (a3047ed) to 67947b6 (merge-base) diff using: tpch File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing perf/parquet-skip-filter-for-full-match (a3047ed) to 67947b6 (merge-base) diff using: tpcds File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagetpch — base (merge-base)
tpch — branch
File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagetpcds — base (merge-base)
tpcds — branch
File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagetpch — base (merge-base)
tpch — branch
File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usageclickbench_partitioned — base (merge-base)
clickbench_partitioned — branch
File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagetpcds — base (merge-base)
tpcds — branch
File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usageclickbench_partitioned — base (merge-base)
clickbench_partitioned — branch
File an issue against this benchmark runner |
Contributor
Author
|
run benchmark sort_tpch env:
DATAFUSION_EXECUTION_PARQUET_PUSHDOWN_FILTERS: true |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing perf/parquet-skip-filter-for-full-match (a3047ed) to 67947b6 (merge-base) diff using: sort_tpch File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagesort_tpch — base (merge-base)
sort_tpch — branch
File an issue against this benchmark runner |
zhuqi-lucas
force-pushed
the
perf/parquet-skip-filter-for-full-match
branch
from
July 31, 2026 14:13
a3047ed to
fea0264
Compare
Builds on the `peek_next_row_group` API landed in apache/arrow-rs#10158. When Parquet stats prove that every row of a row group already satisfies the pushdown predicate (`fully_matched`), running the per-row `RowFilter` inside that row group is pure overhead — every row passes anyway. This change installs an *empty* `RowFilter` on fully-matched runs and only pays the row-level machinery on RGs that still need filtering. * `access_plan.rs` — `ParquetAccessPlan` now tracks per-RG `fully_matched` state, produced during static pruning. * `metrics.rs` — new `row_filter_skipped_fully_matched` counter to observe how often the toggle fires. * `push_decoder.rs` — `RgPlanEntry` carries the per-RG toggle; `RowFilterContext` swaps between the real filter (from the `prebuild_row_filter_candidates` cache) and an empty filter as the decoder crosses row-group boundaries. * `row_filter.rs` — split `build_row_filter` into `prebuild_*` and `row_filter_from_prebuilt`: the expensive tree walk + candidate construction runs once per file, and the cheap per-RG bind runs at each toggle. Preserves the existing public `build_row_filter` API for non-toggling callers. * `opener/mod.rs` — wires the prebuilt cache into the decoder builder; skips page-index loading for fully-matched RGs (their per-row filter is a no-op, so page pruning saves nothing). * `sort.rs` — sort-order-aware RG reorder preserves the per-RG toggle through reordering. * `fully_matched_rgs_skip_row_filter` (`dynamic_row_group_pruning.rs`) — 4 RGs of 3 rows each, predicate `v >= 3` makes RGs 1..=3 fully matched. Asserts: (a) results are correct, (b) `row_filter_skipped_fully_matched` fires at least once at the RG 0 → RG 1 boundary. * All 6 pre-existing `dynamic_row_group_pruning` tests still pass. * Full `datasource-parquet` lib suite (158 tests) passes.
…t_file_name handling lost in rebase Three fixes on top of the rebase onto latest main: 1. row_filter.rs carried a stale pre-apache#23638 copy of the column-resolution machinery (ParquetReadPlan, PushdownChecker, StructFieldAccess, build_filter_schema, etc.) that main has since moved into projection_read_plan.rs. Rebuilt the file on main's shared-module version, keeping only this PR's real additions (PrebuiltRowFilterCandidate, prebuild_row_filter_candidates, row_filter_from_prebuilt, and the RowFilterGenerator removal): 2194 → 1483 lines, -798 net. The stray "strinrg" typo lived in the deleted copy. 2. The rebase silently dropped main's input_file_name() handling in ParquetMorselizer (import, the per-file literal rewrite in open(), and the test_input_file_name_projection test) because this PR's restructure of open() overlapped the region apache#23638 later touched. Restored all three; the test passes again. 3. metrics: the row_filter_skipped_fully_matched doc claimed to count row groups, but the counter increments once per suppression event (a run of consecutive fully-matched RGs shares a single toggle). Doc now states the actual semantics. Verified: 158 datasource-parquet tests pass (with PARQUET_TEST_DATA), cargo doc clean, clippy clean on touched files.
…redAccessPlan::new, regenerate slt for new metric, fix redundant doc link
zhuqi-lucas
force-pushed
the
perf/parquet-skip-filter-for-full-match
branch
from
August 6, 2026 13:30
fea0264 to
2445739
Compare
Contributor
Author
|
run benchmark tpcds |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing perf/parquet-skip-filter-for-full-match (2445739) to 4fcbfb1 (merge-base) diff Run configurationrun benchmark tpcdsResults will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing perf/parquet-skip-filter-for-full-match (2445739) to 4fcbfb1 (merge-base) diff Run configurationrun benchmark tpcdsCPU Details (lscpu)Details
Resource Usagetpcds — base (merge-base)
tpcds — branch
File an issue against this benchmark runner |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #23696 +/- ##
========================================
Coverage 81.29% 81.30%
========================================
Files 1110 1110
Lines 385336 385564 +228
Branches 385336 385564 +228
========================================
+ Hits 313261 313468 +207
- Misses 53594 53601 +7
- Partials 18481 18495 +14 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
zhuqi-lucas
marked this pull request as ready for review
August 7, 2026 07:00
…atched=1 The existing slt coverage only ever showed row_filter_skipped_fully_matched=0 (none of those cases enable pushdown_filters, so no RowFilter exists to skip). Add a static-predicate case (v >= 4 with pushdown on) that drops RG0 by stats, keeps a RowFilter on the straddling RG1, and skips it across the fully-matched run RGs 2..=4 -- surfacing the optimization from SQL with =1.
adriangb
reviewed
Aug 11, 2026
Comment on lines
+597
to
+600
| /// Layout: 4 RGs of 3 values each. Predicate `v >= 3` makes RG 0 a | ||
| /// straddler (some rows fail) but RGs 1..=3 fully matched (every value | ||
| /// >= 3 by stats). RG 0 keeps the row filter, then the toggle flips to | ||
| /// "no filter" when we enter the fully-matched run. |
Contributor
There was a problem hiding this comment.
Can we test v >= 3 AND v <= 10 or make a new test for v <= 10? The point is to test going from fully-matched - > non-fully-matched
Contributor
Author
There was a problem hiding this comment.
Thanks @adriangb for the review, good suggestion! Updated the test to v >= 3 AND v <= 10 in 801b099 — RG 3 now straddles the upper bound (11, 12 fail), so it covers the full toggle lifecycle (filter ON → OFF across the fully-matched run → back ON) and asserts 11/12 are filtered out by the reinstalled RowFilter.
Change the predicate to `v >= 3 AND v <= 10` so RG 3 becomes a straddler again (11, 12 fail the upper bound), exercising the full toggle lifecycle (filter ON -> OFF across the fully-matched run -> back ON) and asserting 11/12 are filtered out by the reinstalled filter.
11 tasks
…_parquet.slt Merging main brought in EXPLAIN ANALYZE cases in push_down_filter_parquet.slt whose expected parquet-scan metrics predate this PR's new `row_filter_skipped_fully_matched` counter. Regenerate the expectations to include it (=0 in these cases; no fully-matched RG run).
alamb
reviewed
Aug 11, 2026
alamb
left a comment
alamb
left a comment
Contributor
There was a problem hiding this comment.
Thanks @zhuqi-lucas -- this looks neat -- I left some suggestions
| if at_boundary { | ||
| match decoder_ref.peek_next_row_group() { | ||
| Ok(Some(actual)) => { | ||
| while let Some(front) = self.rg_plan.front() { |
Contributor
There was a problem hiding this comment.
maybe this would be a good method on rg_plan -- like rg_plan.advance_to_index(actual)` or something 🤔
Contributor
Author
There was a problem hiding this comment.
Done — extracted into advance_rg_plan_to() (e0fbd24).
| let rebuilt = match decoder.into_builder() { | ||
| Ok(b) => b.with_row_groups(new_indices).build(), | ||
| Ok(mut builder) => { | ||
| builder = builder.with_row_groups(new_indices); |
Contributor
There was a problem hiding this comment.
same thing here -- can we somehow encapsulate this logic into a method or function to keep the lengh of the state machine down?
Contributor
Author
There was a problem hiding this comment.
Pulled the advance/prune helpers out (e0fbd24).
Contributor
Author
There was a problem hiding this comment.
Went ahead and extracted it too in 19572d4 — the rebuild+toggle block is now rebuild_decoder_at_boundary, returning Ok(true) to finish the stream, Ok(false) to continue, or Err to propagate, so the Step 2 loop no longer has any inline early-returns.
| .expect("decoder present") | ||
| .is_at_row_group_boundary(); | ||
| let decoder_ref = self.decoder.as_ref().expect("decoder present"); | ||
| let at_boundary = decoder_ref.is_at_row_group_boundary(); |
Contributor
There was a problem hiding this comment.
Can this be handled at a higher layer or encapsulated somehow? Like in a method? The state machine is already complicated enough and so moving this plan manipulation into methods will help hopefully
Contributor
Author
There was a problem hiding this comment.
Done — moved into sync_rg_plan_to_decoder_frontier + prune_boundary_row_groups (e0fbd24).
| // Sync `rg_plan` with the row group the decoder will actually | ||
| // emit next. arrow-rs's `try_next_reader` silently advances | ||
| // past row groups whose row selection is empty (e.g. when | ||
| // page-index pruning has already eliminated every page of |
Contributor
There was a problem hiding this comment.
I think this comment could be shortened significantly -- the point is that try_next_reader can silently advance, so we have to advance the plan accordingly. The details about ColumnIndex and rg_pla.front() seems overly focused on the details
| /// Prebuilt candidates: expression already column-reassigned, projection | ||
| /// mask already resolved. Shared across the file's row groups. `Arc` so | ||
| /// cloning into stream state is cheap. | ||
| pub(crate) prebuilt: Arc<Vec<PrebuiltRowFilterCandidate>>, |
Contributor
There was a problem hiding this comment.
Another thought I had was make this a strucgture of
pub(crate) prebuilt: PrebuiltRowFilterCandidateList,with
struct PrebuiltRowFilterCandidateList {
inner : Arc<Vec<PrebuiltRowFilterCandidate>>,
}Again as a way to try and encapsualte the complexity of this optimization more
Contributor
Author
There was a problem hiding this comment.
Done — added PrebuiltRowFilterCandidateList (e0fbd24).
| pub(crate) row_filter_skipped_fully_matched: Count, | ||
| } | ||
|
|
||
| /// Side-channel state that lets [`PushDecoderStreamState`] **rebuild** the |
Contributor
There was a problem hiding this comment.
why is it a "side channel"? It seems like it is more like a "cache"
| /// Side-channel state that lets [`PushDecoderStreamState`] **rebuild** the | ||
| /// parquet [`RowFilter`] mid-scan. | ||
| /// | ||
| /// The decoder owns the filter once installed, but `Box<dyn ArrowPredicate>` |
Contributor
There was a problem hiding this comment.
"The decoder owns the filter once installed, but Box<dyn ArrowPredicate>
/// has no clone path, so a filter that was replaced at a previous boundary
/// cannot be reinstalled later."
seems like an implementation detail -- keeping the pre-built filters seems like the key point
| #[derive(Debug, Clone)] | ||
| pub(crate) struct RgPlanEntry { | ||
| pub(crate) rg_index: usize, | ||
| /// `true` when the static pruning predicate proved every row of this |
Contributor
There was a problem hiding this comment.
I think the first sentence here would be eough -- the rest is details of the implementation that I don't think adds mich here. The point is that we can skip row filtering when we know it won't filter anything
…im comments Per @alamb's review of the fully-matched RowFilter-skip push decoder: - wrap the prebuilt candidate Arc<Vec> in a PrebuiltRowFilterCandidateList newtype - reframe the RowFilterContext doc as a cache (not a side channel) - extract the row-group-boundary plan manipulation into sync_rg_plan_to_decoder_frontier / advance_rg_plan_to / prune_boundary_row_groups to shorten the state machine loop - trim over-detailed comments to their essential point
Contributor
Author
|
Thanks @alamb for the review and the good suggestions! Addressed all comments now. |
…ndary Follow-up to review: pull the decoder rebuild + per-RG RowFilter toggle out of the Step 2 loop into rebuild_decoder_at_boundary, which returns Ok(true) when the plan is now empty (stream finishes), Ok(false) to continue, or Err to propagate a build failure — replacing the two inline early-returns.
adriangb
reviewed
Aug 12, 2026
adriangb
left a comment
adriangb
left a comment
Contributor
There was a problem hiding this comment.
Looks very cool overall!
Comment on lines
-1438
to
+1437
| let (decoder, rg_plan) = { | ||
| let (decoder, rg_plan, filter_installed, row_filter_context) = { |
Contributor
There was a problem hiding this comment.
This seems like a good candidate to factor out into a function returning a named struct w/ fields
Contributor
Author
There was a problem hiding this 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.
Comment on lines
+594
to
+609
| /// 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. |
Contributor
There was a problem hiding this comment.
This seems like a good general improvement that already applies to main, should we factor it out as an independent PR if so?
Contributor
Author
There was a problem hiding this 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.
Comment on lines
+1513
to
+1514
| builder = builder.with_row_filter( | ||
| parquet::arrow::arrow_reader::RowFilter::new(vec![]), |
Contributor
There was a problem hiding this comment.
should this branch set filter_installed = true or prepared.file_metrics.row_filter_skipped_fully_matched.add(1)?
Contributor
Author
There was a problem hiding this comment.
Good catch — fixed in b31b0b4: the first-RG-fully-matched path now calls row_filter_skipped_fully_matched.add(1), matching the mid-scan toggle. filter_installed stays false (no real filter is installed).
| } | ||
|
|
||
| /// Pop `rg_plan` entries until its front is `target` (or it empties). | ||
| fn advance_rg_plan_to(&mut self, target: usize) { |
Contributor
There was a problem hiding this comment.
If target isn't present in rg_plan (an invariant violation, the decoder's frontier names an RG our plan doesn't know), this loop silently drains. Can we add a check that target is found and if it's not throw an internal_err!() instead of possibly returning bogus results?
Contributor
Author
There was a problem hiding this comment.
Agreed — fixed in b31b0b4. advance_rg_plan_to now returns an internal error if target is not in rg_plan (decoder/plan divergence) instead of silently draining the plan.
Comment on lines
+3513
to
+3514
| #[tokio::test] | ||
| async fn test_input_file_name_projection() { |
Contributor
There was a problem hiding this comment.
This test seems to have moved with no changes - location diff noise?
Contributor
Author
There was a problem hiding this comment.
Just a location move from the rebase (the input_file_name handling was restored during a rebase, shifting this helper) — no functional change. Will rebase to clear the diff noise before merge.
…r divergence Per @adriangb's review: - opener: when the first RG is already fully-matched, count the row_filter_skipped_fully_matched suppression at open time (previously only counted for mid-scan toggles), keeping the metric consistent. - push_decoder: advance_rg_plan_to now returns an internal error if the decoder frontier names an RG absent from rg_plan, instead of silently draining the plan (which would truncate the scan).
Contributor
Author
|
Thanks @adriangb for the review and the good suggestions! Addressed in latest PR. |
Contributor
|
Looks good to me! I'd like to do some more benchmark runs, I do see regressions in the previous runs. I'd also like to know if there are existing benchmarks we expect to improve mechanistically and check that they do indeed improve; otherwise we should add benchmarks first. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
fully_matchedRowFilter skip · needs arrow-rs#10158 (peek_next_row_group) #23067.Rationale for this change
Builds on the
peek_next_row_groupAPI landed in apache/arrow-rs#10158 (in releasedarrow-rs 59.1.0). When Parquet stats prove that every row of a row group already satisfies the pushdown predicate (fully_matched), running the per-rowRowFilterinside that row group is pure overhead — every row passes anyway. This change installs an emptyRowFilteron fully-matched runs and only pays the row-level machinery on RGs that still need filtering.Two-phase reads for those RGs also skip fetching the filter columns when they aren't in the projection — this pattern is common on time-partitioned tables (
WHERE ts >= X AND ts < Ywhere the RG's min/max is contained in[X, Y)) and on the middle stretch of aTopKscan where the dynamic threshold is still loose.What changes are included in this PR?
access_plan.rs—ParquetAccessPlannow tracks per-RGfully_matchedstate, produced during static pruning.metrics.rs— newrow_filter_skipped_fully_matchedcounter to observe how often the toggle fires.push_decoder.rs—RgPlanEntrycarries the per-RG toggle;RowFilterContextswaps between the real filter (from theprebuild_row_filter_candidatescache) and an empty filter as the decoder crosses row-group boundaries viapeek_next_row_group().row_filter.rs— splitbuild_row_filterintoprebuild_row_filter_candidates(once per file: tree walk + candidate construction) androw_filter_from_prebuilt(per RG boundary: cheap bind against the fresh array readers). The existing publicbuild_row_filterAPI is preserved for callers that don't need per-RG toggling.opener/mod.rs— wires the prebuilt cache into the decoder builder; skips page-index loading for fully-matched RGs (their per-row filter is a no-op, so page pruning saves nothing).sort.rs— sort-order-aware RG reorder preserves the per-RG toggle through reordering.Are these changes tested?
Yes.
fully_matched_rgs_skip_row_filter(datafusion/core/tests/parquet/dynamic_row_group_pruning.rs) — 4 RGs of 3 rows each, predicatev >= 3makes RG 0 a straddler and RGs 1..=3 fully matched. Asserts (a) results are correct and (b)row_filter_skipped_fully_matchedfires at least once at the RG 0 → RG 1 boundary.dynamic_row_group_pruning.slt— anEXPLAIN ANALYZEcase (WHERE v >= 4withpushdown_filters=true) that surfacesrow_filter_skipped_fully_matched=1in the scan metrics, so the optimization is observable from SQL and any regression shows up as the count dropping back to 0.dynamic_row_group_pruningintegration tests still pass.datafusion-datasource-parquetlib suite passes.Are there any user-facing changes?
row_filter_skipped_fully_matchedmetric appears inEXPLAIN ANALYZEon parquet scans (0 when the toggle didn't fire).build_row_filterstill exists and behaves identically for non-toggling callers.