Skip to content

perf: Optimize scalar fast path for nanvl#20205

Open
kumarUjjawal wants to merge 1 commit intoapache:mainfrom
kumarUjjawal:perf/nanvl_scalar_pah
Open

perf: Optimize scalar fast path for nanvl#20205
kumarUjjawal wants to merge 1 commit intoapache:mainfrom
kumarUjjawal:perf/nanvl_scalar_pah

Conversation

@kumarUjjawal
Copy link
Contributor

Which issue does this PR close?

Rationale for this change

nanvl currently evaluates scalar inputs via make_scalar_function(nanvl, vec![]), which converts scalar values into
size‑1 arrays before execution and then converts back. This adds unnecessary overhead for constant folding / scalar
evaluation

What changes are included in this PR?

  • Add match-based scalar fast path for ColumnarValue::Scalar + ColumnarValue::Scalar
  • Add Criterion benchmarks:
    • nanvl/scalar_f64
    • nanvl/scalar_f32

Benchmark | Before | After | Speedup
━━━━━━━━━━━━━━━━━━━━━━━
nanvl/scalar_f64 | ~240.1 ns | 50.104 ns ~4.79x
nanvl/scalar_f32 |~237.1 ns | 49.284 ns ~4.81x

Are these changes tested?

Yes

Are there any user-facing changes?

No

@github-actions github-actions bot added the functions Changes to functions implementation label Feb 7, 2026
match (&x, &y) {
(ColumnarValue::Scalar(x), ColumnarValue::Scalar(y)) => {
// NULL propagation
if x.is_null() || y.is_null() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is quite right; from Spark:

>>> spark.sql("select nanvl(null, 1)").show()
+--------------+
|nanvl(NULL, 1)|
+--------------+
|          NULL|
+--------------+

>>> spark.sql("select nanvl(1, null)").show()
+--------------+
|nanvl(1, NULL)|
+--------------+
|           1.0|
+--------------+

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

functions Changes to functions implementation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants