Faster iterations#15
Merged
Merged
Conversation
Reading a validated column is the hot path: the bounds are known once (the column length, or a list element's offset range), so re-checking each element through arrow's `value()` is wasted work. Add `LogicalType::value_unchecked` (safe default = `value`), overridden to call arrow's unchecked accessor for the leaf encodings, the trivial wrappers (`Option`, newtypes, `As`, duration, timestamp), and `List`/`LargeList`. `Dictionary` keeps its value lookup checked, so a malformed key can't cause UB; `Run` forwards. `Column` (via `TypedArray::value_unchecked`) and `ListValue` now read through it: `get`/`value` bounds-check once, and the iterators read unchecked over their validated range. The iterators also gain `count`/`last`/`nth`/`fold`, `DoubleEndedIterator`, and `FusedIterator`. The workspace `unsafe_code` lint moves from `deny` to `warn`, and `quiver_types` opts in with a crate-level `expect` documenting the one use. Verified with Miri (no UB). Also move `ListValue` to its own `list_value` module. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR optimizes quiver’s validated read hot paths by introducing LogicalType::value_unchecked and routing Column/ListValue iteration through unchecked Arrow accessors after a single upfront bounds check, while also expanding iterator ergonomics and adding a CI Miri soundness job.
Changes:
- Add
LogicalType::value_unchecked(defaulting tovalue) and implement it for key logical types to use Arrow unchecked accessors where sound. - Update
TypedArray,Columniterators, andListValueto do one bounds check up front and then iterate via unchecked reads; addDoubleEndedIterator/FusedIteratorand optimized combinators. - Add a Miri CI job and adjust workspace
unsafe_codelinting; moveListValueinto a dedicated module and update exports.
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Remove “unsafe forbidden” badge to reflect new unsafe usage. |
| Cargo.toml | Change workspace unsafe_code lint level to warn. |
| .github/workflows/rust.yml | Add nightly Miri job for soundness validation. |
| crates/quiver/tests/column.rs | Add tests for new iterator combinators and nested list iteration. |
| crates/quiver_types/src/datatype.rs | Add LogicalType::value_unchecked and implement it for flat/marker types. |
| crates/quiver_types/src/typed_array.rs | Route get/value through unchecked access after a single bounds check. |
| crates/quiver_types/src/column.rs | Optimize iterators to use unchecked reads; add combinators + DoubleEndedIterator/FusedIterator. |
| crates/quiver_types/src/list.rs | Add list value_unchecked using unchecked offset access; re-export ListValue. |
| crates/quiver_types/src/list_value.rs | New module for ListValue, updated to iterate via unchecked reads and provide more iterator traits. |
| crates/quiver_types/src/lib.rs | Add crate-level expect(unsafe_code) and re-export ListValue from new module. |
| crates/quiver_types/src/option.rs | Implement value_unchecked for Option<L>. |
| crates/quiver_types/src/newtype.rs | Forward value_unchecked through newtype wrappers. |
| crates/quiver_types/src/duration.rs | Forward value_unchecked to Arrow unchecked accessor. |
| crates/quiver_types/src/timestamp.rs | Forward value_unchecked to Arrow unchecked accessor. |
| crates/quiver_types/src/string.rs | Implement unchecked access for AnyUtf8 encodings. |
| crates/quiver_types/src/binary.rs | Implement unchecked access for binary encodings and AnyBinary. |
| crates/quiver_types/src/fixed_size_binary.rs | Implement unchecked access for fixed-size binary. |
| crates/quiver_types/src/dictionary.rs | Implement unchecked key read but keep checked value lookup to avoid UB on malformed keys. |
| crates/quiver_types/src/run.rs | Forward unchecked reads through run-end mapping. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Address Copilot review on PR #15: - Reword the crate-level `unsafe_code` rationale and `value_unchecked` safety doc to state the precondition is `index < length`, with arrow's buffer/offset invariants resting on construction (quiver validates datatype and nullability, not those internal invariants). - Add `debug_assert!(index <= end)` to `ListValue::new` so an inverted range is caught in tests and under Miri before it can make safe iteration unsound. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
emilk
added a commit
that referenced
this pull request
Jun 12, 2026
…rs (#17) 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.
Reading a validated column is the hot path: the bounds are known once
(the column length, or a list element's offset range), so re-checking
each element through arrow's
value()is wasted work.Add
LogicalType::value_unchecked(safe default =value), overriddento call arrow's unchecked accessor for the leaf encodings, the trivial
wrappers (
Option, newtypes,As, duration, timestamp), andList/LargeList.Dictionarykeeps its value lookup checked, so amalformed key can't cause UB;
Runforwards.Column(viaTypedArray::value_unchecked) andListValuenow readthrough it:
get/valuebounds-check once, and the iterators readunchecked over their validated range. The iterators also gain
count/last/nth/fold,DoubleEndedIterator, andFusedIterator.The workspace
unsafe_codelint moves fromdenytowarn, andquiver_typesopts in with a crate-levelexpectdocumenting the oneuse. Verified with Miri (no UB).
Also move
ListValueto its ownlist_valuemodule.Co-Authored-By: Claude Opus 4.8 (1M context) noreply@anthropic.com