Faster reads: unchecked AnyList, unchecked null probe, inline accessors#17
Merged
Conversation
Three hot-path follow-ups to PR #15 (`value_unchecked` iteration): 1. `AnyList` gained `value_unchecked` / `is_null_unchecked`. It previously inherited the trait default (which forwards to the bounds-checked `value`), so every element of a `Column<AnyList<_>>` re-checked bounds despite the range being validated once at construction — exactly the cost #15 set out to remove, but missed for the any-encoding list type. 2. New `LogicalType::is_null_unchecked` (safe default = `is_null`). The `Option<L>` hot path read the value unchecked but still probed nullity through the bounds-checked `is_null`, so nullable iteration kept paying a per-element check on the validity bitmap. A single `leaf_is_null_unchecked` helper reads the bitmap via `nulls().inner().value_unchecked()`; the leaf encodings override it and the trivial wrappers (`Option`, newtypes, `As`, duration, timestamp, `Run`, `AnyList`) forward it. `Dictionary` keeps its value lookup checked (a malformed key must not be UB). 3. Added `#[inline]` to the per-element accessors (`is_null`, `is_null_unchecked`, `value`, `value_unchecked`, `value_ref`, `values`), the `TypedArray` accessors, and the `Column`/`ListValue` iterator `next` — tiny generic forwarders that cross the `quiver_types` -> `quiver` crate boundary, where cross-crate inlining of the wrapper chains is not guaranteed. The crate had no `#[inline]` at all before. Soundness tests extended over sliced arrays for the new paths (`nullable_string`, `any_list`, `nullable_any_list`); verified with Miri (no UB). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
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.
Three hot-path follow-ups to #15 (
value_uncheckediteration). Reading a validated column is the hot path — bounds are known once, so per-element re-checks are wasted work.Changes
1.
AnyListunchecked reads (any_list.rs)AnyListonly implementedvalue, so it inherited the trait's defaultvalue_unchecked(which forwards to the bounds-checkedvalue). Every element of aColumn<AnyList<_>>therefore re-checked bounds — the exact cost #15 removed elsewhere, but missed for the any-encoding list type. Addedvalue_unchecked+is_null_uncheckeddispatching to each concrete encoding.2. Unchecked null probe (
datatype.rs,option.rs, leaves)New
LogicalType::is_null_unchecked(safe default =is_null).Option<L>::value_uncheckedread the value unchecked but still probed nullity through the bounds-checkedis_null, so nullable iteration kept paying a per-element check on the validity bitmap. One helperleaf_is_null_uncheckedreads the bitmap vianulls().inner().value_unchecked(); leaf encodings override it, trivial wrappers (Option, newtypes,As, duration, timestamp,Run,AnyList) forward it.Dictionarykeeps its value lookup checked — a malformed key must not be UB.3.
#[inline]sweepThe crate had zero
#[inline]. Added it to the per-element accessors (is_null,is_null_unchecked,value,value_unchecked,value_ref,values),TypedArrayaccessors, and theColumn/ListValueiteratornext— tiny generic forwarders crossing thequiver_types→quivercrate boundary, where inlining the wrapper chains is not guaranteed.Safety / testing
is_null_uncheckedisunsafe(preconditionindex < length), covered by the existing crate-levelunsafe_coderationale (updated inlib.rs).nullable_string_column_sliced,any_list_column_sliced,nullable_any_list_column_sliced.cargo clippy --all-features --all-targetsclean, full test suite green, Miri clean (no UB).🤖 Generated with Claude Code