Skip to content

fix: preserve aggregate filter pushdown order - #22926

Merged
neilconway merged 6 commits into
apache:mainfrom
discord9:fix/aggregate-filter-pushdown-order
Jul 29, 2026
Merged

fix: preserve aggregate filter pushdown order#22926
neilconway merged 6 commits into
apache:mainfrom
discord9:fix/aggregate-filter-pushdown-order

Conversation

@discord9

@discord9discord9 commented Jun 12, 2026

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Rationale for this change

FilterPushdown maps each child pushdown result back to its original parent
filter by position. AggregateExec::gather_filters_for_pushdown previously
split parent filters into safe and unsafe buckets and then concatenated the
results, which changed their order.

For example, in cnt@2 = 1 AND b@1 = bar, the grouping-column predicate on
b can cross the aggregate, while the predicate on aggregate output cnt
must remain above it. Reordering the returned results could make the optimizer
associate the pushed-down b result with the cnt predicate and incorrectly
remove the latter.

Aggregate pushdown also needs to account for empty-input semantics. A global
aggregate, or a grouping-sets aggregate containing (), can emit a row even
when its input is empty. Moving any parent predicate below such an aggregate —
including a column-free predicate such as false — can therefore change the
result.

What changes are included in this PR?

  • Preserve parent-filter order by constructing each child description once
    with ChildFilterDescription::from_child_with_allowed_indices.
  • Allow only grouping-output columns that are present in every grouping set;
    aggregate-result columns remain above the aggregate.
  • Mark all parent filters unsupported for global aggregates and grouping sets
    containing an empty grouping set, while retaining aggregate-generated
    dynamic filters.
  • Validate that every child returns one parent-filter result per input filter
    before positional remapping.
  • Add physical optimizer and SQL regression coverage for mixed filter order,
    global-aggregate name collisions, constant predicates, and grouping sets.

Are these changes tested?

Yes:

  • cargo test -p datafusion --test core_integration physical_optimizer::filter_pushdown
  • cargo fmt --all -- --check
  • cargo check -p datafusion-physical-plan -p datafusion-physical-optimizer
  • git diff --check upstream/main
  • Full GitHub CI, including Rust tests, clippy, and sqllogictests

Are there any user-facing changes?

There are no public API changes. Filter pushdown now preserves mixed aggregate
predicates correctly and avoids moving predicates across aggregates when doing
so could change empty-input results.

@github-actionsgithub-actionsBot added core Core DataFusion crate physical-plan Changes to the physical-plan crate sqllogictest SQL Logic Tests (.slt) auto detected api change Auto detected API change labels Jun 12, 2026
@discord9

Copy link
Copy Markdown
ContributorAuthor

Hi, this PR fixes #22925 and is still waiting for review.

The only failing check is Circular Dependency Check, which appears to fail while compiling cargo-credential rather than from this PR's code. Could someone please rerun it or advise if I should do anything here?

Thanks!

@discord9
discord9force-pushed the fix/aggregate-filter-pushdown-order branch from 05e5a77 to 3626a96CompareJuly 22, 2026 07:28
@github-actionsgithub-actionsBot removed the auto detected api change Auto detected API change label Jul 22, 2026
@codecov-commenter

codecov-commenter commented Jul 22, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 90.47619% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.75%. Comparing base (ef94a85) to head (4e9f319).
⚠️ Report is 9 commits behind head on main.

Files with missing linesPatch %Lines
...tafusion/physical-optimizer/src/filter_pushdown.rs75.00%1 Missing ⚠️
datafusion/physical-plan/src/aggregates/mod.rs94.11%1 Missing ⚠️
Additional details and impacted files
@@ Coverage Diff @@## main #22926 +/- ##
==========================================
+ Coverage 80.70% 80.75% +0.04% 
==========================================
Files 1095 1096 +1 Lines 372554 373588 +1034 Branches 372554 373588 +1034 ==========================================
+ Hits 300665 301677 +1012 + Misses 53921 53906 -15 - Partials 17968 18005 +37 

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@neilconwayneilconway left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this PR! Overall, this is excellent work: clear diagnosis, minimal and correct fix, and good test coverage. I'm sorry it didn't get reviewed sooner -- a few minor suggestions but it should be good to land shortly.

