You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "pepijnve (via GitHub)" <gi...@apache.org> on 2025/10/09 10:00:17 UTC
[PR] #17982 Make `nvl` a thin wrapper for `coalesce` [datafusion]
pepijnve opened a new pull request, #17991:
URL: https://github.com/apache/datafusion/pull/17991
## Which issue does this PR close?
- Closes #17982
## Rationale for this change
By making `NVLFunc` a wrapper for `CoalesceFunc` with a more restrictive signature the implementation automatically benefits from any optimisation work related to `coalesce`.
## What changes are included in this PR?
- Make `NVLFunc` a thin wrapper of `CoalesceFunc`. This seemed like the simplest way to reuse the coalesce logic, but keep the stricter signature of `nvl`.
- Add `ScalarUDF::conditional_arguments` as a more precise complement to `ScalarUDF::short_circuits`. By letting each function expose which arguments are eager and which are lazy, we provide more precise information to the optimizer which may enable better optimisation.
## Are these changes tested?
Assumed to be covered by sql logic tests.
Unit tests for the custom implementation were removed since those are no longer relevant.
## Are there any user-facing changes?
The rewriting of `nvl` to `case when ... then ... else ... end is visible in the physical query plan.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] #17982 Make `nvl` a thin wrapper for `coalesce` [datafusion]
Posted by "pepijnve (via GitHub)" <gi...@apache.org>.
pepijnve commented on PR #17991:
URL: https://github.com/apache/datafusion/pull/17991#issuecomment-3385983672
PR #17997 is an alternative implementation for lazy argument evaluation along with an implementation for `nvl`.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] #17982 Make `nvl` a thin wrapper for `coalesce` [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #17991:
URL: https://github.com/apache/datafusion/pull/17991
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] #17982 Make `nvl` a thin wrapper for `coalesce` [datafusion]
Posted by "pepijnve (via GitHub)" <gi...@apache.org>.
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] #17982 Make `nvl` a thin wrapper for `coalesce` [datafusion]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on PR #17991:
URL: https://github.com/apache/datafusion/pull/17991#issuecomment-3386640529
> Regarding the plan change, the alternative in #17997 avoids the plan change by letting the `nvl` implementation handle lazy evaluation itself.
Thanks @pepijnve I think we can document it for now in user doc, that NVL is lazily evaluated
```
pub struct NVLFunc
```
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org