Make PushDownFilter and CommonSubexprEliminate aware of Expr::placement by adriangb · Pull Request #20239 · apache/datafusion

adriangb · 2026-02-09T13:21:40Z

Teaches PushDownFilter to not push down through ExpressionPlacement::MoveTowardsLeafNodes using the same approach already in place for volatile expressions.

Split out from #20117.

Copilot

Pull request overview

Updates PushDownFilter’s projection-rewrite behavior to avoid re-inlining certain “move towards leaf nodes” expressions (e.g., get_field) back into filter predicates, preventing optimizer instability with the ExtractLeafExpressions pipeline mentioned in the linked work.

Changes:

Classifies non-volatile projection expressions and prevents pushing filters through MoveTowardsLeafNodes projection outputs.
Rewrites pushed predicates using a reduced replacement map that excludes MoveTowardsLeafNodes expressions.
Adds unit tests covering: non-push through MoveTowardsLeafNodes, push through regular projections, push through KeepInPlace, and mixed-predicate partial pushdown.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

alamb

Thanks @adriangb -- I think this looks good to me. I had some suggestions, but none are required to merge this PR in my opinion

alamb · 2026-02-09T15:46:40Z

+
+ /// Identical MoveTowardsLeafNodes expressions should NOT be deduplicated
+ /// by CSE — they are cheap (e.g. struct field access) and the extraction
+ /// rules deliberately duplicate them. Deduplicating causes optimizer


What does "deduplicating causes optimizer instability" mean? Maybe we could make that more specific

alamb · 2026-02-09T15:49:38Z

+ // MoveTowardsLeafNodes expressions (like get_field) are cheap — no benefit to
+ // re-inlining them into filters, and it causes optimizer instability with
+ // ExtractLeafExpressions.
+ let (move_towards_leaves_map, pushable_map): (HashMap<_, _>, HashMap<_, _>) =


Is there any reason to have two separate hash maps? It seems like a single one would work (and maybe rename from volatile_map to no_move_map or something ?)

alamb · 2026-02-09T15:51:03Z

+
+ /// Test that filters are NOT pushed through MoveTowardsLeafNodes projections.
+ /// These are cheap expressions (like get_field) where re-inlining into a filter
+ /// has no benefit and causes optimizer instability with ExtractLeafExpressions.


same question here about what instablity is caused

alamb · 2026-02-09T15:52:09Z

 }
+
+ /// A mock UDF that reports MoveTowardsLeafNodes placement (like get_field).
+ #[derive(Debug, PartialEq, Eq, Hash)]


This is basically the same as in datafusion/optimizer/src/common_subexpr_eliminate.rs
I wonder if it would be better to put them in a common place (both to avoid code duplication, and make new tests easier to write)?

alamb · 2026-02-09T15:54:04Z

+ let udf = ScalarUDF::new_from_impl(LeafUDF {
+ signature: Signature::exact(vec![DataType::UInt32], Volatility::Immutable),
+ });
+ Expr::ScalarFunction(ScalarFunction::new_udf(Arc::new(udf), vec![arg]))


I think you could do this via call 🤔
Suggested change
Expr::ScalarFunction(ScalarFunction::new_udf(Arc::new(udf),vec![arg]))
udf.call(vec![arg])

alamb · 2026-02-09T15:55:35Z

+ )
+ }
+
+ /// Test that filters ARE pushed through regular (Column/KeepInPlace) projections.


this is likely already tested by other tests
One way you maybe could make it clear this is the same as above, only difference is the expression, might be with a builder type API
Something like
let udf_expr = leaf_udf().with_placement(ExpressionPlacement::KeepInPlace)// <----- Builder API to manually control udf).call(vec![col("a")]).alias("val"),

- Merge volatile_map + move_towards_leaves_map into single non_pushable_map in rewrite_projection - Deduplicate LeafUDF into shared PlacementTestUDF with builder API in test/udfs.rs, using udf.call() instead of manual Expr construction - Remove redundant tests (filter_pushed_through_regular_projection, filter_pushed_through_keep_in_place_projection) - Clarify "optimizer instability" comments to explain the infinite loop mechanism consistently across CSE and push_down_filter Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

…ld_push_towards_leaves() = true

…r instability CSE was deduplicating cheap get_field() expressions, creating intermediate projections that fought with ExtractLeafExpressions / PushDownLeafProjections. The extraction rules deliberately duplicate get_field (one copy for a filter predicate, another for an output column). CSE deduplicating them caused alias counter inflation (__datafusion_extracted_5 instead of _1) and unnecessary optimizer iterations. The fix teaches CSE to skip MoveTowardsLeafNodes expressions (e.g. get_field) since they are cheap struct field accesses with no benefit from deduplication. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

- Merge volatile_map + move_towards_leaves_map into single non_pushable_map in rewrite_projection - Deduplicate LeafUDF into shared PlacementTestUDF with builder API in test/udfs.rs, using udf.call() instead of manual Expr construction - Remove redundant tests (filter_pushed_through_regular_projection, filter_pushed_through_keep_in_place_projection) - Clarify "optimizer instability" comments to explain the infinite loop mechanism consistently across CSE and push_down_filter Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Followup to #20238 bringing in the shared test UDFs from #20239

