Skip to content

feat: add mandatory demand mask to scalar_fn execution#8854

Draft
joseph-isaacs wants to merge 8 commits into
developfrom
claude/scalar-fn-demand-mask-r6yk9n
Draft

feat: add mandatory demand mask to scalar_fn execution#8854
joseph-isaacs wants to merge 8 commits into
developfrom
claude/scalar-fn-demand-mask-r6yk9n

Conversation

@joseph-isaacs

@joseph-isaacs joseph-isaacs commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Rationale for this change

This PR introduces the demand-mask API for scalar functions and migrates the first fallible fn family (checked numeric arithmetic) to honor it. Nothing produces a non-all-true mask in production yet, so there is no behavior change for existing pipelines.

A demand mask is an execution-time non-nullable boolean array (length = row_count) telling a scalar fn which rows' output values the caller will observe. It is the non-compacting analogue of DataFusion's PhysicalExpr::evaluate_selection mask and Velox's SelectivityVector. Because it is an ArrayRef, demand can use any encoding — constant, run-end, or a lazy expression — and consumers materialize it once at the kernel boundary via child_to_validity + Validity::execute_mask (O(1) for the constant all-true case). The contract, documented on ExecutionArgs::demand and ScalarFnVTable::execute:

  • The result always contains row_count rows — demand never compacts.
  • Undemanded rows are don't-care: any well-typed value (including null) is acceptable at those positions.
  • Fallible fns (see ScalarFnVTable::is_fallible) must not raise a domain error attributable solely to undemanded rows. This is the same speculative-evaluation hazard the is_fallible docs already describe for dict-values pushdown — demand is the general fix for it.
  • Infallible fns may ignore the mask entirely; for them it is purely a performance hint.

Demand is not part of a scalar fn's identity: it does not affect return_dtype, Eq/Hash, serialization, or simplification. It is purely an input to execute.

How do we ensure fns actually respect the mask? (semantic break, not a type break)

A fn that ignores the mask still compiles, returns correct values at every demanded position, and only misbehaves by erroring on rows nobody asked about. That failure mode only becomes reachable once something produces a non-all-true mask, so enforcement is staged:

  1. No producers until migration completes. Nothing in the tree constructs a non-all-true demand today (the dynamic fn only forwards). Demand pushdown (scan filter-conjunct loop, projection-under-filter, execute_parent kernels) is deliberately deferred to its own track, gated on step 3 completing. Until then the mask is semantically inert and there is no window for wrong behavior.
  2. Conformance harness as the contract check (follow-up). A shared property harness (alongside compute/conformance/) asserting, per fn: filter(execute(args, demand), demand) == filter(execute(args, all_true), demand) whenever the all-true run succeeds; and for fallible fns, inputs producing domain errors only at undemanded positions must succeed. A fn "respects demand" iff it passes this harness — that is the definition, not code inspection.
  3. Exhaustive migration checklist enforced by test. A registry-driven test enumerates every scalar fn id registered in the default session and requires each to appear in exactly one of MIGRATED (has conformance coverage) or PENDING (known not yet migrated). Adding a new fn without choosing fails CI, and the pushdown PR's precondition becomes mechanical: PENDING contains no fallible fns.
  4. Runtime tripwires. The choke-point assertions in TypedScalarFnInstance::execute (length and non-nullable-bool dtype) catch malformed demand universally; the harness additionally fuzzes random demand masks per fn so a migrated fn that later regresses fails loudly in CI rather than mis-erroring in a scan.
  5. Docs at the point of implementation. The contract lives on ScalarFnVTable::execute and ExecutionArgs::demand — the two things every new fn author reads — including the fallible/infallible split.

The one standing exception is the FFI vtable (scalar_fn/foreign.rs): it cannot honor demand until the C ABI grows a demand channel, so it stays in PENDING and future pushdown must never pass a non-trivial mask through foreign fns.

Follow-up PRs

  1. Core API + checked numeric arithmetic (this PR)
  2. Conformance harness + migration checklist test
  3. Remaining fallible fns (cast, like, between, list_*, variant_get, json_to_variant, geo, tensor, row encode/size) — same pattern: keep the bulk kernel, consult demand only on the error path
  4. Demand transformers (case_when, zip, fill_null) refining demand for their branches
  5. (separate track) Executor threading for deferred ExecutionResult steps, execute_parent kernel demand transformation, then scan pushdown

What changes are included in this PR?

