bert-beyondloops commented on issue #25329:
URL: https://github.com/apache/datafusion/issues/25329#issuecomment-5994053810
**Issue**: `ExpressionPlacement::KeepInPlace` isn't enforced, and its
current default makes it unusable for that purpose
`KeepInPlace's` doc says it marks "an expensive expression that should stay
where it is... complex scalar functions or UDFs." But
`push_down_filter.rs::rewrite_projection`'s guard only checks `is_volatile()`
and `placement() == MoveTowardsLeafNodes` — never `KeepInPlace` — 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 is `KeepInPlace` for anything that
isn't a bare Column/Literal/Alias, and `ScalarUDFImpl::placement()`'s default
is also `KeepInPlace`
for any function that doesn't override it. So ordinary `a + 1, CASE,
CAST,` and every unclassified function call already resolve to `KeepInPlace`
today. 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 KeepInPlace as a duplication guard.
**Questions**:
1. Was `KeepInPlace` meant to be actively enforced somewhere, or is the
doc comment aspirational with no current consumer?
2. Should the blanket defaults (`Expr::placement()`'s catch-all and
`ScalarUDFImpl::placement()'`s default) return a distinct variant instead,
reserving `KeepInPlace` for 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 https://github.com/apache/datafusion/issues/25459 ?
Honestly, a colleague of mine 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
