Visorgood commented on PR #5558: URL: https://github.com/apache/datafusion-comet/pull/5558#issuecomment-5522758498
Thanks @sunchao – both confirmed against Spark's source. `doGenCode` evaluates the replacement first while `eval` checks the array first, and `ConstantFolding` leaves a throwing expression unfolded with `foldable` still true, so my `!_.foldable` test skipped the guard exactly where it was needed. I've changed the approach rather than patching it. Reproducing Spark's short-circuiting natively is open-ended – two rounds each found a shape I had missed, and since `eval` and `doGenCode` disagree, no single guard ordering matches both. So I took your suggestion of using the dispatcher for what cannot be represented and made it the rule. `getSupportLevel` now reports `Compatible` only when the delimiter and null replacement are each a literal or a column read: those cannot throw or carry state, so evaluation order is unobservable and the problem disappears instead of needing guards. Everything else runs through the dispatcher, which is `doGenCode` itself. `convert` keeps one guard, `IsNull` on a nullable replacement, which is the actual #3178 fix. `guardedArgs` is gone and `foldable` no longer enters the decision. All four of your cases resolve without a guard. More conservative than the last revision, but still strictly more native than `main`, where `array_join` never runs natively at all. -- 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]