Core API:

  • ExecutionArgs::demand() -> &ArrayRef — new required method returning a non-nullable boolean array (deliberately not Option: every execution has a demand, possibly all-true; any array encoding is allowed).
  • VecExecutionArgs::new(inputs, row_count, demand) takes the demand array explicitly (panics on length or dtype violation); VecExecutionArgs::all(inputs, row_count) supplies a constant-true array.
  • All five construction sites updated: ScalarFnArray execution, scalar_at, validity execution, and the vortex-row encoder pass constant-true; fns/dynamic.rs forwards its incoming demand to its Binary delegate so demand is never silently dropped mid-tree.
  • TypedScalarFnInstance::execute (the choke point every execution flows through) asserts the demand length and Bool(NonNullable) dtype alongside the existing row-count/dtype assertions.

First fallible-fn migration — checked numeric arithmetic (Binary add/sub/mul/div):

  • execute_checked_typed materializes demand once (child_to_validity + execute_mask) and intersects it with the valid-lanes mask into care_lanes (the lanes that must produce correct values and whose errors are observable), driving the existing kernels with it — invalid and undemanded lanes were already zero-filled/skipped by the mask machinery, so error suppression and work-skipping fall out of the same intersection. The constant-constant fold errors only when at least one lane is both valid and demanded.
  • The constant-true demand path short-circuits through child_to_validity's constant fast path (AllValidMask::AllTrue, O(1)) and is behaviorally identical to the previous code (verified with the binary_ops divan bench).

Tests: demand plumbing through type-erased dispatch (probe vtable), length- and nullability-invariant rejection at construction and execution, dynamic delegate forwarding, and 9 checked-arithmetic cases covering all operand shapes (array-array, array-constant, constant-array, constant-constant), both kernel styles (split add/sub/mul and one-pass integer div), errors suppressed at undemanded/null lanes, and errors still raised at demanded lanes.

Checks run:

  • cargo build -p vortex-array -p vortex-row
  • cargo nextest run -p vortex-array (3054 passed), cargo nextest run -p vortex-row (23 passed)
  • cargo clippy --all-targets --all-features -p vortex-array -p vortex-row
  • cargo +nightly fmt --all, git diff --check
  • cargo bench --bench binary_ops before/after (all-true demand path at baseline)

What APIs are changed? Are there any user-facing changes?

Breaking for implementors/constructors of ExecutionArgs only:

  • ExecutionArgs gains a required demand() method returning &ArrayRef — external implementors must provide one (holding a ConstantArray::new(true, row_count) is the drop-in upgrade).
  • VecExecutionArgs::new gains a demand: ArrayRef parameter — external callers wanting the old behavior switch to VecExecutionArgs::all.

ScalarFnVTable implementors are unaffected at the type level; they gain the documented (currently inert) semantic obligation described above. Checked numeric execution under an all-true demand is unchanged; under a partial demand it now suppresses domain errors at undemanded lanes, which is only observable to callers that pass such a mask. No file format, serialization, or FFI changes.

🤖 Generated with Claude Code

https://claude.ai/code/session_01GhAvQmWLipNHBKjrZhxnKv

joseph-isaacs and others added 2 commits July 20, 2026 13:15
Introduce ExecutionArgs::demand(), a required Mask identifying the rows
whose output values the caller will observe. Undemanded rows are
don't-care: any well-typed value is acceptable, and fallible scalar fns
must not raise a domain error attributable solely to undemanded rows.
Demand never compacts: results keep the full row count.

VecExecutionArgs::new now takes the demand mask explicitly (asserting
its length matches the row count) and VecExecutionArgs::all constructs
the all-demanded case. All construction sites pass AllTrue except the
dynamic comparison fn, which forwards its incoming demand to its Binary
delegate. TypedScalarFnInstance::execute asserts mask length at the
execution choke point.

No producer constructs a non-trivial mask yet; demand pushdown is
deferred until all fallible fns honor the contract.

Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GhAvQmWLipNHBKjrZhxnKv
…legate forwarding

Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GhAvQmWLipNHBKjrZhxnKv
@github-actions

github-actions Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Polar Signals Profiling Results

Latest Run

Status Commit Job Attempt Link
🟢 Done fcdd188 1 Explore Profiling Data
Previous Runs (7)
Status Commit Job Attempt Link
🟢 Done 4842cc1 1 Explore Profiling Data
🟢 Done a97717c 1 Explore Profiling Data
🟢 Done c0a843b 1 Explore Profiling Data
🟢 Done 32c5287 1 Explore Profiling Data
🟢 Done 992b124 1 Explore Profiling Data
🟢 Done e479e79 1 Explore Profiling Data
🟢 Done e726ee2 1 Explore Profiling Data

Powered by Polar Signals Cloud

@github-actions

github-actions Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Benchmarks: Vortex queries 📖

Verdict: No clear signal (low confidence)
Attributed Vortex impact: +1.1%
Engines: DataFusion No clear signal (+9.0%, low confidence) · DuckDB No clear signal (-6.2%, low confidence)
Vortex (geomean): 0.941x ➖
Parquet (geomean): 0.936x ➖
Shifts: Parquet (control) -6.4% · Median polish -5.4%