…nt (apache#20239) Teaches PushDownFilter to not push down through `ExpressionPlacement::MoveTowardsLeafNodes` using the same approach already in place for volatile expressions. Split out from apache#20117. --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>

Followup to apache#20238 bringing in the shared test UDFs from apache#20239

…re of Expr::placement

github-actionsBot added the optimizer Optimizer rules label Feb 9, 2026

adriangb requested a review from Copilot February 9, 2026 13:22

Copilotstarted reviewing on behalf of adriangb February 9, 2026 13:22 View session

CopilotAI reviewed Feb 9, 2026
View reviewed changes

Comment threaddatafusion/optimizer/src/push_down_filter.rs Outdated

adriangb changed the title ~~Make PushDownFilter skip pushing down through Expr::placement()::should_push_towards_leaves() = true~~Make PushDownFilter skip pushing down through MoveTowardsLeafNodes projectionsFeb 9, 2026

adriangb changed the title ~~Make PushDownFilter skip pushing down through MoveTowardsLeafNodes projections~~Make PushDownFilter aware of Expr::placementFeb 9, 2026

adriangb added a commit to pydantic/datafusion that referenced this pull request Feb 9, 2026
Pull in PushDownFilter approach fromapache#20239
aa29dd4

adriangb requested a review from alamb February 9, 2026 14:16

adriangb changed the title ~~Make PushDownFilter aware of Expr::placement~~Make PushDownFilter and CommonSubexprEliminate aware of Expr::placementFeb 9, 2026

adriangb force-pushed the add-utils branch from e94bd66 to 20a915cCompare February 9, 2026 14:53

github-actionsBot added the sqllogictest SQL Logic Tests (.slt) label Feb 9, 2026

alamb approved these changes Feb 9, 2026
View reviewed changes

adriangband others added 4 commits February 9, 2026 13:15

Make PushDownFilter skip pushing down through Expr::placement()::shou…
4677c6e
…ld_push_towards_leaves() = true

update slts
8cd5055

adriangb force-pushed the add-utils branch from d9b16b7 to 8cd5055Compare February 9, 2026 18:17

adriangband others added 2 commits February 9, 2026 13:19

remove hardcoded name
7c2a31a

fix: add Default impl and remove clone on Copy type in PlacementTestUDF
dfc66d9
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

adriangb added this pull request to the merge queue Feb 9, 2026

Merged via the queue into apache:main with commit 9333f74Feb 9, 2026
32 checks passed

adriangb mentioned this pull request Feb 10, 2026
Clean up expression placement UDF usage in tests #20272
Merged

github-merge-queueBot pushed a commit that referenced this pull request Feb 11, 2026
Clean up expression placement UDF usage in tests (#20272)
f9d37cf
Followup to #20238 bringing in the shared test UDFs from #20239

de-bgunter pushed a commit to de-bgunter/datafusion that referenced this pull request Mar 24, 2026
Clean up expression placement UDF usage in tests (apache#20272)
6bd9fd5
Followup to apache#20238 bringing in the shared test UDFs from apache#20239

Druva-D pushed a commit to Druva-D/datafusion that referenced this pull request May 4, 2026
Portapache#20239: Make PushDownFilter and CommonSubexprEliminate awa…
3a944e0
…re of Expr::placement

Navigation Menu

Uh oh!

Make PushDownFilter and CommonSubexprEliminate aware of Expr::placement - #20239

Make PushDownFilter and CommonSubexprEliminate aware of Expr::placement#20239
adriangb merged 6 commits into
apache:mainfrom
pydantic:add-utils

adriangb commented Feb 9, 2026•
edited
Loading

Uh oh!

CopilotAI left a comment

Uh oh!

Uh oh!

alamb left a comment

Uh oh!

alambFeb 9, 2026

Uh oh!

alambFeb 9, 2026

Uh oh!

alambFeb 9, 2026

Uh oh!

alambFeb 9, 2026

Uh oh!

alambFeb 9, 2026

Uh oh!

alambFeb 9, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	Expr::ScalarFunction(ScalarFunction::new_udf(Arc::new(udf),vec![arg]))
	udf.call(vec![arg])

Uh oh!

Conversation

adriangb commented Feb 9, 2026• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CopilotAI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

alambFeb 9, 2026

Choose a reason for hiding this comment

Uh oh!

alambFeb 9, 2026

Choose a reason for hiding this comment

Uh oh!

alambFeb 9, 2026

Choose a reason for hiding this comment

Uh oh!

alambFeb 9, 2026

Choose a reason for hiding this comment

Uh oh!

alambFeb 9, 2026

Choose a reason for hiding this comment

Uh oh!

alambFeb 9, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

adriangb commented Feb 9, 2026•
edited
Loading