GH-49689: [R][C++] Parquets do not support list-columns of ordered factors (ordered dictionaries) by thisisnic · Pull Request #49937 · apache/arrow

thisisnic · 2026-05-06T12:25:30Z

Rationale for this change

Ordered dictionaries inside nested types lose their ordered flag during construction, because DictionaryBuilder doesn't track it.

What changes are included in this PR?

Store the ordered flag in DictionaryBuilderBase and pass it through when reconstructing the DictionaryType in type() and FinishInternal().

Remove the R-side workaround that was patching this for top-level columns only.

Are these changes tested?

Yes

Are there any user-facing changes?

No

AI usage

Written by Claude, reviewed by me and Codex (via roborev). I had Claude create the tests first, to make sure they failed as expected, then had Claude make the fixes and checked the tests passed. I questioned the approach as I went.

GitHub Issue: [R] Parquets do not support list-columns of ordered factors (ordered dictionaries) #49689

github-actions · 2026-05-06T12:26:04Z

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

thisisnic · 2026-05-07T20:27:59Z

+// GH-49689: Ordered dictionary tests
+
+TEST(TestDictionaryBuilderOrdered, TypePreservesOrderedFlag) {
+  auto ordered_type = dictionary(int8(), utf8(), /*ordered=*/true);
+  std::unique_ptr<ArrayBuilder> boxed_builder;
+  ASSERT_OK(MakeBuilder(default_memory_pool(), ordered_type, &boxed_builder));
+
+  auto builder_type = boxed_builder->type();
+  ASSERT_TRUE(checked_cast<const DictionaryType&>(*builder_type).ordered());
+}
+
+TEST(TestDictionaryBuilderOrdered, UnorderedTypeStaysUnordered) {
+  auto unordered_type = dictionary(int8(), utf8(), /*ordered=*/false);
+  std::unique_ptr<ArrayBuilder> boxed_builder;
+  ASSERT_OK(MakeBuilder(default_memory_pool(), unordered_type, &boxed_builder));
+
+  auto builder_type = boxed_builder->type();
+  ASSERT_FALSE(checked_cast<const DictionaryType&>(*builder_type).ordered());
+}
+
+TEST(TestDictionaryBuilderOrdered, FinishPreservesOrderedFlag) {
+  auto ordered_type = dictionary(int8(), utf8(), /*ordered=*/true);
+  std::unique_ptr<ArrayBuilder> boxed_builder;
+  ASSERT_OK(MakeBuilder(default_memory_pool(), ordered_type, &boxed_builder));
+  auto& builder = checked_cast<DictionaryBuilder<StringType>&>(*boxed_builder);
+
+  ASSERT_OK(builder.Append("a"));
+  ASSERT_OK(builder.Append("b"));
+  ASSERT_OK(builder.Append("a"));
+
+  std::shared_ptr<Array> result;
+  ASSERT_OK(builder.Finish(&result));
+
+  const auto& result_type = checked_cast<const DictionaryType&>(*result->type());
+  ASSERT_TRUE(result_type.ordered());
+
+  auto ex_dict = ArrayFromJSON(utf8(), R"(["a", "b"])");
+  auto ex_indices = ArrayFromJSON(int8(), "[0, 1, 0]");
+  DictionaryArray expected(ordered_type, ex_indices, ex_dict);
+  AssertArraysEqual(expected, *result);
+}
+
+TEST(TestDictionaryBuilderOrdered, ListOfOrderedDictionary) {
+  auto ordered_dict_type = dictionary(int8(), utf8(), /*ordered=*/true);
+  auto list_type = list(field("item", ordered_dict_type));
+
+  std::unique_ptr<ArrayBuilder> boxed_builder;
+  ASSERT_OK(MakeBuilder(default_memory_pool(), list_type, &boxed_builder));
+  auto& list_builder = checked_cast<ListBuilder&>(*boxed_builder);
+  auto& dict_builder =
+      checked_cast<DictionaryBuilder<StringType>&>(*list_builder.value_builder());
+
+  ASSERT_OK(list_builder.Append());
+  ASSERT_OK(dict_builder.Append("a"));
+  ASSERT_OK(dict_builder.Append("b"));
+  ASSERT_OK(list_builder.Append());
+  ASSERT_OK(dict_builder.Append("a"));
+
+  std::shared_ptr<Array> result;
+  ASSERT_OK(list_builder.Finish(&result));
+
+  const auto& result_list_type = checked_cast<const ListType&>(*result->type());
+  const auto& result_dict_type =
+      checked_cast<const DictionaryType&>(*result_list_type.value_type());
+  ASSERT_TRUE(result_dict_type.ordered());
+}
+
+TEST(TestDictionaryBuilderOrdered, MakeDictionaryBuilderPreservesOrdered) {
+  auto ordered_type = dictionary(int8(), utf8(), /*ordered=*/true);
+  std::unique_ptr<ArrayBuilder> builder;
+  ASSERT_OK(MakeDictionaryBuilder(default_memory_pool(), ordered_type,
+                                  /*dictionary=*/nullptr, &builder));
+
+  auto builder_type = builder->type();
+  ASSERT_TRUE(checked_cast<const DictionaryType&>(*builder_type).ordered());
+}
+


This is all AI-generated. I roughly understood what it does but am not confident in it, other than having checked it looks relatively idiomatic, and all of these tests except UnorderedTypeStaysUnordered failed before the PR and passed after.

thisisnic · 2026-05-07T20:34:20Z

  const std::shared_ptr<DataType>& index_type;
  const std::shared_ptr<DataType>& value_type;
-  const std::shared_ptr<Array>& dictionary;
+  std::shared_ptr<Array> dictionary;


This was updated as without it, we ended up with a dangling reference when nullptr is passed in, like when we're creating a builder from a type and don't have a dictionary array to pass in.

thisisnic · 2026-05-07T20:42:33Z

Would you mind giving this a look over some time @pitrou?

I did use Claude and have tried to go through all generated code methodically to understand what it's doing and hopefully make it idiomatic, but let me know if there is anywhere I could have been more diligent in this.

CI failures appear to be ones on main and not related to this PR.

pitrou

Thanks for the PR @thisisnic , here are some comments

pitrou · 2026-05-13T13:13:48Z

+}
+
+TEST(TestDictionaryBuilderOrdered, FinishPreservesOrderedFlag) {
+  auto ordered_type = dictionary(int8(), utf8(), /*ordered=*/true);


You could have a for loop on the ordered flag to test both possibilities.

pitrou · 2026-05-13T13:14:24Z

+}
+
+TEST(TestDictionaryBuilderOrdered, ListOfOrderedDictionary) {
+  auto ordered_dict_type = dictionary(int8(), utf8(), /*ordered=*/true);


pitrou · 2026-05-13T13:23:23Z

+}
+
+TEST(TestDictionaryBuilderOrdered, MakeDictionaryBuilderPreservesOrdered) {
+  auto ordered_type = dictionary(int8(), utf8(), /*ordered=*/true);


pitrou · 2026-05-13T13:23:41Z

+}
+
+TEST(TestDictionaryBuilderOrdered, UnorderedTypeStaysUnordered) {
+  auto unordered_type = dictionary(int8(), utf8(), /*ordered=*/false);


These two tests can be factored into a single one by using a for loop.

pitrou · 2026-05-13T13:25:29Z

        indices_builder_(start_int_size, pool, alignment),
-        value_type_(value_type) {}
+        value_type_(value_type),
+        ordered_(false) {}


Since ordered_ is default-initialized to false in the class declaration (I mean the bool ordered_ = false; part), we don't need to reiterate in the constructors.

pitrou · 2026-05-13T13:34:45Z

+    return ::arrow::dictionary(indices_builder_.type(), null(), ordered_);
  }

+  void set_ordered(bool ordered) { ordered_ = ordered; }


Instead of having this separate method, perhaps we can add a bool ordered = false parameter at the end of DictionaryBuilderBase constructors?

Copilot

Pull request overview

Fixes loss of the ordered flag for dictionary types during array construction (notably for nested/list columns), which caused schema/type mismatches when writing ordered factors (ordered dictionaries) to Parquet from R.

Changes:

Propagate DictionaryType::ordered() into created dictionary builders (MakeBuilder / MakeDictionaryBuilder) and preserve it in type() / FinishInternal().
Remove the R-side post-hoc patch that rewrote the resulting DictionaryType as ordered.
Add C++ and R tests covering ordered dictionaries in both top-level and nested (list/large_list/fixed_size_list) contexts.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
r/tests/testthat/test-Array.R	Adds R regression tests for ordered factors inside list-like nested arrays.
r/src/r_to_arrow.cpp	Removes workaround that manually re-marked finished dictionary arrays as ordered.
cpp/src/arrow/builder.cc	Passes `ordered` through builder creation paths for dictionary types (including nested builder creation).
cpp/src/arrow/array/builder_dict.h	Stores `ordered_` in dictionary builders and uses it when reconstructing `DictionaryType` in `type()` / `FinishInternal()`.
cpp/src/arrow/array/array_dict_test.cc	Adds C++ unit tests ensuring ordered flag is preserved through `MakeBuilder`, `Finish`, and nested list builders.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

stephenashton-dhsc · 2026-06-03T10:36:59Z

I think this has caused an unintended consequence.

The following code runs under the CRAN version of arrow without issue:

library(arrow)
library(dplyr)

starwars |> mutate(sex = factor(sex, ordered = TRUE)) |> write_parquet(sink = tempfile())

However, under this PR, it returns the following error:

Error: Invalid: Column data for field 7 with type dictionary<values=string, indices=int8, ordered=0> is inconsistent with schema dictionary<values=string, indices=int8, ordered=1>

Would it be possible to resolve this?

thisisnic · 2026-06-08T10:35:10Z

I think this has caused an unintended consequence.

The following code runs under the CRAN version of arrow without issue:
library(arrow)
library(dplyr)

starwars |> mutate(sex = factor(sex, ordered = TRUE)) |> write_parquet(sink = tempfile())
However, under this PR, it returns the following error:
Error: Invalid: Column data for field 7 with type dictionary<values=string, indices=int8, ordered=0> is inconsistent with schema dictionary<values=string, indices=int8, ordered=1>
Would it be possible to resolve this?

Thanks for the review @stephenashton-dhsc .Would you mind sharing a bit more about the steps you took to build Arrow to get that error? I tried it myself locally but I can't reproduce your error, so it might be that you had a mismatch between R package and C++ library version, though it'd be good to be sure.

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

stephenashton-dhsc · 2026-06-08T10:42:35Z

I think this has caused an unintended consequence.
The following code runs under the CRAN version of arrow without issue:
library(arrow)
library(dplyr)

starwars |> mutate(sex = factor(sex, ordered = TRUE)) |> write_parquet(sink = tempfile())
However, under this PR, it returns the following error:
Error: Invalid: Column data for field 7 with type dictionary<values=string, indices=int8, ordered=0> is inconsistent with schema dictionary<values=string, indices=int8, ordered=1>
Would it be possible to resolve this?
Thanks for the review @stephenashton-dhsc .Would you mind sharing a bit more about the steps you took to build Arrow to get that error? I tried it myself locally but I can't reproduce your error, so it might be that you had a mismatch between R package and C++ library version, though it'd be good to be sure.

I just did a build/install using pak::pkg_install("apache/arrow/r#49937").

I'll retest under 29dfbc7 now, just in case that has resolved the issue.

thisisnic · 2026-06-08T11:05:50Z

I think this has caused an unintended consequence.
The following code runs under the CRAN version of arrow without issue:
library(arrow)
library(dplyr)

starwars |> mutate(sex = factor(sex, ordered = TRUE)) |> write_parquet(sink = tempfile())
However, under this PR, it returns the following error:
Error: Invalid: Column data for field 7 with type dictionary<values=string, indices=int8, ordered=0> is inconsistent with schema dictionary<values=string, indices=int8, ordered=1>
Would it be possible to resolve this?
Thanks for the review @stephenashton-dhsc .Would you mind sharing a bit more about the steps you took to build Arrow to get that error? I tried it myself locally but I can't reproduce your error, so it might be that you had a mismatch between R package and C++ library version, though it'd be good to be sure.
I just did a build/install using pak::pkg_install("apache/arrow/r#49937").

I'll retest under 29dfbc7 now, just in case that has resolved the issue.

Aha, AFAIK pak::pkg_install() won't rebuild the C++ libraries and will only rebuild the R package but with existing C++ libraries you have installed. If you wanted to test this PR out like that you'd need to do a full rebuild of the Arrow C++ library before installing the R package, as I've modified C++ in this PR. The docs for that are here.

What is it that you're trying to do? If you're trying to use the functionality implemented here, if you wait for the PR to merge, it'll be there in the nightly Arrow binaries later, which you can install using arrow::install_arrow(nightly=TRUE) which is a lot easier than doing a full dev build.

stephenashton-dhsc · 2026-06-08T11:06:34Z

library(arrow)
library(dplyr)

starwars |> mutate(sex = factor(sex, ordered = TRUE)) |> write_parquet(sink = tempfile())

Yep - still getting the same issue:

> pak::pkg_install("apache/arrow/r#49937")

! Using bundled GitHub PAT. Please add your own PAT using `gitcreds::gitcreds_set()`.

 Found  1  deps for  0/1  pkgs [⠋] Resolving apache/arrow/r#49937

ℹ Loading metadata database                                      

✔ Loading metadata database ... done

 Found  36  deps for  1/1  pkgs [⠧] Resolving standard (CRAN/BioC) packages                                                                        
 
→ Package library at 'C:\Users\SAshton\AppData\Local\R\Win-library\4.4'.
→ Will update 1 package.
→ The package (0 B) is cached.
+ arrow 24.0.0.9000 → 24.0.0.9000 [bld][cmp] (GitHub: 29dfbc7)
? Do you want to continue (Y/n) y
ℹ No downloads are needed, 1 pkg is cached
✔ Got arrow 24.0.0.9000 (source) (21.46 MB)
ℹ Packaging arrow 24.0.0.9000               
✔ Installed arrow 24.0.0.9000 (github::apache/arrow@29dfbc7) (407ms)                                                                 
✔ 1 pkg + 14 deps: kept 14, upd 1, dld 1 (NA B) [5m 5.3s]
> library(arrow)

Attaching package: ‘arrow’

The following object is masked from ‘package:utils’:

    timestamp

> library(dplyr)

Attaching package: ‘dplyr’

The following objects are masked from ‘package:stats’:

    filter, lag

The following objects are masked from ‘package:base’:

    intersect, setdiff, setequal, union

Warning message:
package ‘dplyr’ was built under R version 4.4.3 
> starwars |> mutate(sex = factor(sex, ordered = TRUE)) |> write_parquet(sink = tempfile())
Error: Invalid: Column data for field 7 with type dictionary<values=string, indices=int8, ordered=0> is inconsistent with schema dictionary<values=string, indices=int8, ordered=1>

This is the current session info:

> sessionInfo()
R version 4.4.1 (2024-06-14 ucrt)
Platform: x86_64-w64-mingw32/x64
Running under: Windows 11 x64 (build 26200)

Matrix products: default


locale:
[1] LC_COLLATE=English_United Kingdom.utf8  LC_CTYPE=English_United Kingdom.utf8    LC_MONETARY=English_United Kingdom.utf8 LC_NUMERIC=C                            LC_TIME=English_United Kingdom.utf8    

time zone: Europe/London
tzcode source: internal

attached base packages:
[1] stats     graphics  grDevices utils     datasets  methods   base     

other attached packages:
[1] dplyr_1.2.1       arrow_24.0.0.9000

loaded via a namespace (and not attached):
 [1] assertthat_0.2.1 R6_2.6.1         bit_4.6.0        tidyselect_1.2.1 tzdb_0.5.0       magrittr_2.0.5   glue_1.8.1       tibble_3.3.1     pkgconfig_2.0.3  bit64_4.8.2      generics_0.1.4   lifecycle_1.0.5  cli_3.6.6        pak_0.10.0       vctrs_0.7.3      compiler_4.4.1   purrr_1.2.2      pillar_1.11.1    rlang_1.2.0

stephenashton-dhsc · 2026-06-08T11:09:13Z

I think this has caused an unintended consequence.
The following code runs under the CRAN version of arrow without issue:
library(arrow)
library(dplyr)

starwars |> mutate(sex = factor(sex, ordered = TRUE)) |> write_parquet(sink = tempfile())
However, under this PR, it returns the following error:
Error: Invalid: Column data for field 7 with type dictionary<values=string, indices=int8, ordered=0> is inconsistent with schema dictionary<values=string, indices=int8, ordered=1>
Would it be possible to resolve this?
Thanks for the review @stephenashton-dhsc .Would you mind sharing a bit more about the steps you took to build Arrow to get that error? I tried it myself locally but I can't reproduce your error, so it might be that you had a mismatch between R package and C++ library version, though it'd be good to be sure.
I just did a build/install using pak::pkg_install("apache/arrow/r#49937").
I'll retest under 29dfbc7 now, just in case that has resolved the issue.
Aha, AFAIK pak::pkg_install() won't rebuild the C++ libraries and will only rebuild the R package but with existing C++ libraries you have installed. If you wanted to test this PR out like that you'd need to do a full rebuild of the Arrow C++ library before installing the R package, as I've modified C++ in this PR. The docs for that are here.

What is it that you're trying to do? If you're trying to use the functionality implemented here, if you wait for the PR to merge, it'll be there in the nightly Arrow binaries later, which you can install using arrow::install_arrow(nightly=TRUE) which is a lot easier than doing a full dev build.

Ah - my mistake then. Apologies!

pitrou

Looks mostly good to me, but I can't vet the R tests

pitrou · 2026-06-09T13:20:03Z

+    ARROW_SCOPED_TRACE("ordered = ", ordered);
+    auto dict_type = dictionary(int8(), utf8(), ordered);
+    std::unique_ptr<ArrayBuilder> builder;
+    ASSERT_OK(MakeDictionaryBuilder(default_memory_pool(), dict_type,


Same here: use Result-returning variant

Co-authored-by: Antoine Pitrou <pitrou@free.fr>

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

thisisnic · 2026-06-10T10:54:00Z

@jonkeane - Mind giving this a review from the R point of view when you have a moment?

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

jonkeane

Looks good to me. Is there anything we need to update in our docs about this? I can't remember if we list what arrow types are (fully) supported somewhere?

github-actions Bot added Component: R Component: C++ awaiting committer review Awaiting committer review labels May 6, 2026

thisisnic changed the title ~~GH-49689: [R] Parquets do not support list-columns of ordered factors (ordered dictionaries)~~ GH-49689: [R][C++] Parquets do not support list-columns of ordered factors (ordered dictionaries) May 6, 2026

thisisnic commented May 7, 2026

View reviewed changes

github-actions Bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels May 7, 2026

thisisnic commented May 7, 2026

View reviewed changes

thisisnic marked this pull request as ready for review May 7, 2026 20:38

thisisnic requested a review from jonkeane as a code owner May 7, 2026 20:38

thisisnic requested a review from pitrou May 7, 2026 20:42

pitrou reviewed May 13, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings May 28, 2026 09:56

thisisnic force-pushed the GH-49689-ordered branch from bcc614b to f3eb5a0 Compare May 28, 2026 09:56

Copilot started reviewing on behalf of thisisnic May 28, 2026 09:56 View session

github-actions Bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels May 28, 2026

Copilot AI reviewed May 28, 2026

View reviewed changes

thisisnic added 6 commits June 8, 2026 11:36

Add tests for ordered dictionaries

6ff3cc8

Fix failing tests to fail properly

fe18d93

Plumb in changes needed for ordered dictionaries

8583075

Fix dangling reference

1e85543

fix tests

78bbeef

Changes from PR feedback

29dfbc7

Copilot AI review requested due to automatic review settings June 8, 2026 10:38

thisisnic force-pushed the GH-49689-ordered branch from 66d7c53 to 29dfbc7 Compare June 8, 2026 10:38

Copilot started reviewing on behalf of thisisnic June 8, 2026 10:38 View session

Copilot AI reviewed Jun 8, 2026

View reviewed changes

thisisnic requested a review from pitrou June 8, 2026 11:21

pitrou reviewed Jun 9, 2026

View reviewed changes

Update cpp/src/arrow/array/array_dict_test.cc

ac3db34

Co-authored-by: Antoine Pitrou <pitrou@free.fr>

Copilot AI review requested due to automatic review settings June 10, 2026 10:42

Copilot started reviewing on behalf of thisisnic June 10, 2026 10:42 View session

Copilot AI reviewed Jun 10, 2026

View reviewed changes

Comment thread cpp/src/arrow/array/array_dict_test.cc

Update to use Result returning variants

c8ac6ef

fix missing var

09429c4

Copilot AI review requested due to automatic review settings June 10, 2026 12:01

Copilot started reviewing on behalf of thisisnic June 10, 2026 12:01 View session

Copilot AI reviewed Jun 10, 2026

View reviewed changes

jonkeane approved these changes Jun 10, 2026

View reviewed changes

github-actions Bot added awaiting merge Awaiting merge and removed awaiting change review Awaiting change review labels Jun 10, 2026

Conversation

thisisnic commented May 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

AI usage

Uh oh!

github-actions Bot commented May 6, 2026

Uh oh!

thisisnic May 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thisisnic commented May 7, 2026

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

stephenashton-dhsc commented Jun 3, 2026

Uh oh!

thisisnic commented Jun 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

stephenashton-dhsc commented Jun 8, 2026

Uh oh!

thisisnic commented Jun 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stephenashton-dhsc commented Jun 8, 2026

Uh oh!

stephenashton-dhsc commented Jun 8, 2026

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

thisisnic commented Jun 10, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

jonkeane left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

thisisnic commented May 6, 2026 •

edited

Loading

thisisnic May 7, 2026 •

edited

Loading

thisisnic commented Jun 8, 2026 •

edited

Loading

thisisnic commented Jun 8, 2026 •

edited

Loading