pepijnve commented on PR #17991: URL: https://github.com/apache/datafusion/pull/17991#issuecomment-3386434611
> Couple of thoughts though: `NVL` is a specific case of `COALESCE` with 2 args, however NVL should not be short circuited like COALESCE. > > Short circuit looks a nice feature for me to avoid side effects, thus it is more safe to use. > > Major engines btw doesn't support NVL, preferring COALESCE. I check Spark which supports NVL but they also use safer version with short circuit > > The only thing the plan maybe confusing perhaps we can document it The issue this PR is intended to solve requests lazy argument evaluation (i.e. short circuiting). The implementation across engines varies it seems. See https://github.com/apache/datafusion/issues/17982#issuecomment-3384780793 If we want to stick to eager evaluation for nvl, then we should probably reject the #17982. Regarding the plan change, the alternative in #17997 avoids the plan change by letting the `nvl` implementation handle lazy evaluation itself. -- 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]
