Repository navigation
Peel Arrow dictionaries before Arrow importer plugins run - #11185
Merged
Merged
Conversation
Arrow importers of known types are about to fail on data outside their type instead of declining it. A dictionary-encoded extension column imports today only because its importer declines the dictionary type and the session retries with the value type. So the session now unwraps a dictionary field or array before it calls any importer. A plugin only ever sees the value field and the values array, and the result is still a Dict over the imported values. The spatial importers' name-only fallback used to accept the dictionary field itself, with default metadata. A dictionary-encoded GeoArrow field now keeps its CRS in the schema, which matches what the array import already produced. Signed-off-by: Connor Tsui <[email protected]>
connortsui20
marked this pull request as ready for review
November 3, 2026 22:52
connortsui20
changed the title
connortsui20
enabled auto-merge (squash)
November 3, 2026 22:57
12 of 49 tasks
robert3005
reviewed
Nov 4, 2026
| /// fields is preserved. | ||
| #[expect(clippy::disallowed_methods, reason = "interning a dynamic id")] | ||
| pub fn from_arrow_field(&self, field: &Field) -> VortexResult<DType> { | ||
| if let DataType::Dictionary(_, value_type) = field.data_type() { |
Contributor
There was a problem hiding this comment.
do you need the same for runend?
Member
Author
There was a problem hiding this comment.
So apparently the answer (crazily) is no:
Dictionary: Field "s" { type: Dictionary(Int32, Utf8), metadata: {ARROW:extension:name: ...} }
^ the values are a bare DataType: no Field, no metadata
RunEnd: Field "s" { type: RunEndEncoded(
Field "run_ends" { Int32 },
Field "values" { Utf8, metadata: {ARROW:extension:name: ...} } ) }
I am investigating still just to make sure
connortsui20
added a commit
that referenced
this pull request
Nov 4, 2026
TS (#10953) merged on 2026-11-03, and SN1 (#11183) and V10 (#11187) merged on 2026-11-04. The dependency graph marks the three merged and moves its state date to 2026-11-04. It gives SD, DP, FD and U9 their open PRs (#11184, #11185, #11186 and #11188), and it drops #10408, which closed without a merge. The text of parts 4 and 5 and the Fix lines of EXT-V9, EXT-V10, EXT-N2 and EXT-S3 now record the same state. Also record that AX, DS and SN2 are implemented on branches without a PR, and add the approved one-pass design for the masked nulls of SN2. Part 2 and decision X31 also record the fixes: #10953 made every i64 legal, #11183 made the spatial validators reject nullable nested levels, #11187 gave the Date display its fallback, and #10408 closed. Signed-off-by: Connor Tsui <[email protected]>
robert3005
approved these changes
Nov 4, 2026
connortsui20
added a commit
that referenced
this pull request
Nov 4, 2026
DP (#11185) merged on 2026-11-04. SN2, AX and DS are open drafts as Signed-off-by: Connor Tsui <[email protected]> #11223, #11224 and #11225, so the graph and parts 4 and 5 link them.
connortsui20
added a commit
that referenced
this pull request
Nov 4, 2026
…1233) ## Summary - Tracking Issue: #11114 Follow-up to [the dictionary and run-end discussion in #11185](#11185 (comment)). ## Changes Explains why dictionary values need the outer field's extension metadata before importer dispatch, while run-end values have their own field and use normal nested-field imports. Documentation only. Tests, linting, and formatting were not run. Local commit and push hooks were skipped. Signed-off-by: Connor Tsui <[email protected]>
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.
Summary
A dictionary-encoded extension column imports today only because its importer declines the
Dictionarytype, after whichArrowSessionretries with the value type. The next PR in this stack (SN2 in #11114) makes the importers of known types fail on Arrow input outside their type instead of declining it, which would break these columns. So the session now peels the dictionary before any importer runs. The result is still aDictover the imported values, so nothing is decompressed.Changes
The existing dictionary test now also runs with an importer that fails on any dictionary.
API Changes
ArrowImportVTableimplementations no longer receiveDictionaryfields or arrays. They get the value field, with the dictionary field's metadata, and the values array.Note that a dictionary-encoded GeoArrow field with a CRS now keeps its CRS in the dtype from
ArrowSession::from_arrow_fieldandfrom_arrow_schema, which matches what the array import already produced. Before, the name-only fallback of the spatial importers accepted the dictionary field itself and used default metadata.