kazantsev-maksim commented on PR #4744:
URL:
https://github.com/apache/datafusion-comet/pull/4744#issuecomment-5999157766
Thanks for the reproducible example, @sunchao! That was the missing piece —
Spark's `BinaryExpression` codegen evaluates LHS first and skips RHS evaluation
whenever LHS is `NULL` (since `NULL <op> anything` is always `NULL` in SQL 3VL).
Rather than trying to detect fallible expressions in Scala, I have resolved
this directly in Rust by extending `ShortCircuitBinaryExpr` to cover comparison
operators (`=`, `!=`, `<`, `<=`, `>`, `>=`):
1. **Comparison Operand Masking (`ShortCircuitBinaryExpr`):**
- For all comparison operators, the selection mask for RHS is
`!lhs_arr.is_null()`.
- If all LHS elements in the batch are `NULL` (`true_count == 0`), RHS
evaluation is skipped entirely, and an all-null boolean array is returned
directly. This guarantees that fallible expressions (such as `1D /
spark_partition_id()`) are never evaluated on null elements.
- If only a subset of elements is non-null, RHS is evaluated strictly on
matching rows via `evaluate_selection`.
2. **Physical Planner (`planner.rs`):**
- `rewrite_short_circuit_binary` now rewrites `BinaryExpr` comparisons
(`Eq`, `NotEq`, `Lt`, `LtEq`, `Gt`, `GtEq`) into `ShortCircuitBinaryExpr`.
3. **Regression Test:**
- Added the test case `SELECT filter(a, x -> x = 1D /
spark_partition_id()) FROM t` with `a = [NULL]` to `array_filter_ansi.sql`
under `codegen.enabled=false`. It now executes fully natively and returns `[]`
without error.
All native and fallback suites pass. Could you please take another look?
--
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]
