ColumnDesc<L> instead of ColumnDesc<Column<L>> - #24
Merged
Conversation
emilk
commented
Aug 27, 2026
emilk
commented
Aug 27, 2026
emilk
marked this pull request as ready for review
August 27, 2026 14:40
Open
emilk
force-pushed
the
emilk/public-typed-array
branch
from
August 27, 2026 15:19
9891fc6 to
f99b038
Compare
The descriptor now names the logical type, which reads better and is the honest parameter: a descriptor yields both a `Column<L>` (`extract`) and a `TypedArray<L>` (`typed_array`), so the wrapper type was never the thing it was parameterized by. The derive used to paste the field type verbatim; it now pulls `L` out of the `Column<L>` path segment, and says so when the parameter is missing.⚠️ Breaking: annotations spell `ColumnDesc<Utf8>` instead of `ColumnDesc<Column<Utf8>>`. No transitional alias is possible — `impl<L: LogicalType> ColumnDesc<L>` and `impl<L> ColumnDesc<Column<L>>` overlap as far as coherence can tell. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`ColumnKind::Wrapper` keeps only the logical type; the three sites that want the column type spell `#krate::Column<#logical_type>`, which also makes the generated code independent of how the field's type was written. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Most columns declare no metadata, so `&[]` was noise at every call site. The derive picks the short constructor unless the field declares metadata. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
emilk
force-pushed
the
emilk/column-desc-logical-type
branch
from
August 27, 2026 15:21
0e14673 to
b30a2fd
Compare
emilk
added a commit
that referenced
this pull request
Aug 27, 2026
* Make `TypedArray` public `TypedArray<L>` is the data half of a `Column<L>`: the validated, downcast arrow array without the per-column metadata. It was crate-internal, so an array that isn't a record batch column had to be wrapped in a `Column` with an empty metadata map. The value API now lives on `TypedArray` and `Column` delegates to it: - the iterators moved over, and are renamed `TypedArrayIter` / `TypedArrayIntoIter` (they are what `Column::iter` returns too) - constructors (`from_values`, `from_nullable_values`, `Default`, `From<Vec>`, `FromIterator`), `iter`, `to_vec`, `slice`, `Index`, and `as_slice` - `Column::as_typed_array` / `into_typed_array`, and `From<TypedArray>` Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Add `ColumnDesc::typed_array` Validates a loose `ArrayRef` against the column's logical type, without needing to name that type, and labels errors with the column and record type. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Test and document the standalone `TypedArray` Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Keep `ColumnIter` / `ColumnIntoIter` as deprecated aliases The rename to `TypedArrayIter` / `TypedArrayIntoIter` no longer breaks downstream code: the old names stay as deprecated type aliases, so this can go out in a minor release. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Link the main types from the crate docs, and to each other The crate-level docs (the README for `quiver`, the module docs for `quiver_types`) now open with `Column`, `TypedArray`, and `ColumnDesc`, and each of the three explains its relationship to the other two. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Fix redundant explicit link targets in `ColumnDesc` docs `cargo doc --document-private-items` resolves `[`DynColumn`]` on its own, so the explicit `(crate::DynColumn)` target is an error under `-D warnings`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Document that `ColumnDesc` works standalone The docs read as if a descriptor only ever came out of `#[derive(Quiver)]`. `ColumnDesc::new` is a `const fn`, so you can declare your own; say so, with a doctest, and stop describing the fields as derive-only. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Drop the now-redundant explicit link target on `Field` `Column::into_dyn` imports `Field`, so the explicit `arrow::datatypes::Field` target makes `rustdoc::redundant_explicit_links` fire under `cargo doc --document-private-items`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * `ColumnDesc<L>` instead of `ColumnDesc<Column<L>>` (#24) * `ColumnDesc<L>`, not `ColumnDesc<Column<L>>` The descriptor now names the logical type, which reads better and is the honest parameter: a descriptor yields both a `Column<L>` (`extract`) and a `TypedArray<L>` (`typed_array`), so the wrapper type was never the thing it was parameterized by. The derive used to paste the field type verbatim; it now pulls `L` out of the `Column<L>` path segment, and says so when the parameter is missing.⚠️ Breaking: annotations spell `ColumnDesc<Utf8>` instead of `ColumnDesc<Column<Utf8>>`. No transitional alias is possible — `impl<L: LogicalType> ColumnDesc<L>` and `impl<L> ColumnDesc<Column<L>>` overlap as far as coherence can tell. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Test and document `ColumnDesc<L>` Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Build `Column<L>` on demand in the derive `ColumnKind::Wrapper` keeps only the logical type; the three sites that want the column type spell `#krate::Column<#logical_type>`, which also makes the generated code independent of how the field's type was written. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Split `ColumnDesc::new` and `new_with_metadata` Most columns declare no metadata, so `&[]` was noise at every call site. The derive picks the short constructor unless the field declares metadata. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Stop mentioning the old `ColumnDesc<Column<L>>` spelling Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Match `ColumnKind::Wrapper` exhaustively Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Drop a redundant comment in the `ColumnDesc<L>` test Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
emilk
commented
Aug 27, 2026
Member
Author
There was a problem hiding this comment.
Post-merge review, as part of reviewing the 0.6 stack for #30. Clippy and tests are clean at the tip of release-0.6, so everything below is a follow-up. ColumnDesc<L> is the right shape: the descriptor names the one thing it is parameterized by, and stops leaking Column into something that can also produce a TypedArray.
The L: LogicalType bound moved onto the struct definition. The struct holds no L, so only the impls need it. A bound on the data structure propagates outward: every user who writes struct MySchema<L> { desc: ColumnDesc<L> } must repeat it, and #32's hand-written Clone/Copy/Debug/PartialEq have to carry L: LogicalType even though none of those traits needs anything of L. Dropping it costs one where clause per impl block, which column_desc.rs already writes everywhere. Hard to change later.
Compatibility section missing on a breaking PR. This changes ColumnDesc's parameter and ColumnDesc::new's arity, and because it merged into #23's branch it never appears in git log on release-0.6 — the body was the only durable record of the break, and it says "Short and sweet!".
Minor: logical_type_of_column filters generic arguments down to types, so Column<'a, Utf8> extracts Utf8 and generates; rustc's E0107 then rejects the field, so the user reads rustc's error rather than the derive's. Checking args.len() != 1 up front puts your message first.
— Claude
This was referenced Aug 27, 2026
Merged
emilk
added a commit
that referenced
this pull request
Aug 28, 2026
* Add the 0.6.0 changelog Generated with `./scripts/generate_changelog.py --version 0.6.0`, then sorted into sections per `RELEASES.md`. Two hand edits the generator cannot make: * #24 is added by hand — it was merged into #23's branch before #23 landed, so it never appears in this branch's history. * #48 is listed under the name the release ships (`data_type()`), not the `datatype()` its PR title used before #50 renamed it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * nicer changelog --------- Co-authored-by: Claude Opus 5 (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
1 participant
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.
Short and sweet!