ScalarFn VTable

[gatesn]

The vtable itself describes the behavior of a scalar function. I envisage us turning Expression into an enum in the future where ScalarFn is but one variant.

This PR addresses the naming confusion (and prepares us to add AggregateFn VTable in the near future), as well as unifies the vtable names as per #6093

[gatesn]

@claude review this PR

[claude]

Claude finished @gatesn's task in 9m 6s —— View job

---

PR Review: ScalarFn VTable

• Gather context and examine diff
• Review core architectural changes
• Review naming/API changes across encodings
• Check for issues, bugs, or concerns
• Post final review

---

Overview

This PR introduces the scalar_fn module as the proper home for the ScalarFnVTable machinery, following the unified vtable pattern established in #6662. The changes rename/relocate types from crate::expr::* to crate::scalar_fn::* across ~100 files and introduce the full vtable pattern (ScalarFnVTable, ScalarFnPlugin, ScalarFn<V>, ScalarFnRef, DynScalarFn, ScalarFnInner<V>). The architecture looks solid and the design docs clearly reflect the intent.

---

🐛 Bug: Duplicate _ = expr; in stat_expression default

File: vortex-array/src/scalar_fn/vtable.rs, lines 159–165

fn stat_expression(...) -> Option<Expression> {
    _ = options;
    _ = expr;
    _ = expr;  // ← duplicate, `_ = stat;` is missing here!
    _ = stat;
    _ = catalog;
    None
}

The second _ = expr; at line 161 is a duplicate — it looks like _ = stat; was accidentally replaced with _ = expr; (then _ = stat; was added again at line 162). While functionally harmless (both expr and stat are still suppressed somewhere in the block), this is misleading and should be deduplicated. Fix this →

---

🐛 Stale debug struct name in erased.rs

File: vortex-array/src/scalar_fn/erased.rs, line 157

impl Debug for ScalarFnRef {
    fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
        f.debug_struct("BoundExpression")  // ← stale name

The debug format still says "BoundExpression" — this should be "ScalarFnRef" to match the current type name. Fix this →

---

📄 Documentation: session.md is stale (3 occurrences)

File: docs/developer-guide/internals/session.md

The session docs still use the old ExprSession name and the old accessor session.expressions():

Location	Current (stale)	Should be
Line 32 (table)	ExprSession	ScalarFnSession
Line 54 (code example)	session.expressions().register(...)	session.scalar_fns().register(...)
Line 105 (code example)	.with::<ExprSession>()	.with::<ScalarFnSession>()

Fix this →

---

📄 Documentation: vtables.md migration status is imprecise

File: docs/developer-guide/internals/vtables.md, lines 141–145

Two issues in the ScalarFn -- Done section:

1. Missing ScalarFnRef: Line 144 lists the components as ScalarFnVTable, ScalarFnPlugin, ScalarFnInner, DynScalarFn, ScalarFn — but the erased form ScalarFnRef is missing. Per the pattern table at line 18, the erased form should be FooRef. The list should be ..., ScalarFn<V>, ScalarFnRef.

2. Outdated factory function location: Line 145 says "Factory functions (e.g. root(), lit(), eq()) live in expr/exprs.rs". This remains true, but the new sub-functions in scalar_fn::fns (e.g. vtable types, reduce adaptors) are now accessed from there — worth clarifying.

---

⚠️ Dead code: NotSupported in scalar_fn/vtable.rs

File: vortex-array/src/scalar_fn/vtable.rs, line 342

/// A placeholder vtable implementation for unsupported optional functionality of an expression.
pub struct NotSupported;

This struct is declared but has no impl for any trait, and is not used anywhere in the codebase (confirmed by search). The counterpart in vtable/mod.rs has an OperationsVTable implementation. If this is intended to be a future helper (e.g., to implement ScalarFnVTable as a no-op sentinel), it needs at minimum a note and ideally a blanket impl. Otherwise it can be removed to avoid confusion.

---

ℹ️ Minor: unsafe cast in ScalarFnPlugin::new_static

File: vortex-array/src/scalar_fn/plugin.rs, lines 43–48

pub const fn new_static<V: ScalarFnVTable>(vtable: &'static V) -> Self {
    // SAFETY: We can safely cast the vtable to a ScalarFnInner since it has the same layout.
    let adapted: &'static ScalarFnInner<V> =
        unsafe { &*(vtable as *const V as *const ScalarFnInner<V>) };
    ...
}

This is sound given the #[repr(transparent)] on ScalarFnInner<V>, but the safety comment could be more explicit: "SAFETY: ScalarFnInner<V> is #[repr(transparent)] over V, so *const V and *const ScalarFnInner<V> have identical layout and alignment." Minor suggestion.

---

✅ Architecture looks good

• The vtable pattern is consistent with ExtDType and the design doc.
• The separation of ScalarFnVTable (non-object-safe, typed), DynScalarFn (object-safe, sealed), ScalarFnInner<V> (bridge), ScalarFnRef (erased) and ScalarFnPlugin (registry) is clean and well-structured.
• The debug-mode execution validation in typed.rs (row count and dtype checks) is a nice correctness guard.
• Making fns a public module (pub mod fns) while removing the blanket re-export (pub use fns::*) is a reasonable API boundary decision.
• The SimplifyCtx, ReduceNode, ReduceCtx traits are well-designed for extensibility.

---

Summary: 2 small bugs (duplicate _ = expr;, stale "BoundExpression"), 3 stale doc locations in session.md, 1 imprecise doc in vtables.md, and 1 dead-code struct. All minor — the core architecture is solid.

[codspeed-hq]

Merging this PR will degrade performance by 12%

⚡ 2 improved benchmarks
❌ 2 regressed benchmarks
✅ 950 untouched benchmarks
⏩ 1466 skipped benchmarks¹

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

	Mode	Benchmark	BASE	HEAD	Efficiency
❌	Simulation	bench_many_nulls[0.9]	482.2 µs	547.7 µs	-11.96%
❌	Simulation	decompress_alp[f32, (1000, 0.0, 0.25)]	10.8 µs	12.3 µs	-12%
⚡	Simulation	map_each[BufferMut<i32>, 128]	858.1 ns	770.6 ns	+11.36%
⚡	Simulation	bitwise_not_vortex_buffer_mut[128]	530.3 ns	471.9 ns	+12.36%

---

_{Comparing ngates/scalar-fn (431c639) with develop (ea7804c)}

Footnotes

1. 1466 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