Comment threaddatafusion/physical-plan/src/aggregates/mod.rs Outdated
Comment threaddatafusion/physical-plan/src/aggregates/mod.rs Outdated
@discord9
discord9force-pushed the fix/aggregate-filter-pushdown-order branch from 3626a96 to 4b0e6c6CompareJuly 29, 2026 03:54
@github-actionsgithub-actionsBot added the optimizer Optimizer rules label Jul 29, 2026

@neilconwayneilconway left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good! Can you update the PR description, please? Currently refers to the old implementation; would also be good to elaborate on the pushdown change for global aggregates?

Comment threaddatafusion/core/tests/physical_optimizer/filter_pushdown.rs
@discord9

Copy link
Copy Markdown
ContributorAuthor

Updated the PR description to reflect the current from_child_with_allowed_indices implementation, the descriptor-length invariant, and why global aggregates / grouping sets containing () must retain parent filters above the aggregate. Thanks for the review.

@neilconway
neilconway enabled auto-merge July 29, 2026 18:41
@neilconway
neilconway added this pull request to the merge queueJul 29, 2026
Merged via the queue into apache:main with commit 2f25454Jul 29, 2026
40 checks passed
kosiew pushed a commit to kosiew/datafusion that referenced this pull request Aug 12, 2026
## Which issue does this PR close?
- Closesapache#22925.
## Rationale for this change
`FilterPushdown` maps each child pushdown result back to its original
parent
filter by position. `AggregateExec::gather_filters_for_pushdown`
previously
split parent filters into safe and unsafe buckets and then concatenated
the
results, which changed their order.
For example, in `cnt@2 = 1 AND b@1 = bar`, the grouping-column predicate
on
`b` can cross the aggregate, while the predicate on aggregate output
`cnt`
must remain above it. Reordering the returned results could make the
optimizer
associate the pushed-down `b` result with the `cnt` predicate and
incorrectly
remove the latter.
Aggregate pushdown also needs to account for empty-input semantics. A
global
aggregate, or a grouping-sets aggregate containing `()`, can emit a row
even
when its input is empty. Moving any parent predicate below such an
aggregate —
including a column-free predicate such as `false` — can therefore change
the
result.
## What changes are included in this PR?
- Preserve parent-filter order by constructing each child description
once
with `ChildFilterDescription::from_child_with_allowed_indices`.
- Allow only grouping-output columns that are present in every grouping
set;
aggregate-result columns remain above the aggregate.
- Mark all parent filters unsupported for global aggregates and grouping
sets
containing an empty grouping set, while retaining aggregate-generated
dynamic filters.
- Validate that every child returns one parent-filter result per input
filter
before positional remapping.
- Add physical optimizer and SQL regression coverage for mixed filter
order,
global-aggregate name collisions, constant predicates, and grouping
sets.
## Are these changes tested?
Yes:
- `cargo test -p datafusion --test core_integration
physical_optimizer::filter_pushdown`
- `cargo fmt --all -- --check`
- `cargo check -p datafusion-physical-plan -p
datafusion-physical-optimizer`
- `git diff --check upstream/main`
- Full GitHub CI, including Rust tests, clippy, and sqllogictests
## Are there any user-facing changes?
There are no public API changes. Filter pushdown now preserves mixed
aggregate
predicates correctly and avoids moving predicates across aggregates when
doing
so could change empty-input results.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

coreCore DataFusion crateoptimizerOptimizer rulesphysical-planChanges to the physical-plan cratesqllogictestSQL Logic Tests (.slt)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

AggregateExec filter pushdown can reorder parent filter results

3 participants

@discord9@codecov-commenter@neilconway