Skip to content

Commit a1f569c

Browse files
thisisnicpitrou
thisisnic
and
pitrou
authored
GH-49689: [R][C++] Parquets do not support list-columns of ordered factors (ordered dictionaries) (#49937)
### 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](https://github.com/roborev-dev/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: #49689 Lead-authored-by: Nic Crane <thisisnic@gmail.com> Co-authored-by: Antoine Pitrou <pitrou@free.fr> Signed-off-by: Nic Crane <thisisnic@gmail.com>
1 parent ed536d1 commit a1f569c

5 files changed

Lines changed: 176 additions & 45 deletions

File tree

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

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1761,4 +1761,80 @@ TEST(TestDictionaryUnifier, TableZeroColumns) {
17611761
AssertTablesEqual(*table, *unified);
17621762
}
17631763

1764+
// GH-49689: Ordered dictionary tests
1765+
1766+
TEST(TestDictionaryBuilderOrdered, TypePreservesOrderedFlag) {
1767+
for (bool ordered : {true, false}) {
1768+
ARROW_SCOPED_TRACE("ordered = ", ordered);
1769+
auto dict_type = dictionary(int8(), utf8(), ordered);
1770+
ASSERT_OK_AND_ASSIGN(auto boxed_builder, MakeBuilder(dict_type));
1771+
1772+
auto builder_type = boxed_builder->type();
1773+
ASSERT_EQ(checked_cast<const DictionaryType&>(*builder_type).ordered(), ordered);
1774+
}
1775+
}
1776+
1777+
TEST(TestDictionaryBuilderOrdered, FinishPreservesOrderedFlag) {
1778+
for (bool ordered : {true, false}) {
1779+
ARROW_SCOPED_TRACE("ordered = ", ordered);
1780+
auto dict_type = dictionary(int8(), utf8(), ordered);
1781+
ASSERT_OK_AND_ASSIGN(auto boxed_builder, MakeBuilder(dict_type));
1782+
auto& builder = checked_cast<DictionaryBuilder<StringType>&>(*boxed_builder);
1783+
1784+
ASSERT_OK(builder.Append("a"));
1785+
ASSERT_OK(builder.Append("b"));
1786+
ASSERT_OK(builder.Append("a"));
1787+
1788+
std::shared_ptr<Array> result;
1789+
ASSERT_OK(builder.Finish(&result));
1790+
1791+
const auto& result_type = checked_cast<const DictionaryType&>(*result->type());
1792+
ASSERT_EQ(result_type.ordered(), ordered);
1793+
1794+
auto ex_dict = ArrayFromJSON(utf8(), R"(["a", "b"])");
1795+
auto ex_indices = ArrayFromJSON(int8(), "[0, 1, 0]");
1796+
DictionaryArray expected(dict_type, ex_indices, ex_dict);
1797+
AssertArraysEqual(expected, *result);
1798+
}
1799+
}
1800+
1801+
TEST(TestDictionaryBuilderOrdered, ListOfOrderedDictionary) {
1802+
for (bool ordered : {true, false}) {
1803+
ARROW_SCOPED_TRACE("ordered = ", ordered);
1804+
auto dict_type = dictionary(int8(), utf8(), ordered);
1805+
auto list_type = list(field("item", dict_type));
1806+
1807+
ASSERT_OK_AND_ASSIGN(auto boxed_builder, MakeBuilder(list_type));
1808+
auto& list_builder = checked_cast<ListBuilder&>(*boxed_builder);
1809+
auto& dict_builder =
1810+
checked_cast<DictionaryBuilder<StringType>&>(*list_builder.value_builder());
1811+
1812+
ASSERT_OK(list_builder.Append());
1813+
ASSERT_OK(dict_builder.Append("a"));
1814+
ASSERT_OK(dict_builder.Append("b"));
1815+
ASSERT_OK(list_builder.Append());
1816+
ASSERT_OK(dict_builder.Append("a"));
1817+
1818+
std::shared_ptr<Array> result;
1819+
ASSERT_OK(list_builder.Finish(&result));
1820+
1821+
const auto& result_list_type = checked_cast<const ListType&>(*result->type());
1822+
const auto& result_dict_type =
1823+
checked_cast<const DictionaryType&>(*result_list_type.value_type());
1824+
ASSERT_EQ(result_dict_type.ordered(), ordered);
1825+
}
1826+
}
1827+
1828+
TEST(TestDictionaryBuilderOrdered, MakeDictionaryBuilderPreservesOrdered) {
1829+
for (bool ordered : {true, false}) {
1830+
ARROW_SCOPED_TRACE("ordered = ", ordered);
1831+
auto dict_type = dictionary(int8(), utf8(), ordered);
1832+
ASSERT_OK_AND_ASSIGN(auto builder,
1833+
MakeDictionaryBuilder(dict_type, /*dictionary=*/nullptr));
1834+
1835+
auto builder_type = builder->type();
1836+
ASSERT_EQ(checked_cast<const DictionaryType&>(*builder_type).ordered(), ordered);
1837+
}
1838+
}
1839+
17641840
} // namespace arrow

‎cpp/src/arrow/array/builder_dict.h‎

Lines changed: 61 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -154,40 +154,43 @@ class DictionaryBuilderBase : public ArrayBuilder {
154154
const std::shared_ptr<DataType>&>
155155
value_type,
156156
MemoryPool* pool = default_memory_pool(),
157-
int64_t alignment = kDefaultBufferAlignment)
157+
int64_t alignment = kDefaultBufferAlignment, bool ordered = false)
158158
: ArrayBuilder(pool, alignment),
159159
memo_table_(new internal::DictionaryMemoTable(pool, value_type)),
160160
delta_offset_(0),
161161
byte_width_(-1),
162162
indices_builder_(start_int_size, pool, alignment),
163-
value_type_(value_type) {}
163+
value_type_(value_type),
164+
ordered_(ordered) {}
164165

165166
template <typename T1 = T>
166167
explicit DictionaryBuilderBase(
167168
enable_if_t<!is_fixed_size_binary_type<T1>::value, const std::shared_ptr<DataType>&>
168169
value_type,
169170
MemoryPool* pool = default_memory_pool(),
170-
int64_t alignment = kDefaultBufferAlignment)
171+
int64_t alignment = kDefaultBufferAlignment, bool ordered = false)
171172
: ArrayBuilder(pool, alignment),
172173
memo_table_(new internal::DictionaryMemoTable(pool, value_type)),
173174
delta_offset_(0),
174175
byte_width_(-1),
175176
indices_builder_(pool, alignment),
176-
value_type_(value_type) {}
177+
value_type_(value_type),
178+
ordered_(ordered) {}
177179

178180
template <typename T1 = T>
179181
explicit DictionaryBuilderBase(
180182
const std::shared_ptr<DataType>& index_type,
181183
enable_if_t<!is_fixed_size_binary_type<T1>::value, const std::shared_ptr<DataType>&>
182184
value_type,
183185
MemoryPool* pool = default_memory_pool(),
184-
int64_t alignment = kDefaultBufferAlignment)
186+
int64_t alignment = kDefaultBufferAlignment, bool ordered = false)
185187
: ArrayBuilder(pool, alignment),
186188
memo_table_(new internal::DictionaryMemoTable(pool, value_type)),
187189
delta_offset_(0),
188190
byte_width_(-1),
189191
indices_builder_(index_type, pool, alignment),
190-
value_type_(value_type) {}
192+
value_type_(value_type),
193+
ordered_(ordered) {}
191194

192195
template <typename B = BuilderType, typename T1 = T>
193196
DictionaryBuilderBase(uint8_t start_int_size,
@@ -196,38 +199,41 @@ class DictionaryBuilderBase : public ArrayBuilder {
196199
const std::shared_ptr<DataType>&>
197200
value_type,
198201
MemoryPool* pool = default_memory_pool(),
199-
int64_t alignment = kDefaultBufferAlignment)
202+
int64_t alignment = kDefaultBufferAlignment, bool ordered = false)
200203
: ArrayBuilder(pool, alignment),
201204
memo_table_(new internal::DictionaryMemoTable(pool, value_type)),
202205
delta_offset_(0),
203206
byte_width_(static_cast<const T1&>(*value_type).byte_width()),
204207
indices_builder_(start_int_size, pool, alignment),
205-
value_type_(value_type) {}
208+
value_type_(value_type),
209+
ordered_(ordered) {}
206210

207211
template <typename T1 = T>
208212
explicit DictionaryBuilderBase(
209213
enable_if_fixed_size_binary<T1, const std::shared_ptr<DataType>&> value_type,
210214
MemoryPool* pool = default_memory_pool(),
211-
int64_t alignment = kDefaultBufferAlignment)
215+
int64_t alignment = kDefaultBufferAlignment, bool ordered = false)
212216
: ArrayBuilder(pool, alignment),
213217
memo_table_(new internal::DictionaryMemoTable(pool, value_type)),
214218
delta_offset_(0),
215219
byte_width_(static_cast<const T1&>(*value_type).byte_width()),
216220
indices_builder_(pool, alignment),
217-
value_type_(value_type) {}
221+
value_type_(value_type),
222+
ordered_(ordered) {}
218223

219224
template <typename T1 = T>
220225
explicit DictionaryBuilderBase(
221226
const std::shared_ptr<DataType>& index_type,
222227
enable_if_fixed_size_binary<T1, const std::shared_ptr<DataType>&> value_type,
223228
MemoryPool* pool = default_memory_pool(),
224-
int64_t alignment = kDefaultBufferAlignment)
229+
int64_t alignment = kDefaultBufferAlignment, bool ordered = false)
225230
: ArrayBuilder(pool, alignment),
226231
memo_table_(new internal::DictionaryMemoTable(pool, value_type)),
227232
delta_offset_(0),
228233
byte_width_(static_cast<const T1&>(*value_type).byte_width()),
229234
indices_builder_(index_type, pool, alignment),
230-
value_type_(value_type) {}
235+
value_type_(value_type),
236+
ordered_(ordered) {}
231237

232238
template <typename T1 = T>
233239
explicit DictionaryBuilderBase(
@@ -237,13 +243,15 @@ class DictionaryBuilderBase : public ArrayBuilder {
237243
// This constructor doesn't check for errors. Use InsertMemoValues instead.
238244
explicit DictionaryBuilderBase(const std::shared_ptr<Array>& dictionary,
239245
MemoryPool* pool = default_memory_pool(),
240-
int64_t alignment = kDefaultBufferAlignment)
246+
int64_t alignment = kDefaultBufferAlignment,
247+
bool ordered = false)
241248
: ArrayBuilder(pool, alignment),
242249
memo_table_(new internal::DictionaryMemoTable(pool, dictionary)),
243250
delta_offset_(0),
244251
byte_width_(-1),
245252
indices_builder_(pool, alignment),
246-
value_type_(dictionary->type()) {}
253+
value_type_(dictionary->type()),
254+
ordered_(ordered) {}
247255

248256
~DictionaryBuilderBase() override = default;
249257

@@ -490,7 +498,7 @@ class DictionaryBuilderBase : public ArrayBuilder {
490498
Status Finish(std::shared_ptr<DictionaryArray>* out) { return FinishTyped(out); }
491499

492500
std::shared_ptr<DataType> type() const override {
493-
return ::arrow::dictionary(indices_builder_.type(), value_type_);
501+
return ::arrow::dictionary(indices_builder_.type(), value_type_, ordered_);
494502
}
495503

496504
protected:
@@ -561,6 +569,7 @@ class DictionaryBuilderBase : public ArrayBuilder {
561569

562570
BuilderType indices_builder_;
563571
std::shared_ptr<DataType> value_type_;
572+
bool ordered_ = false;
564573
};
565574

566575
template <typename BuilderType>
@@ -571,31 +580,53 @@ class DictionaryBuilderBase<BuilderType, NullType> : public ArrayBuilder {
571580
enable_if_t<std::is_base_of<AdaptiveIntBuilderBase, B>::value, uint8_t>
572581
start_int_size,
573582
const std::shared_ptr<DataType>& value_type,
574-
MemoryPool* pool = default_memory_pool())
575-
: ArrayBuilder(pool), indices_builder_(start_int_size, pool) {}
583+
MemoryPool* pool = default_memory_pool(),
584+
int64_t alignment = kDefaultBufferAlignment, bool ordered = false)
585+
: ArrayBuilder(pool, alignment),
586+
indices_builder_(start_int_size, pool, alignment),
587+
ordered_(ordered) {}
576588

577589
explicit DictionaryBuilderBase(const std::shared_ptr<DataType>& value_type,
578-
MemoryPool* pool = default_memory_pool())
579-
: ArrayBuilder(pool), indices_builder_(pool) {}
590+
MemoryPool* pool = default_memory_pool(),
591+
int64_t alignment = kDefaultBufferAlignment,
592+
bool ordered = false)
593+
: ArrayBuilder(pool, alignment),
594+
indices_builder_(pool, alignment),
595+
ordered_(ordered) {}
580596

581597
explicit DictionaryBuilderBase(const std::shared_ptr<DataType>& index_type,
582598
const std::shared_ptr<DataType>& value_type,
583-
MemoryPool* pool = default_memory_pool())
584-
: ArrayBuilder(pool), indices_builder_(index_type, pool) {}
599+
MemoryPool* pool = default_memory_pool(),
600+
int64_t alignment = kDefaultBufferAlignment,
601+
bool ordered = false)
602+
: ArrayBuilder(pool, alignment),
603+
indices_builder_(index_type, pool, alignment),
604+
ordered_(ordered) {}
585605

586606
template <typename B = BuilderType>
587607
explicit DictionaryBuilderBase(
588608
enable_if_t<std::is_base_of<AdaptiveIntBuilderBase, B>::value, uint8_t>
589609
start_int_size,
590-
MemoryPool* pool = default_memory_pool())
591-
: ArrayBuilder(pool), indices_builder_(start_int_size, pool) {}
610+
MemoryPool* pool = default_memory_pool(),
611+
int64_t alignment = kDefaultBufferAlignment, bool ordered = false)
612+
: ArrayBuilder(pool, alignment),
613+
indices_builder_(start_int_size, pool, alignment),
614+
ordered_(ordered) {}
592615

593-
explicit DictionaryBuilderBase(MemoryPool* pool = default_memory_pool())
594-
: ArrayBuilder(pool), indices_builder_(pool) {}
616+
explicit DictionaryBuilderBase(MemoryPool* pool = default_memory_pool(),
617+
int64_t alignment = kDefaultBufferAlignment,
618+
bool ordered = false)
619+
: ArrayBuilder(pool, alignment),
620+
indices_builder_(pool, alignment),
621+
ordered_(ordered) {}
595622

596623
explicit DictionaryBuilderBase(const std::shared_ptr<Array>& dictionary,
597-
MemoryPool* pool = default_memory_pool())
598-
: ArrayBuilder(pool), indices_builder_(pool) {}
624+
MemoryPool* pool = default_memory_pool(),
625+
int64_t alignment = kDefaultBufferAlignment,
626+
bool ordered = false)
627+
: ArrayBuilder(pool, alignment),
628+
indices_builder_(pool, alignment),
629+
ordered_(ordered) {}
599630

600631
/// \brief Append a scalar null value
601632
Status AppendNull() final {
@@ -647,7 +678,7 @@ class DictionaryBuilderBase<BuilderType, NullType> : public ArrayBuilder {
647678

648679
Status FinishInternal(std::shared_ptr<ArrayData>* out) override {
649680
ARROW_RETURN_NOT_OK(indices_builder_.FinishInternal(out));
650-
(*out)->type = dictionary((*out)->type, null());
681+
(*out)->type = dictionary((*out)->type, null(), ordered_);
651682
(*out)->dictionary = NullArray(0).data();
652683
return Status::OK();
653684
}
@@ -659,11 +690,12 @@ class DictionaryBuilderBase<BuilderType, NullType> : public ArrayBuilder {
659690
Status Finish(std::shared_ptr<DictionaryArray>* out) { return FinishTyped(out); }
660691

661692
std::shared_ptr<DataType> type() const override {
662-
return ::arrow::dictionary(indices_builder_.type(), null());
693+
return ::arrow::dictionary(indices_builder_.type(), null(), ordered_);
663694
}
664695

665696
protected:
666697
BuilderType indices_builder_;
698+
bool ordered_ = false;
667699
};
668700

669701
} // namespace internal

‎cpp/src/arrow/builder.cc‎

Lines changed: 18 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -168,17 +168,21 @@ struct DictionaryBuilderCase {
168168
template <typename ValueType>
169169
Status CreateFor() {
170170
using AdaptiveBuilderType = DictionaryBuilder<ValueType>;
171+
using ExactBuilderType =
172+
internal::DictionaryBuilderBase<TypeErasedIntBuilder, ValueType>;
171173
if (dictionary != nullptr) {
172-
out->reset(new AdaptiveBuilderType(dictionary, pool));
174+
out->reset(
175+
new AdaptiveBuilderType(dictionary, pool, kDefaultBufferAlignment, ordered));
173176
} else if (exact_index_type) {
174177
if (!is_integer(index_type->id())) {
175178
return Status::TypeError("MakeBuilder: invalid index type ", *index_type);
176179
}
177-
out->reset(new internal::DictionaryBuilderBase<TypeErasedIntBuilder, ValueType>(
178-
index_type, value_type, pool));
180+
out->reset(new ExactBuilderType(index_type, value_type, pool,
181+
kDefaultBufferAlignment, ordered));
179182
} else {
180183
auto start_int_size = index_type->byte_width();
181-
out->reset(new AdaptiveBuilderType(start_int_size, value_type, pool));
184+
out->reset(new AdaptiveBuilderType(start_int_size, value_type, pool,
185+
kDefaultBufferAlignment, ordered));
182186
}
183187
return Status::OK();
184188
}
@@ -188,8 +192,9 @@ struct DictionaryBuilderCase {
188192
MemoryPool* pool;
189193
const std::shared_ptr<DataType>& index_type;
190194
const std::shared_ptr<DataType>& value_type;
191-
const std::shared_ptr<Array>& dictionary;
195+
std::shared_ptr<Array> dictionary;
192196
bool exact_index_type;
197+
bool ordered;
193198
std::unique_ptr<ArrayBuilder>* out;
194199
};
195200

@@ -206,6 +211,7 @@ struct MakeBuilderImpl {
206211
dict_type.value_type(),
207212
/*dictionary=*/nullptr,
208213
exact_index_type,
214+
dict_type.ordered(),
209215
&out};
210216
return visitor.Make();
211217
}
@@ -332,9 +338,13 @@ Status MakeDictionaryBuilder(MemoryPool* pool, const std::shared_ptr<DataType>&
332338
const std::shared_ptr<Array>& dictionary,
333339
std::unique_ptr<ArrayBuilder>* out) {
334340
const auto& dict_type = static_cast<const DictionaryType&>(*type);
335-
DictionaryBuilderCase visitor = {
336-
pool, dict_type.index_type(), dict_type.value_type(),
337-
dictionary, /*exact_index_type=*/false, out};
341+
DictionaryBuilderCase visitor = {pool,
342+
dict_type.index_type(),
343+
dict_type.value_type(),
344+
dictionary,
345+
/*exact_index_type=*/false,
346+
dict_type.ordered(),
347+
out};
338348
return visitor.Make();
339349
}
340350

‎r/src/r_to_arrow.cpp‎

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -996,14 +996,6 @@ class RDictionaryConverter<ValueType, enable_if_has_string_view<ValueType>>
996996
Result<std::shared_ptr<ChunkedArray>> ToChunkedArray() override {
997997
ARROW_ASSIGN_OR_RAISE(auto result, this->builder_->Finish());
998998

999-
auto result_type = checked_cast<DictionaryType*>(result->type().get());
1000-
if (this->dict_type_->ordered() && !result_type->ordered()) {
1001-
// TODO: we should not have to do that, there is probably something wrong
1002-
// in the DictionaryBuilder code
1003-
result->data()->type =
1004-
arrow::dictionary(result_type->index_type(), result_type->value_type(), true);
1005-
}
1006-
1007999
return std::make_shared<ChunkedArray>(
10081000
std::make_shared<DictionaryArray>(result->data()));
10091001
}

0 commit comments

Comments
 (0)