Repository navigation
CommonSubexprEliminate + leaf-expression pushdown duplicate a UDF call wrapped in get_field #25329
Description
Describe the bug
When a non-pushable (KeepInPlace) function call appears once bare and once wrapped in get_field(...),
CommonSubexprEliminate correctly deduplicates the bare occurrence into a __common_expr_N column — but leaf-expression pushdown (ExtractLeafExpressions / PushDownLeafProjections) then re-derives the full original call a second time when it tries to push the get_field expression down, because __common_expr_N's Column reference is indistinguishable, at the ExpressionPlacement level, from a genuine cheap base-table column.
Net effect: the function is evaluated twice instead of once.
To Reproduce
Using here the arrow_field UDF as an example since this function returns a struct type.
CREATE TABLE t (a INT) AS VALUES (1), (2);
EXPLAIN SELECT
CASE WHEN arrow_field(a) IS NOT NULL
THEN get_field(arrow_field(a), 'name') END AS c1
FROM t; Actual physical plan
+---------------+-------------------------------+
| plan_type | plan |
+---------------+-------------------------------+
| physical_plan | ┌───────────────────────────┐ |
| | │ ProjectionExec │ |
| | │ -------------------- │ |
| | │ c1: │ |
| | │ CASE WHEN arrow_field(a) │ |
| | │ IS NOT NULL THEN │ |
| | │ get_field │ |
| | │ (arrow_field(a), │ |
| | │ name) END │ |
| | └─────────────┬─────────────┘ |
| | ┌─────────────┴─────────────┐ |
| | │ DataSourceExec │ |
| | │ -------------------- │ |
| | │ bytes: 112 │ |
| | │ format: memory │ |
| | │ rows: 1 │ |
| | └───────────────────────────┘ |
| | |
+---------------+-------------------------------+
The function (arrow_field) is called twice.
Expected behavior
Expected physical plan
+---------------+-------------------------------+
| plan_type | plan |
+---------------+-------------------------------+
| physical_plan | ┌───────────────────────────┐ |
| | │ ProjectionExec │ |
| | │ -------------------- │ |
| | │ c1: │ |
| | │ CASE WHEN __common_expr_1 │ |
| | │ IS NOT NULL THEN │ |
| | │ get_field │ |
| | │ (__common_expr_1, │ |
| | │ name) END │ |
| | └─────────────┬─────────────┘ |
| | ┌─────────────┴─────────────┐ |
| | │ ProjectionExec │ |
| | │ -------------------- │ |
| | │ __common_expr_1: │ |
| | │ arrow_field(a) │ |
| | └─────────────┬─────────────┘ |
| | ┌─────────────┴─────────────┐ |
| | │ DataSourceExec │ |
| | │ -------------------- │ |
| | │ bytes: 112 │ |
| | │ format: memory │ |
| | │ rows: 1 │ |
| | └───────────────────────────┘ |
| | |
+---------------+-------------------------------+
The function (arrow_field) is called once;
get_field references the shared __common_expr_1 column.
Additional context
No response
Activity
bert-beyondloops commentedon Sep 15, 2026
take
liningpan commentedon Sep 15, 2026
Duplicate with #23655. There is a PR already #23691
adriangb commentedon Sep 16, 2026
Good timing!
@bert-beyondloops does #23691 solve your issue? If there anything in #25330 that you think is still valuable (tests, slight tweak on implementation) I’d be happy to review it after a rebase.
bert-beyondloops commentedon Sep 16, 2026
Hi @adriangb
Thanks for spotting the duplicate issue.
I've marked #25330 as draft for the moment.
I'll retest with your landed changes first.
bert-beyondloops commentedon Sep 16, 2026
Repro: a struct-returning function referenced both as a bare value (
arrow_field(a) IS NOT NULL) and viaget_field/[...]inside a Filter predicate is evaluated twice,even on the datafusion commit incorporating the fix: "evaluate struct-returning UDFs once across repeated field accesses", #23691.
PR #23691 / issue #23655 only regression-tests this pattern inside a
Projection; the same pattern inside a Filter predicate is not covered.
Steps to reproduce :
CREATE TABLE arrow_field_filter_cse_test(a INT) AS VALUES (1), (2);
EXPLAIN FORMAT TREE SELECT a FROM arrow_field_filter_cse_test
WHERE CASE WHEN arrow_field(a) IS NOT NULL THEN arrow_field(a)['nullable'] IS NULL END;
The __common_expr_7 expression is only referenced once in the actual filter.
pepijnve commentedon Sep 16, 2026
Here's an excerpt of the optimisation output from current
main(606ae0f).Here too it's
extract_leaf_expressionsthat's causing the function call duplication.adriangb commentedon Sep 16, 2026
@bert-beyondloops would you like to open a PR to add the missing filter coverage / @pepijnve's example above? It'd be great to think about how we can close gaps generically so we're not patching individual cases.
pepijnve commentedon Sep 16, 2026
With a minor change to
extract_leaf_expressionsI was able to get towhich is what we're going for I think.
a little bit further down the line
push_down_filtergoes ahead and ruins things again.Does that seem like something we should prevent as well? Moving a
NOT NULLfilter before a projection that computes the same value doesn't seem sensible.pepijnve commentedon Sep 16, 2026
I think I've been able to come up with some minimal tweaks that resolve the remaining issues described above. I'll try to get a pr ready later today.
fix(optimizer): do not push a filter that duplicates an expensive pro…
fix(optimizer): do not re-inline a shared definition in leaf extraction
test: cover a struct-returning function evaluated one time in `cse.slt`
bert-beyondloops commentedon Oct 5, 2026
Issue:
ExpressionPlacement::KeepInPlaceisn't enforced, and its current default makes it unusable for that purposeKeepInPlace'sdoc says it marks "an expensive expression that should stay where it is... complex scalar functions or UDFs." Butpush_down_filter.rs::rewrite_projection's guard only checksis_volatile()andplacement() == MoveTowardsLeafNodes— neverKeepInPlace— so a filter referencing a projected column can still be pushed/substituted through that projection, duplicating an expensive expression multiple times.Naively adding a KeepInPlace check doesn't work either:
Expr::placement()'s catch-all default isKeepInPlacefor anything that isn't a bare Column/Literal/Alias, andScalarUDFImpl::placement()'s default is alsoKeepInPlacefor any function that doesn't override it. So ordinary
a + 1, CASE, CAST,and every unclassified function call already resolve toKeepInPlacetoday. Gating pushdown on it would block common, cheap, currently-working optimizations (e.g. pushing a filter on a + 1 AS b down to the scan), not just genuinely expensive expressions.Core problem: the default collides "deliberately expensive, don't duplicate" with "nobody classified this" — both produce the same enum value, so no optimizer can safely treat
KeepInPlaceas a duplication guard.Questions:
KeepInPlacemeant to be actively enforced somewhere, or is the doc comment aspirational with no current consumer?Expr::placement()'s catch-all andScalarUDFImpl::placement()'s default) return a distinct variant instead, reservingKeepInPlacefor explicit opt-in — so pushdown rules can gate on it safely?Is this design issue being tackled already in many of those outstanding issues mentioned in #25459 ?
Honestly, a colleague of mine (@pepijnve) and I have lost track of the overall direction across these issues — where the design is heading and how the various fixes are meant to fit together.
@adriangb We definitely want to help out some way or another.
fix: resolve pass-through columns against the input when merging an e…
adriangb commentedon Oct 28, 2026
I'm sorry I've let this get to this state. I need to invest time that I simply don't have in refreshing my mind on these implementations and tying together all of the loose ends. I'll get to it in the next couple of weeks.
Maybe it would help if I wrote a high level description of what the problem we are trying to solve is in my mind? Or maybe we can add all of the different cases we think are handled wrong as tests and work from there TDD style?
fix(optimizer): do not push a filter that duplicates an expensive pro…
fix(optimizer): do not re-inline a shared definition in leaf extraction
test: cover a struct-returning function evaluated one time in `cse.slt`
fix(optimizer): do not push a filter that duplicates an expensive pro…
fix(optimizer): do not re-inline a shared definition in leaf extraction
test: cover a struct-returning function evaluated one time in `cse.slt`
test: count evaluations of volatile calls across optimizer rules