andygrove commented on PR #4827: URL: https://github.com/apache/datafusion-comet/pull/4827#issuecomment-5441831899
> **Note on this review:** this was generated by an LLM (Claude Code) at my request while I worked through a review backlog. I have not verified the individual findings myself. Please treat everything below as suggestions to evaluate rather than as authoritative review feedback, and push back on anything that is wrong or already handled. This is a good idea and the implementation is careful. Losing the whole native island because one expression in a projection has no translation is a real cliff, and detouring just that subtree through machinery that already exists on main is the cheapest possible way to fix it. The reasoning for a plain `ThreadLocal` over `DynamicVariable`, and scoping the detour to projections and filters only until other contexts are benchmarked, are both the right calls and are well explained in the comments. Three things. **Short-circuit semantics** This is my main concern. Spark's `If`, `CaseWhen`, `And`, `Or`, and `Coalesce` short-circuit: the unevaluated branch is never evaluated for rows that do not reach it. A detour turns a subexpression into an operand that is evaluated for the whole batch. So for something like: ```sql SELECT CASE WHEN x > 0 THEN 1 ELSE unsupported_fn(y) END FROM t ``` if `CaseWhen` converts natively and `unsupported_fn(y)` detours, the detour runs on every row including those where `x > 0`. If `unsupported_fn` can throw (a division by zero, an ANSI cast overflow, an array index out of range), Comet throws where Spark returns a row. #5409 hit the same issue and dealt with it by refusing to admit nested source expressions for exactly this reason. Does the detour hook guard against firing inside a conditional branch? If it does not, that is a correctness bug rather than a performance question, and it needs either a guard or a documented restriction. **"Outermost" in the comment may be backwards** The comment says the hook retries "the *outermost* unsupported node". Since `exprToProtoInternal` recurses depth-first, the innermost failing node reaches its own `.orElse` first, so I read the actual behavior as "detour the innermost node that fails, and widen outward only if that detour also fails". That is better behavior than what the comment describes, but the wording will send the next reader looking for the wrong thing. Worth rewording, and worth a test that pins which node actually gets detoured for a two-level unsupported tree. **No benchmark numbers** A 195-line benchmark is added but the description has no results. The premise is that one FFI round trip per batch for a detoured subexpression beats losing the whole native island, which is very plausible but data-dependent: if the detoured expression sits on a wide column, or if the operator was cheap anyway, the detour could be slower than the fallback. Numbers would also inform the guard question above: if the win is large, it is worth building a proper short-circuit-aware guard; if it is marginal, restricting the detour to non-conditional contexts is a fine simplification. **One smaller note** Clearing `FALLBACK_REASONS` and setting `withInfo` on a successful detour, to preserve the `withInfo`-xor-`withFallbackReason` invariant, is a nice detail. Is there a test asserting that a detoured node does not appear in the operator-level fallback rollup in `EXPLAIN` output? That invariant is easy to break from the other side later. -- 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]
