#17982 Make nvl a thin wrapper for coalesce

[pepijnve]

Which issue does this PR close?

• Closes Implement lazy evaluation for nvl #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.

[pepijnve]

PR #17997 is an alternative implementation for lazy argument evaluation along with an implementation for nvl.

[comphead]

Thanks @pepijnve I think it makes a lot of sense.

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

[pepijnve]

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 #17982 (comment)

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.

[comphead]

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

[alamb]

Thanks @pepijnve and @comphead -- I took the liberty of

1. merging this PR up from main
2. updating the API comments a bit to explain the return value
3. Adding some explicit tests for nvl short circuiting