GH-50514: [R] read_ipc_stream fails to unify nested Enum fields inside a Struct array across record batches - #51153
GH-50514: [R] read_ipc_stream fails to unify nested Enum fields inside a Struct array across record batches#51153thisisnic wants to merge 5 commits into
Conversation
|
|
There was a problem hiding this comment.
🟢 Approval recommended
The changes are coherent, well-tested across the reported edge cases (nested dictionaries/structs/chunks/slicing/empty), and the implementation matches the stated rationale without introducing obvious correctness or API risks.
Pull request overview
Fixes inconsistent R type decisions when converting Arrow list-like columns (including nested dictionaries/factors) by unifying conversion decisions across all chunks/values, preventing malformed factors during downstream operations like tidyr::unnest() (GH-50514).
Changes:
- Build one values-level converter per list-like chunked array (list/large_list/fixed_size_list/map) so factor levels and numeric sizing decisions are made once for the whole column.
- Add slice-aware allocation/conversion hooks to avoid altrep vectors that incorrectly shadow the full chunked array when converting list elements (notably for structs and extension types).
- Add targeted tests + user-facing documentation/NEWS updates; fix a missing
return()in list type inference fromptype.
File summaries
| File | Description |
|---|---|
| r/tests/testthat/test-chunked-array.R | Adds regression test ensuring chunked list conversion unifies nested factor levels. |
| r/tests/testthat/test-Array.R | Adds comprehensive tests for list-like conversions across chunks (dictionary/ordered/struct nesting/int64+uint32 behavior/extension types/empty cases/slicing). |
| r/src/type_infer.cpp | Fixes missing return when inferring list() type from attr(x, "ptype"). |
| r/src/array_to_vector.cpp | Refactors list-like converters to reuse a unified values converter; introduces AllocateSlice()/ConvertSlice() to ensure correct slice semantics and ptype derivation. |
| r/R/type.R | Documents that int64/uint32 sizing decisions inside lists are made once per column. |
| r/NEWS.md | Notes the user-facing behavior change for list-column element type consistency (factors/integers) and references #50514. |
| r/man/data-type.Rd | Updates rendered documentation to match the new list-nested sizing behavior. |
Review details
Files not reviewed (1)
- r/man/data-type.Rd: Generated file
- Files reviewed: 6/7 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
ea9cb5a to
ac1dae3
Compare
There was a problem hiding this comment.
🟢 Approval recommended
The changes are well-scoped to R conversion internals, and the PR adds comprehensive regression tests covering the reported failure mode and key edge cases (chunks, nesting, slicing, extension types, and empty/null inputs).
Review details
Files not reviewed (1)
- r/man/data-type.Rd: Generated file
- Files reviewed: 6/7 changed files
- Comments generated: 0 new
- Review effort level: Lite
Rationale for this change
When converting list columns to R, each list element was converted independently, so decisions like factor levels (and uint32/int64 integer-vs-double sizing) could differ between elements and from the column's
vctrs_list_ofptype, producing malformed factors when unnesting.What changes are included in this PR?
List, large list, fixed-size list, and map converters now build one converter over the values of all chunks, so factor levels are unified and integer sizing is decided once for the whole column; the ptype is derived from that converter instead of an empty array. Also fixes a missing
return()when inferring a list type from a ptype.Are these changes tested?
Yes, new tests covering dictionaries (including ordered, nested in structs, and across chunks), int64/uint32 consistency, sliced arrays, extension types, and empty/all-null columns.
Are there any user-facing changes?
Yes: elements of converted list columns now all share one R type (e.g. factors with unified levels), documented in NEWS.md.