kazantsev-maksim commented on PR #4744: URL: https://github.com/apache/datafusion-comet/pull/4744#issuecomment-5744410754
Thanks for the detailed follow-up and the great reproduction cases, @sunchao! The `monotonically_increasing_id()` and `rand()` examples really hit the nail on the head — speculative execution of conditionally skipped branches isn't just about arithmetic errors in ANSI mode, but can silently corrupt results for any stateful or non-deterministic expression. I looked into maintaining a Scala-side AST whitelist/blacklist for guarded branches, but it quickly became an endless, fragile game of whack-a-mole: - A conservative whitelist immediately broke valid, common queries like `x.id IS NOT NULL AND x.name LIKE 'a%'` (because `Like` and string predicates weren't on the list, causing fallback to JVM codegen dispatch, which cannot bind struct fields on `NamedLambdaVariable`). - A blacklist is equally impractical given the sheer number of Spark expressions that can fail or hold state (`element_at(0)`, `abs(INT_MIN)`, date arithmetic, etc.), across varying Spark versions and captured error modes. This points directly to your first suggestion: **preserving per-element evaluation masks**. The root cause is DataFusion's `BinaryExpr` for `AND`/`OR`, which skips masking when the active row count exceeds its 20% threshold. In array lambdas where batches contain very few rows (e.g. `[0, 1]`), this threshold is almost always exceeded. Instead of patching Scala with fragile AST inspections, what do you think about solving this on the physical execution side in Rust? I could introduce a strict boolean physical expression (e.g., `CometStrictAnd` / `CometStrictOr`, or an adapter during lambda physical planning) that: 1. Evaluates the left child. 2. For rows that require right-hand evaluation, filters the batch (`arrow::compute::filter`) so the right child is evaluated **only** on matching rows. 3. Merges the results back. This would: - Guarantee Spark's strict per-element short-circuiting semantics for all expressions (`DIV`, `monotonically_increasing_id`, `element_at`, `abs`, etc.). - Completely eliminate the need for AST whitelists/blacklists and captured error mode checks in Scala. - Keep all valid predicates (including `LIKE`, nested structs, etc.) running on the fast native path. Does implementing this strict masking adapter in Rust for lambda bodies sound like the right direction to you, or did you have a different mechanism in mind for preserving the evaluation mask? -- 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]
