镜像站点 · 本页由第三方 GitHub 只读镜像提供,非 GitHub 官方站点,不接受任何登录或凭据输入。前往 github.com
Skip to content

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

Merged
thisisnic merged 5 commits into
apache:mainfrom
thisisnic:GH-50514-enums-new-new
Sep 16, 2026
Merged

thisisnic merged 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

expect_type(result[[2]], "double")
})

test_that("list of dictionary: ptype and elements share the dictionary's levels (GH-50514)", {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Whoa these are finnicky. I vaguely remember trying to get something that would make this work years ago, but the C++ was too much for me at the time. Glad the funky data structure which should probably never happen is supported!

@github-actions github-actions Bot added awaiting merge Awaiting merge and removed awaiting committer review Awaiting committer review labels Sep 16, 2026
@thisisnic
thisisnic merged commit 8b617fe into apache:main Sep 16, 2026
36 checks passed
@thisisnic thisisnic removed the awaiting merge Awaiting merge label Sep 16, 2026
pitrou pushed a commit to Alex-PLACET/arrow that referenced this pull request Sep 17, 2026
… inside a Struct array across record batches (apache#51153)

### 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.

* GitHub Issue: apache#50514

Authored-by: Nic Crane <thisisnic@gmail.com>
Signed-off-by: Nic Crane <thisisnic@gmail.com>
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.

3 participants