Refactor validity checks to require explicit ExecutionCtx#8273
Merged
Conversation
`Validity::is_valid`/`is_null` previously created an internal `LEGACY_SESSION` execution context to evaluate the validity array. Replace them with `execute_is_valid`/`execute_is_null` that accept an `&mut ExecutionCtx` from the caller, threading the context through where one is available and falling back to a `LEGACY_SESSION` context only at boundaries that do not yet have one. Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk>
Keep the original ctx-free `is_valid`/`is_null` methods for backward compatibility, marked `#[deprecated]`, delegating to the new `execute_is_valid`/`execute_is_null` via a `LEGACY_SESSION` context. Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk>
joseph-isaacs
added
the
changelog/deprecation
Claude
Merging this PR will not alter performance
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Simulation | compare[29] |
152.5 µs | 205.1 µs | -25.66% |
| ❌ | Simulation | varbinview_zip_block_mask |
2.9 ms | 3.7 ms | -21.56% |
| ❌ | Simulation | varbinview_zip_fragmented_mask |
6.1 ms | 6.9 ms | -11.3% |
| ⚡ | Simulation | compare[31] |
212.7 µs | 157.1 µs | +35.46% |
| ⚡ | Simulation | rebuild_naive |
141.9 µs | 113.4 µs | +25.14% |
| ⚡ | Simulation | extend_from_array_non_zctl_overlapping[(10000, 8)] |
4.7 ms | 4 ms | +18.44% |
| ⚡ | WallTime | cuda/bitpacked_u8/unpack/3bw[100M] |
352.6 µs | 300 µs | +17.56% |
| ⚡ | Simulation | extend_from_array_non_zctl_overlapping[(1000, 8)] |
527.9 µs | 453.7 µs | +16.36% |
| ⚡ | Simulation | extend_from_array_zctl[(10000, 8)] |
2.5 ms | 2.2 ms | +12.73% |
| ⚡ | Simulation | extend_from_array_zctl[(1000, 8)] |
291 µs | 263 µs | +10.64% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing claude/eager-cerf-Sj3pp (803380c) with develop (9a7d966)
Consolidate the validity/scalar checks in the tests updated by the `execute_is_valid` migration so each test creates one execution context and reuses it, instead of constructing a fresh `LEGACY_SESSION` context per call (some inside loops). Also unify the ALP sliced-vector test on a single `SESSION` context for both encode and execute, rather than mixing `SESSION` and `LEGACY_SESSION`. Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk>
…ation `VarBinViewArray`/`VarBinArray` validation only needs an execution context when validity is array-backed. Previously `is_null` short-circuited for the `NonNullable`/`AllValid` variants without ever creating a context, but the ctx-threaded version constructed a `LEGACY_SESSION` context unconditionally, adding overhead to the common non-null construction path (observed as a regression in the `varbinview_zip` benchmarks). Construct the context lazily, only for `Validity::Array`, and resolve the constant variants without one. The UTF-8 validation loop is extracted into `VarBinArray::validate_utf8` with an `is_null_at` helper to keep the function within the cognitive-complexity budget. Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk>
AdamGS
reviewed
Jun 8, 2026
| if validity.is_null(idx)? { | ||
| let is_null = match ctx.as_mut() { | ||
| Some(ctx) => validity.execute_is_null(idx, ctx)?, | ||
| None => matches!(validity, Validity::AllInvalid), |
Contributor
There was a problem hiding this comment.
If its AllInvalid we can just break
The reworked `validate_utf8` tripped two clippy lints: - `cognitive_complexity`: the per-validity-variant `match` lived inside the `match_each_integer_ptype!` macro, multiplying its complexity across every integer arm. Resolve the validity mask once before the dtype dispatch and keep a single loop inside the macro. - `unnecessary_fallible_conversions`: replace `usize::try_from(..)` on the offset with the infallible `AsPrimitive::as_()` already used for the window bounds. Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk>
AdamGS
reviewed
Jun 9, 2026
| .is_valid(index) | ||
| .unwrap_or(false) | ||
| ); | ||
| #[expect(clippy::debug_assert_with_mut_call)] |
AdamGS
approved these changes
Jun 9, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR refactors the
ValidityAPI to require an explicitExecutionCtxparameter for validity checks, improving execution context management and making it explicit where execution happens.Changes
New execution-aware methods in
Validity:execute_is_valid(index, ctx)- checks if an element is valid using the provided contextexecute_is_null(index, ctx)- checks if an element is null using the provided contextDeprecated legacy methods:
is_valid(index)as deprecated in favor ofexecute_is_validis_null(index)as deprecated in favor ofexecute_is_nullLEGACY_SESSION