How to read Verdict and Engines
  • Verdict: Overall PR-level signal after subtracting baseline drift estimated from Parquet control rows. It can be Likely improvement, Likely regression, or No clear signal.
  • Engines: Per-engine attribution. DataFusion is compared against DataFusion/Parquet controls; DuckDB is compared against DuckDB/Parquet controls. This answers whether each engine improved or regressed independently.
  • Confidence: Based on directional consistency, share of rows above the noise floor, and control-run noise.

datafusion / vortex-file-compressed (0.969x ➖, 0↑ 0↓)
name PR fcdd188 (ns) base 1113b83 (ns) ratio (PR/base)
vortex_q00/datafusion:vortex-file-compressed 9715465 9897812 0.98
vortex_q01/datafusion:vortex-file-compressed 6079544 6349778 0.96
datafusion / parquet (0.890x ✅, 1↑ 0↓)
name PR fcdd188 (ns) base 1113b83 (ns) ratio (PR/base)
vortex_q00/datafusion:parquet 19815536 21935536 0.90
vortex_q01/datafusion:parquet 🚀 4525394 5164883 0.88
duckdb / vortex-file-compressed (0.923x ➖, 0↑ 0↓)
name PR fcdd188 (ns) base 1113b83 (ns) ratio (PR/base)
vortex_q00/duckdb:vortex-file-compressed 9917455 10659218 0.93
vortex_q01/duckdb:vortex-file-compressed 5930711 6480145 0.92
duckdb / parquet (0.984x ➖, 0↑ 0↓)
name PR fcdd188 (ns) base 1113b83 (ns) ratio (PR/base)
vortex_q00/duckdb:parquet 23205275 23522821 0.99
vortex_q01/duckdb:parquet 9300660 9472792 0.98

No file size changes detected.

joseph-isaacs and others added 4 commits July 21, 2026 09:44
Intersect the demand mask with the valid-lanes mask in the Binary
checked arithmetic family (add/sub/mul/div) so domain errors at
undemanded lanes are suppressed and one-pass kernels skip undemanded
work. The all-true demand path is unchanged.

Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GhAvQmWLipNHBKjrZhxnKv
…e mask buffer

Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GhAvQmWLipNHBKjrZhxnKv
The operand split (PrimitiveOperand) and the validity-and-demand merge
(care_lanes) both happen before the kernels run, so the three shape
dispatchers and twelve shape-specific kernel wrappers reduce to a single
checked_op generic over a lane accessor. Slices and constants are bound
outside the closures so codegen matches the old direct-slice kernels
(verified with the binary_ops divan bench).

Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GhAvQmWLipNHBKjrZhxnKv
…ne dispatch"

This reverts commit 32c5287897299552896518135705421b9de85df9, restoring the
per-shape checked kernels.

Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GhAvQmWLipNHBKjrZhxnKv
@joseph-isaacs joseph-isaacs added the changelog/feature A new feature label Jul 21, 2026
@joseph-isaacs
joseph-isaacs force-pushed the claude/scalar-fn-demand-mask-r6yk9n branch from c0a843b to a97717c Compare July 21, 2026 10:43
ExecutionArgs::demand now returns an ArrayRef (Bool NonNullable) instead
of a vortex-mask Mask, so demand can use any array encoding: constant,
run-end, or a lazy expression. Typed dispatch asserts both the length
and the dtype invariant. Consumers materialize once at the kernel
boundary via child_to_validity + Validity::execute_mask, which keeps the
constant all-true case O(1) (verified with the binary_ops bench).

Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GhAvQmWLipNHBKjrZhxnKv
@codspeed-hq

codspeed-hq Bot commented Jul 21, 2026

Copy link
Copy Markdown

Merging this PR will improve performance by 25.67%

⚡ 2 improved benchmarks
✅ 1794 untouched benchmarks
⏩ 46 skipped benchmarks1

Performance Changes

Mode Benchmark BASE HEAD Efficiency
Simulation copy_nullable[65536] 1.4 ms 1 ms +31.86%
Simulation copy_non_nullable[65536] 1,094.3 µs 913.7 µs +19.76%

Tip

Curious why this is faster? Comment @codspeedbot explain why this is faster on this PR, or directly use the CodSpeed MCP with your agent.


Comparing claude/scalar-fn-demand-mask-r6yk9n (fcdd188) with develop (c42934b)2

Open in CodSpeed

Footnotes

  1. 46 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.

  2. No successful run was found on develop (1113b83) during the generation of this report, so c42934b was used instead as the comparison base. There might be some changes unrelated to this pull request in this report.

u
Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changelog/feature A new feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant