kazantsev-maksim commented on PR #4744:
URL:
https://github.com/apache/datafusion-comet/pull/4744#issuecomment-5750035150
Thanks for the guidance, @sunchao! Moving strict masking into the native
physical path via `ShortCircuitBinaryExpr` cleanly resolves the evaluation
semantics without having to maintain fragile AST whitelists/blacklists in Scala.
### Implementation Details
1. **`ShortCircuitBinaryExpr` (`lambda.rs`):**
- **SQL Three-Valued Logic (3VL):**
- **`AND`**: RHS is evaluated only when LHS is `TRUE` or `NULL`
(skipped when LHS is strictly `FALSE`).
- **`OR`**: RHS is evaluated only when LHS is `FALSE` or `NULL`
(skipped when LHS is strictly `TRUE`).
- **Zero-row / unneeded RHS short-circuit:** When no elements require RHS
evaluation (`true_count == 0`), RHS evaluation is skipped entirely. This
protects stateful expressions (`monotonically_increasing_id`, `rand`) and
fallible operations (`1 DIV x`, `element_at(..., 0)`, `abs(INT_MIN)`).
- **Masked evaluation via `evaluate_selection`:** When a subset of rows
requires the RHS, we evaluate via DataFusion's
`PhysicalExpr::evaluate_selection`, which filters the batch and scatters the
result back into place while preserving original element order. The arrays are
then combined using Arrow's Kleene boolean logic (`and_kleene` / `or_kleene`).
- Implements `children()`, `with_new_children()`, and `fmt_sql()` to
seamlessly support optimizer projection rewriting.
2. **Recursive Planning (`planner.rs`):**
- Added `rewrite_short_circuit_binary` in `PhysicalPlanner`, which
recursively walks the planned lambda body tree and replaces `BinaryExpr`
(`Operator::And` and `Operator::Or`) with `ShortCircuitBinaryExpr`.
- Preserved `EmptyBatchGuardExpr` to protect zero-row batches at the
outer lambda boundary.
3. **Scala Serde Cleanup & Fallbacks (`CometHighOrderFunction.scala`):**
- Removed AST whitelists for `AND` / `OR`. Supported expressions such as
`LIKE`, string functions, and nested struct access (`x.id IS NOT NULL AND
x.name LIKE 'a%'`) now remain fully native.
- Retained `try-catch NonFatal` during speculative serialization to
prevent eager planning exceptions (e.g. `cast.eval()` in guarded branches).
- Retained codegen dispatch fallback for guarded fallible branches in
`CASE WHEN`, `IF`, and `COALESCE`.
4. **Documentation Updates:**
- Updated the `## lambda_funcs` documentation table: `filter` is now
documented as **`Hybrid`** (single-argument lambdas and `array_compact` run
natively with strict per-element masking; multi-argument lambdas with index and
unsupported shapes fall back to JVM codegen dispatch).
- Updated ScalaDoc on `CometHighOrderFunction` and configuration
docstrings.
### Regression Test Coverage
Added native SQL fixture tests under ANSI mode covering:
- **All 5 reported cases:**
- Stateful counter: `x = 0 OR monotonically_increasing_id() = 0` (counter
no longer advances speculatively).
- PRNG state: `x = 0 OR rand(42L) > 0.5`.
- Non-ANSI runtime failure: `x = 0 OR element_at(array(1, 2), 0) = 1`
(index 0 is protected).
- Arithmetic overflow: `x = 0 OR abs(-2147483648) > 0`.
- Division by zero: `x <> 0 AND (1 DIV x) > 0` and `x = 0 OR (1 DIV x) >
0`.
- **Nullable boolean conditions (3VL):**
- Evaluates RHS when LHS is `NULL` (e.g. `(x > 0) OR (x IS NULL)`
preserving `NULL` elements).
- **Nested predicates:**
- Multi-level compound predicates combining `AND` and `OR` with division
guards.
- **Empty & mixed array batches:**
- `[]` and `NULL` rows with `1 DIV spark_partition_id()`.
- **Baseline control:**
- Standard comparisons (`x > 0 AND x < 10`) continue to execute natively.
--
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]
