kosiew commented on code in PR #26634: URL: https://github.com/apache/datafusion/pull/26634#discussion_r4548813561
########## datafusion/sqllogictest/test_files/nested_loop_join_conditional.slt: ########## @@ -0,0 +1,139 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +# Projection pushdown must not move a part of a nested loop join filter that +# is evaluated only for some rows into a projection below the join. Each +# query returns the same rows as its filter applied to the cross product of +# the inputs; evaluating the cast for every row of `cond_l` fails on 'x'. +# The top-level conjuncts of the filter are still searched in full. + +statement ok +SET datafusion.execution.target_partitions = 1 + +statement ok +CREATE TABLE cond_l(s VARCHAR) AS VALUES ('1'), ('x') + +statement ok +CREATE TABLE cond_r(v INT) AS VALUES (0), (5) + +statement ok +CREATE TABLE cond_r2(v INT) AS VALUES (0), (5), (-1) + +# CASE THEN branch +query TI +SELECT l.s, r.v FROM cond_l l JOIN cond_r r Review Comment: As an optional coverage improvement, could we also test a guarded ELSE containing an invalid cast, such as `CASE WHEN r.v < 100 THEN 0 ELSE CAST(l.s AS INT) END > 1`? The existing tests cover THEN and later WHEN branches, but not an explicit ELSE; this query should return zero rows without an error. ########## datafusion/physical-optimizer/src/projection_pushdown.rs: ########## @@ -433,6 +465,28 @@ impl<'a> JoinFilterRewriter<'a> { } } +/// Returns the number of leading children of `expr` that are evaluated for every row, or `None` +/// if all children are. The remaining children may be skipped for some rows. +/// +/// Mirrors `conditional_children` in the logical `CommonSubexprEliminate` rule. +fn always_evaluated_child_count(expr: &dyn PhysicalExpr) -> Option<usize> { + if let Some(case) = expr.downcast_ref::<CaseExpr>() { + // The children are the optional base expression, then each WHEN/THEN pair, then the + // optional ELSE. Only the base expression and the first WHEN are always evaluated. + Some(usize::from(case.expr().is_some()) + 1) Review Comment: **[high]** A simple CASE does not always evaluate its first WHEN. When the base is NULL, `CaseBody::case_when_with_expr` skips WHEN evaluation, so `Some(2)` can hoist a guarded cast and cause an error. Please return `Some(1)` for both CASE forms and add a regression using `CASE r.v WHEN CAST(l.s AS INT) THEN 1 ELSE 0 END = 1` with `r.v = NULL` and `l.s = 'x'`, verifying zero rows without an error. ########## datafusion/physical-optimizer/src/projection_pushdown.rs: ########## @@ -433,6 +465,28 @@ impl<'a> JoinFilterRewriter<'a> { } } +/// Returns the number of leading children of `expr` that are evaluated for every row, or `None` +/// if all children are. The remaining children may be skipped for some rows. +/// +/// Mirrors `conditional_children` in the logical `CommonSubexprEliminate` rule. +fn always_evaluated_child_count(expr: &dyn PhysicalExpr) -> Option<usize> { + if let Some(case) = expr.downcast_ref::<CaseExpr>() { + // The children are the optional base expression, then each WHEN/THEN pair, then the + // optional ELSE. Only the base expression and the first WHEN are always evaluated. + Some(usize::from(case.expr().is_some()) + 1) + } else if let Some(binary) = expr.downcast_ref::<BinaryExpr>() { + matches!(binary.op(), Operator::And | Operator::Or).then_some(1) + } else if let Some(func) = expr.downcast_ref::<ScalarFunctionExpr>() { Review Comment: As a follow-up, consider focused rewriter tests for the `ScalarFunctionExpr` and `HigherOrderFunctionExpr` short-circuit branches, since the coalesce SQL test lowers to CASE. Use mixed-side expressions with extractable single-side arguments to verify that `short_circuits() = true` blocks argument extraction while eager functions permit it. -- 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]
