Fix CASE evaluation for custom column expressions - #24484
Conversation
Signed-off-by: Hasnaat Hussain <hasnaat.hussain.2@gmail.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #24484 +/- ##
==========================================
- Coverage 81.24% 81.24% -0.01%
==========================================
Files 1113 1113
Lines 392744 392808 +64
Branches 392744 392808 +64
==========================================
+ Hits 319090 319122 +32
- Misses 54900 54924 +24
- Partials 18754 18762 +8 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
kosiew
left a comment
There was a problem hiding this comment.
Thanks for working on this. I tested the CASE expression changes locally and the fix looks correct for the reported custom PhysicalExpr failure. The new regression test also fails as expected if the projection guards are removed, so it is exercising the actual bug.
I left a few non-blocking suggestions below. The main thing I would like us to be explicit about is that this is a conservative fix for the custom leaf case, rather than a complete solution to the broader scope-aware dependency problem discussed in #21231.
Overall, the fallback to evaluating the original CASE body against the full batch looks correct to me.
| expr.downcast_ref::<LambdaVariable>() | ||
| { | ||
| used_column_indices.insert(lambda_variable.index()); | ||
| } else if expr.downcast_ref::<Literal>().is_none() |
There was a problem hiding this 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.
| } | ||
|
|
||
| #[test] | ||
| fn case_without_expr_with_custom_column() -> Result<()> { |
There was a problem hiding this 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.
| struct ProjectedCaseBody { | ||
| projection: Vec<usize>, | ||
| body: CaseBody, | ||
| supports_projection: bool, |
There was a problem hiding this 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.
| } else if expr.downcast_ref::<Literal>().is_none() | ||
| && expr.children().is_empty() | ||
| { | ||
| // Unknown leaves may read input columns without exposing a Column child. |
There was a problem hiding this 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.
| ) -> Result<ColumnarValue> { | ||
| let return_type = self.data_type(&batch.schema())?; | ||
| // projected.projection may include indexes of lambda variables not available on this batch | ||
| let projection = projected |
There was a problem hiding this 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.
Which issue does this PR close?
Rationale for this change
A custom column-like
PhysicalExprcan read an input column without downcasting to DataFusion's concreteColumntype. CASE's internal projection then misses that dependency and evaluates the expression against a zero-column batch, causing a runtime error.What changes are included in this PR?
CASE projection now falls back to the original input batch when it encounters an unknown leaf expression. Known columns, lambda variables, literals, and composite expressions keep the existing projection optimization.
The regression test uses a custom
PhysicalExprthat wraps a column without exposing a concreteColumnnode.Are these changes tested?
Yes:
cargo fmt --all -- --checkcargo clippy -p datafusion-physical-expr --all-targets --all-features -- -D warningscargo test -p datafusion-physical-expr expressions::case::tests(36 passed)cargo test -p datafusion-physical-expr(1,596 passed, 2 ignored; 13 doctests passed)The extended workspace command also reached the existing TPC-H q15 unparser failure. Its multi-statement error reproduces unchanged at the base commit, outside this patch.
Are there any user-facing changes?
Custom physical expressions can now be evaluated correctly inside searched CASE expressions. There is no public API change.