kumarUjjawal commented on code in PR #25074:
URL: https://github.com/apache/datafusion/pull/25074#discussion_r3958797237
##########
datafusion/optimizer/src/unions_to_filter.rs:
##########
@@ -278,16 +292,18 @@ fn wrap_branch(mut plan: LogicalPlan, wrappers:
&[Wrapper]) -> Result<LogicalPla
Ok(plan)
}
-fn strip_passthrough_nodes(mut plan: LogicalPlan) -> LogicalPlan {
+/// Removes aliases below a branch filter, but refuses to remove projections.
+///
+/// A projection may compute new values, so dropping it can make branches with
+/// different results appear equivalent to `unions_to_filter`.
+fn strip_passthrough_nodes(mut plan: LogicalPlan) -> Option<LogicalPlan> {
loop {
plan = match plan {
- LogicalPlan::Projection(Projection { input, .. }) => {
- Arc::unwrap_or_clone(input)
- }
+ LogicalPlan::Projection(_) => return None,
Review Comment:
Removing `SubqueryAlias` has the same defect you just fixed for
projections? `SELECT x.a FROM t AS x WHERE x.a = 1 UNION SELECT x.a FROM t AS x
WHERE x.a = 2` plans as a filter over the alias over `TableScan: t`, so the
merged predicate still names `x.a` while the source only exposes `t.a`.
We should trip nothing and take the filter input as the source. `GroupKey`
equality then keeps different projections apart and still merges identical
ones, so views and derived tables keep the rewrite instead of losing it. That
source needs the volatility test that the wrappers already get.
##########
datafusion/optimizer/src/unions_to_filter.rs:
##########
@@ -200,14 +206,22 @@ fn extract_branch(plan: LogicalPlan) ->
Result<Option<UnionBranch>> {
debug!("unions_to_filter skipped: branch contains ORDER BY /
SORT");
Ok(None)
}
- other => Ok(Some(UnionBranch {
- source: strip_passthrough_nodes(other),
- predicate: Expr::Literal(
- datafusion_common::ScalarValue::Boolean(Some(true)),
- None,
- ),
- wrappers,
- })),
+ other => {
+ let Some(source) = strip_passthrough_nodes(other) else {
Review Comment:
`peel_wrappers` already consumed every `Projection` and `SubqueryAlias`
before this match, so the plan that reaches this arm can never be either one.
`strip_passthrough_nodes` returns `Some` on the first iteration here, and
the new debug message cannot fire. I would use `other` directly as the source.
--
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]
