Skip to content

Make ScalarFn array validity lazy when the function defines a validity expression#8336

Draft
joseph-isaacs wants to merge 1 commit into
developfrom
claude/cool-bardeen-l8jlsy-4-lazy-scalarfn-validity
Draft

Make ScalarFn array validity lazy when the function defines a validity expression#8336
joseph-isaacs wants to merge 1 commit into
developfrom
claude/cool-bardeen-l8jlsy-4-lazy-scalarfn-validity

Conversation

@joseph-isaacs

@joseph-isaacs joseph-isaacs commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

Summary

Lazy validity for ScalarFn arrays. (Originally part of a 4-PR stack; the prerequisites — definitely_no_nulls/execute_no_nulls in #8333, the mask_eq semantics fix in #8334, and definitely_all_null — have all since landed on develop, so this PR is now a single standalone commit rebased onto current develop.)

Previously ValidityVTable<ScalarFn> always eagerly executed the validity expression via the legacy session. Now, when the scalar function provides a validity expression over its inputs, the expression is converted into a lazy ScalarFn array DAG instead:

  • Literal nodes → ConstantArray
  • ArrayExpr leaves → unwrap to the child array they hold
  • interior nodes → lazy ScalarFn arrays via Array::<ScalarFn>::try_new_with_len
  • constant results are folded back into AllValid/AllInvalid via child_to_validity

Why the eager path remains for some functions

Functions that don't define a validity expression (Kleene and/or, where validity depends on the computed values) keep the eager path. The erased fallback for these is is_not_null(expr) — self-referential: lazily materializing it means resolving the validity of the inner node, which spawns another is_not_null DAG over a fresh copy of the same node, recursing without ever shrinking. This manifested as a stack overflow in element-wise execution paths (execute_scalaris_invalidvalidity()), caught by test_bool_consistency. The new ScalarFnRef::validity_opt exposes whether a function defines its own validity expression so the vtable can pick the right path.

Checks

  • cargo nextest run -p vortex-array (3197 passed)
  • cargo nextest run -p vortex-file -p vortex-layout -p vortex-scan -p vortex-btrblocks -p vortex-compressor -p vortex-datafusion -p vortex-fastlanes -p vortex-runend -p vortex-sparse -p vortex-fsst -p vortex-alp -p vortex-zstd -p vortex-pco -p vortex-datetime-parts -p vortex-ipc (1429 passed)
  • cargo clippy -p vortex-array --all-targets, cargo +nightly fmt --check
  • Could not run vortex-cuda, vortex-duckdb, or lance-dependent benches locally (CUDA toolkit / DuckDB source download / protoc unavailable in the sandbox) — CI covers them.

https://claude.ai/code/session_01VPQ7dfZtijfrsjAipwXvEj

@joseph-isaacs joseph-isaacs added the changelog/feature A new feature label Jun 10, 2026 — with Claude
@codspeed-hq

codspeed-hq Bot commented Jun 10, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

⚡ 2 improved benchmarks
❌ 1 regressed benchmark
✅ 1627 untouched benchmarks
⏩ 42 skipped benchmarks1

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Mode Benchmark BASE HEAD Efficiency
Simulation compact_sliced[(4096, 90)] 779.7 ns 867.2 ns -10.09%
Simulation chunked_varbinview_into_canonical[(100, 100)] 307.5 µs 272.6 µs +12.79%
Simulation bitwise_not_vortex_buffer_mut[128] 273.6 ns 244.4 ns +11.93%

Tip

Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.


Comparing claude/cool-bardeen-l8jlsy-4-lazy-scalarfn-validity (5a83d1d) with develop (42bea5d)

Open in CodSpeed

Footnotes

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

Copy link
Copy Markdown
Contributor Author

Investigated the CodSpeed varbinview_zip_* regressions — they are a codegen-layout artifact, not a real cost of this change:

  1. The changed code never runs in these benchmarks. Instrumenting ScalarFn::validity with a counter shows 0 calls during varbinview_zip_block_mask/varbinview_zip_fragmented_mask (vs ~47k calls in the bool-consistency tests, confirming the instrumentation works). The benches go straight through the dedicated ZipKernel for VarBinView and never query a ScalarFn array's validity.

  2. Bisecting the two files in this PR reproduces the full ~19% delta locally only when arrays/scalar_fn/vtable/validity.rs changes — the file whose code is never executed here. The scalar_fn/erased.rs change alone shows no delta.

  3. codegen-units=1 erases the regression entirely (branch 3: 505µs median vs branch 3 + this PR's files: 494µs). With default codegen units, adding code to that module shifts LLVM's CGU partitioning and changes inlining in the unrelated hot zip loop, which CodSpeed's instruction-counting simulation faithfully reports.

The same flavor of noise shows in the rest of the stack: the bitwise_not_vortex_buffer_mut and chunked_*_canonical_into benchmarks flip between ±20–47% on adjacent PRs that don't touch vortex-buffer or the chunked builders at all.

I can't acknowledge regressions on the CodSpeed dashboard from this session — that needs someone with dashboard access.

https://claude.ai/code/session_01VPQ7dfZtijfrsjAipwXvEj


Generated by Claude Code

/// Transforms the expression into one representing the validity of this expression.
pub fn validity(&self, expr: &Expression) -> VortexResult<Expression> {
Ok(self.0.validity(expr)?.unwrap_or_else(|| {
Ok(self.validity_opt(expr)?.unwrap_or_else(|| {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

do you want to remove the TODO?

@joseph-isaacs joseph-isaacs force-pushed the claude/cool-bardeen-l8jlsy-4-lazy-scalarfn-validity branch from d72800b to f4e8d62 Compare June 11, 2026 10:17
@joseph-isaacs joseph-isaacs force-pushed the claude/cool-bardeen-l8jlsy-3-definitely-all-invalid branch 2 times, most recently from 5d2c247 to e515a28 Compare June 11, 2026 10:19
@joseph-isaacs joseph-isaacs force-pushed the claude/cool-bardeen-l8jlsy-4-lazy-scalarfn-validity branch 2 times, most recently from 9673272 to e9341da Compare June 11, 2026 10:41
@joseph-isaacs joseph-isaacs force-pushed the claude/cool-bardeen-l8jlsy-3-definitely-all-invalid branch 2 times, most recently from e507aa3 to 2d2498f Compare June 11, 2026 10:58
@joseph-isaacs joseph-isaacs force-pushed the claude/cool-bardeen-l8jlsy-4-lazy-scalarfn-validity branch from e9341da to 587baf9 Compare June 11, 2026 10:58
@joseph-isaacs joseph-isaacs marked this pull request as draft June 11, 2026 11:01
@joseph-isaacs joseph-isaacs force-pushed the claude/cool-bardeen-l8jlsy-3-definitely-all-invalid branch from 2d2498f to a13f0ab Compare June 11, 2026 11:01
@joseph-isaacs joseph-isaacs force-pushed the claude/cool-bardeen-l8jlsy-4-lazy-scalarfn-validity branch 2 times, most recently from 6fe8231 to f9b0a16 Compare June 11, 2026 14:12
@joseph-isaacs joseph-isaacs changed the base branch from claude/cool-bardeen-l8jlsy-3-definitely-all-invalid to develop June 16, 2026 10:38
@github-actions

github-actions Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

This PR has been marked as stale because it has been open for 14 days with no activity. Please comment or remove the stale label if you wish to keep it active, otherwise it will be closed in 7 days

@github-actions github-actions Bot added the stale This PR is stale and will be auto-closed soon label Jul 1, 2026
@github-actions

Copy link
Copy Markdown
Contributor

This PR was closed because it has been inactive for 7 days since being marked as stale.

@github-actions github-actions Bot closed this Jul 10, 2026
@robert3005 robert3005 reopened this Jul 10, 2026
@robert3005 robert3005 removed the stale This PR is stale and will be auto-closed soon label Jul 10, 2026
…y expression

Previously ValidityVTable<ScalarFn> always eagerly executed the
validity expression via the legacy session. Now, when the scalar
function provides a validity expression over its inputs, the expression
is converted into a lazy ScalarFn array DAG instead: Literal nodes
become ConstantArrays, ArrayExpr leaves unwrap to the child arrays they
hold, and interior nodes become lazy ScalarFn arrays. Constant results
are folded back into AllValid/AllInvalid via child_to_validity.

Functions that do not define a validity expression (e.g. Kleene logic
and/or, where validity depends on the computed values) keep the eager
path. The erased fallback for these is is_not_null over the expression
itself, so a lazy representation would be self-referential: resolving
the validity of the inner node spawns another is_not_null DAG, which
recurses without ever shrinking (this manifested as a stack overflow in
element-wise execution paths). ScalarFnRef::validity_opt is added to
expose whether a function defines its own validity expression.

https://claude.ai/code/session_01VPQ7dfZtijfrsjAipwXvEj
Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk>
@joseph-isaacs joseph-isaacs force-pushed the claude/cool-bardeen-l8jlsy-4-lazy-scalarfn-validity branch from f9b0a16 to 5a83d1d Compare July 10, 2026 13:30
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.

2 participants