kosiew commented on code in PR #24484:
URL: https://github.com/apache/datafusion/pull/24484#discussion_r3872993929
##########
datafusion/physical-expr/src/expressions/case.rs:
##########
@@ -141,6 +142,11 @@ impl CaseBody {
expr.downcast_ref::<LambdaVariable>()
{
used_column_indices.insert(lambda_variable.index());
+ } else if expr.downcast_ref::<Literal>().is_none()
Review Comment:
I think this check is a little broader than the specific hazard we are
trying to protect against. It will also disable CASE projection for some
in-tree leaf expressions that cannot actually read the input batch positionally.
For example, a nullary `ScalarFunctionExpr` such as `random()` has no
children, is not a `Literal`, and is not const-folded because it is volatile.
That means an expression like `CASE WHEN random() < 0.5 THEN a ELSE b END`
would now lose the projection fast path on a wide batch.
Would it make sense to keep known-safe DataFusion leaves projection-enabled,
and reserve this fallback for expressions whose dependencies we genuinely
cannot determine? If we prefer the conservative behavior for now, I think it
would be worth calling out the performance tradeoff in the PR description.
##########
datafusion/physical-expr/src/expressions/case.rs:
##########
@@ -1878,6 +1935,38 @@ mod tests {
Ok(())
}
+ #[test]
+ fn case_without_expr_with_custom_column() -> Result<()> {
Review Comment:
Nice regression test. One thing I noticed is that this currently covers only
the `NoExpression` evaluation path, while the PR adds the same projection guard
to `case_when_with_expr` and `expr_or_expr` as well.
Could we add small tests for those two shapes too? A base-expression CASE
using `CustomColumn`, plus a single-WHEN-with-ELSE case, would make sure all
three guarded paths stay covered if this code is refactored later.
##########
datafusion/physical-expr/src/expressions/case.rs:
##########
@@ -141,6 +142,11 @@ impl CaseBody {
expr.downcast_ref::<LambdaVariable>()
{
used_column_indices.insert(lambda_variable.index());
+ } else if expr.downcast_ref::<Literal>().is_none()
+ && expr.children().is_empty()
+ {
+ // Unknown leaves may read input columns without exposing
a Column child.
Review Comment:
This looks like a reasonable conservative fix for the reported crash. One
process point though: the discussion in #21231 was also considering a
`PhysicalExpr` dependency-reporting extension point, or a dedicated
opt-in/opt-out mechanism for projection.
This PR takes a narrower approach by treating unknown leaves as unsafe. I
think that is fine as a targeted fix, but it would be helpful to say so
explicitly in the PR description and use wording like `Part of #21231` rather
than closing the issue entirely. The separate scope-aware traversal problem
still remains.
##########
datafusion/physical-expr/src/expressions/case.rs:
##########
Review Comment:
Small cleanup suggestion: the logic that filters the projection to valid
batch indices, checks whether projection is supported, and decides whether
projection is worthwhile is now repeated in three places.
A helper on `ProjectedCaseBody`, something like `projection_for(&self,
batch) -> Option<Vec<usize>>`, could centralize that decision. That would also
make it harder for a future CASE evaluation path to accidentally forget the
`supports_projection` check.
##########
datafusion/physical-expr/src/expressions/case.rs:
##########
@@ -251,6 +258,7 @@ impl CaseBody {
struct ProjectedCaseBody {
projection: Vec<usize>,
body: CaseBody,
+ supports_projection: bool,
Review Comment:
Could we document the invariant on `supports_projection` here? When this is
`false`, the derived `projection` and rewritten `body` must not be used, and
evaluation needs to fall back to the original CASE body on the full batch.
That relationship is important enough that a short field comment, plus a
sentence in the `ProjectedCaseBody` docs, would make the representation much
easier to reason about.
--
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]
