Skip to content

GH-50514: [R] read_ipc_stream fails to unify nested Enum fields inside a Struct array across record batches - #51153

Open
thisisnic wants to merge 5 commits into
apache:mainfrom
thisisnic:GH-50514-enums-new-new
Open

GH-50514: [R] read_ipc_stream fails to unify nested Enum fields inside a Struct array across record batches#51153
thisisnic wants to merge 5 commits into
apache:mainfrom
thisisnic:GH-50514-enums-new-new

Conversation

@thisisnic

@thisisnic thisisnic commented Sep 3, 2026

Copy link
Copy Markdown
Member

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_of ptype, 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.

Copilot AI lite review requested due to automatic review settings September 3, 2026 13:27
@thisisnic
thisisnic requested a review from jonkeane as a code owner September 3, 2026 13:27
@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown

⚠️ GitHub issue #50514 has been automatically assigned in GitHub to PR creator.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 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 from ptype.
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.

@thisisnic
thisisnic force-pushed the GH-50514-enums-new-new branch from ea9cb5a to ac1dae3 Compare September 3, 2026 15:02
@thisisnic
thisisnic marked this pull request as ready for review September 4, 2026 10:16
Copilot AI review requested due to automatic review settings September 4, 2026 10:16

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants