viirya commented on code in PR #6763:
URL: https://github.com/apache/datafusion-comet/pull/6763#discussion_r4232815494


##########
native/spark-expr/src/conditional_funcs/case_when.rs:
##########
@@ -349,6 +350,13 @@ impl PhysicalExpr for CaseWhenExpr {
     }
 
     fn evaluate(&self, batch: &RecordBatch) -> Result<ColumnarValue> {
+        // No row chooses a branch, which both the eager and the lazy 
evaluation answer with a
+        // scalar NULL. A scalar from an empty batch is taken to mean the 
expression is constant,
+        // as IN does to build its list once, so return an empty array instead.
+        if batch.num_rows() == 0 {
+            let data_type = self.data_type(&batch.schema())?;
+            return Ok(ColumnarValue::Array(new_empty_array(&data_type)));

Review Comment:
   Thanks both. As @andygrove found, this predates the PR: `InListExpr` and 
`NestedPredicate` evaluate every later candidate over the whole batch, and `id 
IN (0L, CAST(s AS BIGINT))` already fails this way on main. This PR moves a 
column-reading CASE from the frozen constant onto that same path. I opened 
#6823 for the whole-batch evaluation of later candidates and linked it from 
#6006, and left the per-row evaluation out of this PR.



##########
native/spark-expr/src/conditional_funcs/case_when.rs:
##########
@@ -349,6 +350,13 @@ impl PhysicalExpr for CaseWhenExpr {
     }
 
     fn evaluate(&self, batch: &RecordBatch) -> Result<ColumnarValue> {
+        // No row chooses a branch, which both the eager and the lazy 
evaluation answer with a
+        // scalar NULL. A scalar from an empty batch is taken to mean the 
expression is constant,
+        // as IN does to build its list once, so return an empty array instead.

Review Comment:
   Yes, I think it should. `branch-1.1` uses the same DataFusion 55.1.0, and 
its `IfExpr::evaluate` delegates to `CaseExpr`, which returns a NULL THEN 
literal as a scalar for an empty batch, the same lazy path I confirmed returns 
a scalar NULL on main. I'll open a `branch-1.1` backport after this merges, 
with the check in `IfExpr::evaluate` and in a wrapper around the `CaseExpr` 
from `create_case_expr`, and verify the reproduction there first.



-- 
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]

Reply via email to