pitrou commented on code in PR #50475:
URL: https://github.com/apache/arrow/pull/50475#discussion_r3623327462
##########
cpp/src/arrow/array/array_dict_test.cc:
##########
@@ -1040,6 +1040,177 @@ TYPED_TEST(TestDictionaryBuilderIndexByteWidth,
MakeBuilder) {
AssertIndexByteWidth<TypeParam, NullType>();
}
+// ----------------------------------------------------------------------
+// GH-37476: the requested index type's signedness must be preserved
+
+TEST(TestDictionaryBuilderIndexType, TypePreservesUnsignedIndexType) {
+ for (auto index_type : {uint8(), uint16(), uint32(), uint64()}) {
+ ARROW_SCOPED_TRACE("index_type = ", index_type->ToString());
+ auto dict_type = dictionary(index_type, utf8());
+ ASSERT_OK_AND_ASSIGN(auto boxed_builder, MakeBuilder(dict_type));
+
+ auto builder_type = boxed_builder->type();
+ const auto& dict_builder_type = checked_cast<const
DictionaryType&>(*builder_type);
+ AssertTypeEqual(*index_type, *dict_builder_type.index_type());
+ }
+}
+
+TEST(TestDictionaryBuilderIndexType, TypePreservesSignedIndexType) {
+ for (auto index_type : {int8(), int16(), int32(), int64()}) {
+ ARROW_SCOPED_TRACE("index_type = ", index_type->ToString());
+ auto dict_type = dictionary(index_type, utf8());
+ ASSERT_OK_AND_ASSIGN(auto boxed_builder, MakeBuilder(dict_type));
+
+ auto builder_type = boxed_builder->type();
+ const auto& dict_builder_type = checked_cast<const
DictionaryType&>(*builder_type);
+ AssertTypeEqual(*index_type, *dict_builder_type.index_type());
+ }
+}
+
+TEST(TestDictionaryBuilderIndexType, FinishPreservesUnsignedIndexType) {
+ for (auto index_type : {uint8(), uint16(), uint32(), uint64()}) {
+ ARROW_SCOPED_TRACE("index_type = ", index_type->ToString());
+ auto dict_type = dictionary(index_type, utf8());
+ ASSERT_OK_AND_ASSIGN(auto boxed_builder, MakeBuilder(dict_type));
+ auto& builder =
checked_cast<DictionaryBuilder<StringType>&>(*boxed_builder);
+
+ ASSERT_OK(builder.Append("a"));
+ ASSERT_OK(builder.Append("b"));
+ ASSERT_OK(builder.AppendNull());
+ ASSERT_OK(builder.Append("a"));
+
+ std::shared_ptr<Array> result;
+ ASSERT_OK(builder.Finish(&result));
Review Comment:
Can use the Result-returning `Finish` version:
```suggestion
ASSERT_OK_AND_ASSIGN(auto result, builder.Finish());
```
##########
cpp/src/arrow/array/array_dict_test.cc:
##########
@@ -1040,6 +1040,177 @@ TYPED_TEST(TestDictionaryBuilderIndexByteWidth,
MakeBuilder) {
AssertIndexByteWidth<TypeParam, NullType>();
}
+// ----------------------------------------------------------------------
+// GH-37476: the requested index type's signedness must be preserved
+
+TEST(TestDictionaryBuilderIndexType, TypePreservesUnsignedIndexType) {
+ for (auto index_type : {uint8(), uint16(), uint32(), uint64()}) {
+ ARROW_SCOPED_TRACE("index_type = ", index_type->ToString());
+ auto dict_type = dictionary(index_type, utf8());
+ ASSERT_OK_AND_ASSIGN(auto boxed_builder, MakeBuilder(dict_type));
+
+ auto builder_type = boxed_builder->type();
+ const auto& dict_builder_type = checked_cast<const
DictionaryType&>(*builder_type);
+ AssertTypeEqual(*index_type, *dict_builder_type.index_type());
+ }
+}
+
+TEST(TestDictionaryBuilderIndexType, TypePreservesSignedIndexType) {
Review Comment:
You could fold this test into the above, no need for duplicate code here
IMHO.
##########
cpp/src/arrow/array/array_dict_test.cc:
##########
@@ -1040,6 +1040,177 @@ TYPED_TEST(TestDictionaryBuilderIndexByteWidth,
MakeBuilder) {
AssertIndexByteWidth<TypeParam, NullType>();
}
+// ----------------------------------------------------------------------
+// GH-37476: the requested index type's signedness must be preserved
+
+TEST(TestDictionaryBuilderIndexType, TypePreservesUnsignedIndexType) {
+ for (auto index_type : {uint8(), uint16(), uint32(), uint64()}) {
+ ARROW_SCOPED_TRACE("index_type = ", index_type->ToString());
+ auto dict_type = dictionary(index_type, utf8());
+ ASSERT_OK_AND_ASSIGN(auto boxed_builder, MakeBuilder(dict_type));
+
+ auto builder_type = boxed_builder->type();
+ const auto& dict_builder_type = checked_cast<const
DictionaryType&>(*builder_type);
+ AssertTypeEqual(*index_type, *dict_builder_type.index_type());
+ }
+}
+
+TEST(TestDictionaryBuilderIndexType, TypePreservesSignedIndexType) {
+ for (auto index_type : {int8(), int16(), int32(), int64()}) {
+ ARROW_SCOPED_TRACE("index_type = ", index_type->ToString());
+ auto dict_type = dictionary(index_type, utf8());
+ ASSERT_OK_AND_ASSIGN(auto boxed_builder, MakeBuilder(dict_type));
+
+ auto builder_type = boxed_builder->type();
+ const auto& dict_builder_type = checked_cast<const
DictionaryType&>(*builder_type);
+ AssertTypeEqual(*index_type, *dict_builder_type.index_type());
+ }
+}
+
+TEST(TestDictionaryBuilderIndexType, FinishPreservesUnsignedIndexType) {
+ for (auto index_type : {uint8(), uint16(), uint32(), uint64()}) {
+ ARROW_SCOPED_TRACE("index_type = ", index_type->ToString());
+ auto dict_type = dictionary(index_type, utf8());
+ ASSERT_OK_AND_ASSIGN(auto boxed_builder, MakeBuilder(dict_type));
+ auto& builder =
checked_cast<DictionaryBuilder<StringType>&>(*boxed_builder);
+
+ ASSERT_OK(builder.Append("a"));
+ ASSERT_OK(builder.Append("b"));
+ ASSERT_OK(builder.AppendNull());
+ ASSERT_OK(builder.Append("a"));
+
+ std::shared_ptr<Array> result;
+ ASSERT_OK(builder.Finish(&result));
+ ASSERT_OK(result->ValidateFull());
+
+ auto ex_dict = ArrayFromJSON(utf8(), R"(["a", "b"])");
+ auto ex_indices = ArrayFromJSON(index_type, "[0, 1, null, 0]");
+ DictionaryArray expected(dict_type, ex_indices, ex_dict);
+ AssertTypeEqual(*dict_type, *result->type());
+ AssertArraysEqual(expected, *result);
+ }
+}
+
+TEST(TestDictionaryBuilderIndexType, UnsignedIndexWidthStillAdapts) {
+ // The index width remains adaptive, exactly as it is for signed index types:
+ // only the signedness of the requested type is preserved.
+ auto dict_type = dictionary(uint8(), utf8());
+ ASSERT_OK_AND_ASSIGN(auto boxed_builder, MakeBuilder(dict_type));
+ auto& builder = checked_cast<DictionaryBuilder<StringType>&>(*boxed_builder);
+
+ // More distinct values than a uint8 index start width accommodates.
+ for (int i = 0; i < 200; ++i) {
+ ASSERT_OK(builder.Append(std::to_string(i)));
+ }
+
+ std::shared_ptr<Array> result;
+ ASSERT_OK(builder.Finish(&result));
+ ASSERT_OK(result->ValidateFull());
+
+ const auto& result_type = checked_cast<const
DictionaryType&>(*result->type());
+ // Widened, but still unsigned rather than falling back to a signed type.
+ AssertTypeEqual(*uint16(), *result_type.index_type());
+}
+
+TEST(TestDictionaryBuilderIndexType, NullValueTypeUnsignedIndexType) {
+ auto dict_type = dictionary(uint32(), null());
+ ASSERT_OK_AND_ASSIGN(auto boxed_builder, MakeBuilder(dict_type));
+ auto& builder = checked_cast<DictionaryBuilder<NullType>&>(*boxed_builder);
+
+ ASSERT_OK(builder.AppendNull());
+
+ std::shared_ptr<Array> result;
+ ASSERT_OK(builder.Finish(&result));
+ ASSERT_OK(result->ValidateFull());
+
+ const auto& result_type = checked_cast<const
DictionaryType&>(*result->type());
+ AssertTypeEqual(*uint32(), *result_type.index_type());
+}
+
+TEST(TestDictionaryBuilderIndexType, FinishDeltaPreservesUnsignedIndexType) {
+ auto dict_type = dictionary(uint32(), utf8());
+ ASSERT_OK_AND_ASSIGN(auto boxed_builder, MakeBuilder(dict_type));
+ auto& builder = checked_cast<DictionaryBuilder<StringType>&>(*boxed_builder);
+
+ ASSERT_OK(builder.Append("a"));
+ ASSERT_OK(builder.Append("b"));
+
+ std::shared_ptr<Array> result_indices, result_delta;
+ ASSERT_OK(builder.FinishDelta(&result_indices, &result_delta));
+ ASSERT_OK(result_indices->ValidateFull());
+
+ // The delta indices must carry the requested index type too, not the signed
+ // type that the adaptive indices builder produces internally.
+ AssertTypeEqual(*uint32(), *result_indices->type());
+ AssertArraysEqual(*ArrayFromJSON(uint32(), "[0, 1]"), *result_indices);
+}
+
+TEST(TestDictionaryBuilderIndexType,
SuppliedDictionaryPreservesUnsignedIndexType) {
+ // The supplied-dictionary constructor starts the adaptive indices builder
at its
+ // default width rather than at the requested one, so the requested *width*
is not
+ // honoured on this path. That predates this change: a requested int32
reports int8
+ // too. What must hold is that the *signedness* survives, i.e. uint8 and not
int8.
+ auto dict_values = ArrayFromJSON(utf8(), R"(["a", "b"])");
+
+ ASSERT_OK_AND_ASSIGN(auto unsigned_builder,
+ MakeDictionaryBuilder(dictionary(uint32(), utf8()),
dict_values));
+ auto unsigned_type = unsigned_builder->type();
+ AssertTypeEqual(*uint8(),
+ *checked_cast<const
DictionaryType&>(*unsigned_type).index_type());
+
+ ASSERT_OK_AND_ASSIGN(auto signed_builder,
+ MakeDictionaryBuilder(dictionary(int32(), utf8()),
dict_values));
Review Comment:
So we're also testing for a signed index type here? Wasn't it already
tested? If not, at least the test name and comment should be fixed.
##########
cpp/src/arrow/array/builder_dict.h:
##########
@@ -134,6 +134,33 @@ class ARROW_EXPORT DictionaryMemoTable {
namespace internal {
+/// \brief Return the unsigned integer type of the same width when an unsigned
+/// dictionary index type was requested.
+///
+/// The adaptive indices builder only ever produces signed integer types.
Dictionary
+/// indices are non-negative, so the signed and unsigned integer types of a
given
+/// width have identical memory layout and reporting one as the other is
+/// value-preserving (GH-37476). The width itself stays adaptive, exactly as
it is
+/// for signed index types; only the signedness of the requested type is
preserved.
+inline std::shared_ptr<DataType> MaybeUnsignedIndexType(
Review Comment:
I don't think this has to be inlined in this header file, as this function
shouldn't be performance-critical.
##########
python/pyarrow/tests/test_array.py:
##########
@@ -4527,3 +4527,48 @@ def test_dunders_checked_overflow():
arr ** pa.scalar(2, type=pa.int8())
with pytest.raises(pa.ArrowInvalid, match=error_match):
arr / (-arr)
+
+
[email protected]("index_type", [pa.uint8(), pa.uint16(),
+ pa.uint32(), pa.uint64()])
+def test_dictionary_array_preserves_unsigned_index_type(index_type):
+ # GH-37476: an unsigned dictionary index type used to be silently replaced
+ # by the signed integer type of the same width.
+ dict_type = pa.dictionary(index_type, pa.string())
+
+ arr = pa.array(["a", "b", None, "a"], type=dict_type)
+ assert arr.type == dict_type
+ assert arr.to_pylist() == ["a", "b", None, "a"]
+ arr.validate(full=True)
+
+ chunked = pa.chunked_array([["a", "b", "a"]], dict_type)
+ assert chunked.type == dict_type
+
+
[email protected]("index_type", [pa.int8(), pa.int16(),
+ pa.int32(), pa.int64()])
+def test_dictionary_array_signed_index_type_unchanged(index_type):
+ dict_type = pa.dictionary(index_type, pa.string())
+ arr = pa.array(["a", "b", "a"], type=dict_type)
+ assert arr.type == dict_type
+
+
+def test_dictionary_array_index_width_still_adapts():
+ # The index width remains adaptive, as it is for signed index types; only
+ # the signedness of the requested index type is preserved.
+ values = [str(i) for i in range(200)]
+ arr = pa.array(values, type=pa.dictionary(pa.uint8(), pa.string()))
+ assert arr.type == pa.dictionary(pa.uint16(), pa.string())
+ assert arr.to_pylist() == values
+
+
[email protected]
+def test_dictionary_uint64_index_to_pandas_not_supported():
+ # GH-37476: uint64 dictionary indices are now preserved instead of being
+ # silently downgraded to int64. Arrow deliberately does not support
converting
+ # them to pandas (pandas categorical codes are signed), so that
pre-existing
+ # limitation is now reachable, where the silent downgrade used to mask it.
Review Comment:
Hmm, that sounds like a potential regression for third-party code. Can we
support converting uint64 indices to Pandas?
You can take a look at `CategoricalWriter` in `arrow_to_pandas.cc`. It seems
that `WriteIndicesUniform` is the main place that needs to be changed.
##########
cpp/src/arrow/array/array_dict_test.cc:
##########
@@ -1040,6 +1040,177 @@ TYPED_TEST(TestDictionaryBuilderIndexByteWidth,
MakeBuilder) {
AssertIndexByteWidth<TypeParam, NullType>();
}
+// ----------------------------------------------------------------------
+// GH-37476: the requested index type's signedness must be preserved
+
+TEST(TestDictionaryBuilderIndexType, TypePreservesUnsignedIndexType) {
+ for (auto index_type : {uint8(), uint16(), uint32(), uint64()}) {
+ ARROW_SCOPED_TRACE("index_type = ", index_type->ToString());
+ auto dict_type = dictionary(index_type, utf8());
+ ASSERT_OK_AND_ASSIGN(auto boxed_builder, MakeBuilder(dict_type));
+
+ auto builder_type = boxed_builder->type();
+ const auto& dict_builder_type = checked_cast<const
DictionaryType&>(*builder_type);
+ AssertTypeEqual(*index_type, *dict_builder_type.index_type());
+ }
+}
+
+TEST(TestDictionaryBuilderIndexType, TypePreservesSignedIndexType) {
+ for (auto index_type : {int8(), int16(), int32(), int64()}) {
+ ARROW_SCOPED_TRACE("index_type = ", index_type->ToString());
+ auto dict_type = dictionary(index_type, utf8());
+ ASSERT_OK_AND_ASSIGN(auto boxed_builder, MakeBuilder(dict_type));
+
+ auto builder_type = boxed_builder->type();
+ const auto& dict_builder_type = checked_cast<const
DictionaryType&>(*builder_type);
+ AssertTypeEqual(*index_type, *dict_builder_type.index_type());
+ }
+}
+
+TEST(TestDictionaryBuilderIndexType, FinishPreservesUnsignedIndexType) {
+ for (auto index_type : {uint8(), uint16(), uint32(), uint64()}) {
+ ARROW_SCOPED_TRACE("index_type = ", index_type->ToString());
+ auto dict_type = dictionary(index_type, utf8());
+ ASSERT_OK_AND_ASSIGN(auto boxed_builder, MakeBuilder(dict_type));
+ auto& builder =
checked_cast<DictionaryBuilder<StringType>&>(*boxed_builder);
+
+ ASSERT_OK(builder.Append("a"));
+ ASSERT_OK(builder.Append("b"));
+ ASSERT_OK(builder.AppendNull());
+ ASSERT_OK(builder.Append("a"));
+
+ std::shared_ptr<Array> result;
+ ASSERT_OK(builder.Finish(&result));
+ ASSERT_OK(result->ValidateFull());
+
+ auto ex_dict = ArrayFromJSON(utf8(), R"(["a", "b"])");
+ auto ex_indices = ArrayFromJSON(index_type, "[0, 1, null, 0]");
+ DictionaryArray expected(dict_type, ex_indices, ex_dict);
+ AssertTypeEqual(*dict_type, *result->type());
+ AssertArraysEqual(expected, *result);
+ }
+}
+
+TEST(TestDictionaryBuilderIndexType, UnsignedIndexWidthStillAdapts) {
+ // The index width remains adaptive, exactly as it is for signed index types:
+ // only the signedness of the requested type is preserved.
+ auto dict_type = dictionary(uint8(), utf8());
+ ASSERT_OK_AND_ASSIGN(auto boxed_builder, MakeBuilder(dict_type));
+ auto& builder = checked_cast<DictionaryBuilder<StringType>&>(*boxed_builder);
+
+ // More distinct values than a uint8 index start width accommodates.
+ for (int i = 0; i < 200; ++i) {
+ ASSERT_OK(builder.Append(std::to_string(i)));
+ }
+
+ std::shared_ptr<Array> result;
+ ASSERT_OK(builder.Finish(&result));
+ ASSERT_OK(result->ValidateFull());
+
+ const auto& result_type = checked_cast<const
DictionaryType&>(*result->type());
+ // Widened, but still unsigned rather than falling back to a signed type.
+ AssertTypeEqual(*uint16(), *result_type.index_type());
+}
+
+TEST(TestDictionaryBuilderIndexType, NullValueTypeUnsignedIndexType) {
+ auto dict_type = dictionary(uint32(), null());
+ ASSERT_OK_AND_ASSIGN(auto boxed_builder, MakeBuilder(dict_type));
+ auto& builder = checked_cast<DictionaryBuilder<NullType>&>(*boxed_builder);
+
+ ASSERT_OK(builder.AppendNull());
+
+ std::shared_ptr<Array> result;
+ ASSERT_OK(builder.Finish(&result));
+ ASSERT_OK(result->ValidateFull());
+
+ const auto& result_type = checked_cast<const
DictionaryType&>(*result->type());
+ AssertTypeEqual(*uint32(), *result_type.index_type());
+}
+
+TEST(TestDictionaryBuilderIndexType, FinishDeltaPreservesUnsignedIndexType) {
+ auto dict_type = dictionary(uint32(), utf8());
+ ASSERT_OK_AND_ASSIGN(auto boxed_builder, MakeBuilder(dict_type));
+ auto& builder = checked_cast<DictionaryBuilder<StringType>&>(*boxed_builder);
+
+ ASSERT_OK(builder.Append("a"));
+ ASSERT_OK(builder.Append("b"));
+
+ std::shared_ptr<Array> result_indices, result_delta;
+ ASSERT_OK(builder.FinishDelta(&result_indices, &result_delta));
+ ASSERT_OK(result_indices->ValidateFull());
+
+ // The delta indices must carry the requested index type too, not the signed
+ // type that the adaptive indices builder produces internally.
+ AssertTypeEqual(*uint32(), *result_indices->type());
+ AssertArraysEqual(*ArrayFromJSON(uint32(), "[0, 1]"), *result_indices);
+}
+
+TEST(TestDictionaryBuilderIndexType,
SuppliedDictionaryPreservesUnsignedIndexType) {
+ // The supplied-dictionary constructor starts the adaptive indices builder
at its
+ // default width rather than at the requested one, so the requested *width*
is not
+ // honoured on this path. That predates this change: a requested int32
reports int8
Review Comment:
"this change" will not be understandable to a future reader of this comment,
can you reword it to be less context-dependent?
##########
cpp/src/arrow/array/array_dict_test.cc:
##########
@@ -1040,6 +1040,177 @@ TYPED_TEST(TestDictionaryBuilderIndexByteWidth,
MakeBuilder) {
AssertIndexByteWidth<TypeParam, NullType>();
}
+// ----------------------------------------------------------------------
+// GH-37476: the requested index type's signedness must be preserved
+
+TEST(TestDictionaryBuilderIndexType, TypePreservesUnsignedIndexType) {
+ for (auto index_type : {uint8(), uint16(), uint32(), uint64()}) {
+ ARROW_SCOPED_TRACE("index_type = ", index_type->ToString());
+ auto dict_type = dictionary(index_type, utf8());
+ ASSERT_OK_AND_ASSIGN(auto boxed_builder, MakeBuilder(dict_type));
+
+ auto builder_type = boxed_builder->type();
+ const auto& dict_builder_type = checked_cast<const
DictionaryType&>(*builder_type);
+ AssertTypeEqual(*index_type, *dict_builder_type.index_type());
+ }
+}
+
+TEST(TestDictionaryBuilderIndexType, TypePreservesSignedIndexType) {
+ for (auto index_type : {int8(), int16(), int32(), int64()}) {
+ ARROW_SCOPED_TRACE("index_type = ", index_type->ToString());
+ auto dict_type = dictionary(index_type, utf8());
+ ASSERT_OK_AND_ASSIGN(auto boxed_builder, MakeBuilder(dict_type));
+
+ auto builder_type = boxed_builder->type();
+ const auto& dict_builder_type = checked_cast<const
DictionaryType&>(*builder_type);
+ AssertTypeEqual(*index_type, *dict_builder_type.index_type());
+ }
+}
+
+TEST(TestDictionaryBuilderIndexType, FinishPreservesUnsignedIndexType) {
+ for (auto index_type : {uint8(), uint16(), uint32(), uint64()}) {
+ ARROW_SCOPED_TRACE("index_type = ", index_type->ToString());
+ auto dict_type = dictionary(index_type, utf8());
+ ASSERT_OK_AND_ASSIGN(auto boxed_builder, MakeBuilder(dict_type));
+ auto& builder =
checked_cast<DictionaryBuilder<StringType>&>(*boxed_builder);
+
+ ASSERT_OK(builder.Append("a"));
+ ASSERT_OK(builder.Append("b"));
+ ASSERT_OK(builder.AppendNull());
+ ASSERT_OK(builder.Append("a"));
+
+ std::shared_ptr<Array> result;
+ ASSERT_OK(builder.Finish(&result));
+ ASSERT_OK(result->ValidateFull());
+
+ auto ex_dict = ArrayFromJSON(utf8(), R"(["a", "b"])");
+ auto ex_indices = ArrayFromJSON(index_type, "[0, 1, null, 0]");
+ DictionaryArray expected(dict_type, ex_indices, ex_dict);
+ AssertTypeEqual(*dict_type, *result->type());
+ AssertArraysEqual(expected, *result);
+ }
+}
+
+TEST(TestDictionaryBuilderIndexType, UnsignedIndexWidthStillAdapts) {
+ // The index width remains adaptive, exactly as it is for signed index types:
+ // only the signedness of the requested type is preserved.
+ auto dict_type = dictionary(uint8(), utf8());
+ ASSERT_OK_AND_ASSIGN(auto boxed_builder, MakeBuilder(dict_type));
+ auto& builder = checked_cast<DictionaryBuilder<StringType>&>(*boxed_builder);
+
+ // More distinct values than a uint8 index start width accommodates.
+ for (int i = 0; i < 200; ++i) {
Review Comment:
Hmm, is that right? A uint8 index should be able to represent 256 distinct
dictionary entries (or 255 depending on how it's coded internally :-)).
##########
cpp/src/arrow/array/array_dict_test.cc:
##########
@@ -1040,6 +1040,177 @@ TYPED_TEST(TestDictionaryBuilderIndexByteWidth,
MakeBuilder) {
AssertIndexByteWidth<TypeParam, NullType>();
}
+// ----------------------------------------------------------------------
+// GH-37476: the requested index type's signedness must be preserved
+
+TEST(TestDictionaryBuilderIndexType, TypePreservesUnsignedIndexType) {
+ for (auto index_type : {uint8(), uint16(), uint32(), uint64()}) {
+ ARROW_SCOPED_TRACE("index_type = ", index_type->ToString());
+ auto dict_type = dictionary(index_type, utf8());
+ ASSERT_OK_AND_ASSIGN(auto boxed_builder, MakeBuilder(dict_type));
+
+ auto builder_type = boxed_builder->type();
+ const auto& dict_builder_type = checked_cast<const
DictionaryType&>(*builder_type);
+ AssertTypeEqual(*index_type, *dict_builder_type.index_type());
+ }
+}
+
+TEST(TestDictionaryBuilderIndexType, TypePreservesSignedIndexType) {
+ for (auto index_type : {int8(), int16(), int32(), int64()}) {
+ ARROW_SCOPED_TRACE("index_type = ", index_type->ToString());
+ auto dict_type = dictionary(index_type, utf8());
+ ASSERT_OK_AND_ASSIGN(auto boxed_builder, MakeBuilder(dict_type));
+
+ auto builder_type = boxed_builder->type();
+ const auto& dict_builder_type = checked_cast<const
DictionaryType&>(*builder_type);
+ AssertTypeEqual(*index_type, *dict_builder_type.index_type());
+ }
+}
+
+TEST(TestDictionaryBuilderIndexType, FinishPreservesUnsignedIndexType) {
+ for (auto index_type : {uint8(), uint16(), uint32(), uint64()}) {
+ ARROW_SCOPED_TRACE("index_type = ", index_type->ToString());
+ auto dict_type = dictionary(index_type, utf8());
+ ASSERT_OK_AND_ASSIGN(auto boxed_builder, MakeBuilder(dict_type));
+ auto& builder =
checked_cast<DictionaryBuilder<StringType>&>(*boxed_builder);
+
+ ASSERT_OK(builder.Append("a"));
+ ASSERT_OK(builder.Append("b"));
+ ASSERT_OK(builder.AppendNull());
+ ASSERT_OK(builder.Append("a"));
+
+ std::shared_ptr<Array> result;
+ ASSERT_OK(builder.Finish(&result));
+ ASSERT_OK(result->ValidateFull());
+
+ auto ex_dict = ArrayFromJSON(utf8(), R"(["a", "b"])");
+ auto ex_indices = ArrayFromJSON(index_type, "[0, 1, null, 0]");
+ DictionaryArray expected(dict_type, ex_indices, ex_dict);
+ AssertTypeEqual(*dict_type, *result->type());
+ AssertArraysEqual(expected, *result);
+ }
+}
+
+TEST(TestDictionaryBuilderIndexType, UnsignedIndexWidthStillAdapts) {
+ // The index width remains adaptive, exactly as it is for signed index types:
+ // only the signedness of the requested type is preserved.
+ auto dict_type = dictionary(uint8(), utf8());
+ ASSERT_OK_AND_ASSIGN(auto boxed_builder, MakeBuilder(dict_type));
+ auto& builder = checked_cast<DictionaryBuilder<StringType>&>(*boxed_builder);
+
+ // More distinct values than a uint8 index start width accommodates.
+ for (int i = 0; i < 200; ++i) {
+ ASSERT_OK(builder.Append(std::to_string(i)));
+ }
+
+ std::shared_ptr<Array> result;
+ ASSERT_OK(builder.Finish(&result));
+ ASSERT_OK(result->ValidateFull());
+
+ const auto& result_type = checked_cast<const
DictionaryType&>(*result->type());
+ // Widened, but still unsigned rather than falling back to a signed type.
+ AssertTypeEqual(*uint16(), *result_type.index_type());
+}
+
+TEST(TestDictionaryBuilderIndexType, NullValueTypeUnsignedIndexType) {
+ auto dict_type = dictionary(uint32(), null());
+ ASSERT_OK_AND_ASSIGN(auto boxed_builder, MakeBuilder(dict_type));
+ auto& builder = checked_cast<DictionaryBuilder<NullType>&>(*boxed_builder);
+
+ ASSERT_OK(builder.AppendNull());
+
+ std::shared_ptr<Array> result;
+ ASSERT_OK(builder.Finish(&result));
+ ASSERT_OK(result->ValidateFull());
+
+ const auto& result_type = checked_cast<const
DictionaryType&>(*result->type());
+ AssertTypeEqual(*uint32(), *result_type.index_type());
+}
+
+TEST(TestDictionaryBuilderIndexType, FinishDeltaPreservesUnsignedIndexType) {
+ auto dict_type = dictionary(uint32(), utf8());
+ ASSERT_OK_AND_ASSIGN(auto boxed_builder, MakeBuilder(dict_type));
+ auto& builder = checked_cast<DictionaryBuilder<StringType>&>(*boxed_builder);
+
+ ASSERT_OK(builder.Append("a"));
+ ASSERT_OK(builder.Append("b"));
+
+ std::shared_ptr<Array> result_indices, result_delta;
+ ASSERT_OK(builder.FinishDelta(&result_indices, &result_delta));
+ ASSERT_OK(result_indices->ValidateFull());
+
+ // The delta indices must carry the requested index type too, not the signed
+ // type that the adaptive indices builder produces internally.
+ AssertTypeEqual(*uint32(), *result_indices->type());
+ AssertArraysEqual(*ArrayFromJSON(uint32(), "[0, 1]"), *result_indices);
+}
+
+TEST(TestDictionaryBuilderIndexType,
SuppliedDictionaryPreservesUnsignedIndexType) {
+ // The supplied-dictionary constructor starts the adaptive indices builder
at its
+ // default width rather than at the requested one, so the requested *width*
is not
+ // honoured on this path. That predates this change: a requested int32
reports int8
+ // too. What must hold is that the *signedness* survives, i.e. uint8 and not
int8.
+ auto dict_values = ArrayFromJSON(utf8(), R"(["a", "b"])");
+
+ ASSERT_OK_AND_ASSIGN(auto unsigned_builder,
+ MakeDictionaryBuilder(dictionary(uint32(), utf8()),
dict_values));
+ auto unsigned_type = unsigned_builder->type();
+ AssertTypeEqual(*uint8(),
+ *checked_cast<const
DictionaryType&>(*unsigned_type).index_type());
+
+ ASSERT_OK_AND_ASSIGN(auto signed_builder,
+ MakeDictionaryBuilder(dictionary(int32(), utf8()),
dict_values));
+ auto signed_type = signed_builder->type();
+ AssertTypeEqual(*int8(),
+ *checked_cast<const
DictionaryType&>(*signed_type).index_type());
+}
+
+TEST(TestDictionaryBuilderIndexType, FixedSizeBinaryUnsignedIndexType) {
Review Comment:
Yet another test that may not be very useful, is it actually exercising a
distinct code path from the above?
##########
cpp/src/arrow/array/array_dict_test.cc:
##########
@@ -1040,6 +1040,177 @@ TYPED_TEST(TestDictionaryBuilderIndexByteWidth,
MakeBuilder) {
AssertIndexByteWidth<TypeParam, NullType>();
}
+// ----------------------------------------------------------------------
+// GH-37476: the requested index type's signedness must be preserved
+
+TEST(TestDictionaryBuilderIndexType, TypePreservesUnsignedIndexType) {
+ for (auto index_type : {uint8(), uint16(), uint32(), uint64()}) {
+ ARROW_SCOPED_TRACE("index_type = ", index_type->ToString());
+ auto dict_type = dictionary(index_type, utf8());
+ ASSERT_OK_AND_ASSIGN(auto boxed_builder, MakeBuilder(dict_type));
+
+ auto builder_type = boxed_builder->type();
+ const auto& dict_builder_type = checked_cast<const
DictionaryType&>(*builder_type);
+ AssertTypeEqual(*index_type, *dict_builder_type.index_type());
+ }
+}
+
+TEST(TestDictionaryBuilderIndexType, TypePreservesSignedIndexType) {
+ for (auto index_type : {int8(), int16(), int32(), int64()}) {
+ ARROW_SCOPED_TRACE("index_type = ", index_type->ToString());
+ auto dict_type = dictionary(index_type, utf8());
+ ASSERT_OK_AND_ASSIGN(auto boxed_builder, MakeBuilder(dict_type));
+
+ auto builder_type = boxed_builder->type();
+ const auto& dict_builder_type = checked_cast<const
DictionaryType&>(*builder_type);
+ AssertTypeEqual(*index_type, *dict_builder_type.index_type());
+ }
+}
+
+TEST(TestDictionaryBuilderIndexType, FinishPreservesUnsignedIndexType) {
+ for (auto index_type : {uint8(), uint16(), uint32(), uint64()}) {
+ ARROW_SCOPED_TRACE("index_type = ", index_type->ToString());
+ auto dict_type = dictionary(index_type, utf8());
+ ASSERT_OK_AND_ASSIGN(auto boxed_builder, MakeBuilder(dict_type));
+ auto& builder =
checked_cast<DictionaryBuilder<StringType>&>(*boxed_builder);
+
+ ASSERT_OK(builder.Append("a"));
+ ASSERT_OK(builder.Append("b"));
+ ASSERT_OK(builder.AppendNull());
+ ASSERT_OK(builder.Append("a"));
+
+ std::shared_ptr<Array> result;
+ ASSERT_OK(builder.Finish(&result));
+ ASSERT_OK(result->ValidateFull());
+
+ auto ex_dict = ArrayFromJSON(utf8(), R"(["a", "b"])");
+ auto ex_indices = ArrayFromJSON(index_type, "[0, 1, null, 0]");
+ DictionaryArray expected(dict_type, ex_indices, ex_dict);
+ AssertTypeEqual(*dict_type, *result->type());
+ AssertArraysEqual(expected, *result);
+ }
+}
+
+TEST(TestDictionaryBuilderIndexType, UnsignedIndexWidthStillAdapts) {
+ // The index width remains adaptive, exactly as it is for signed index types:
+ // only the signedness of the requested type is preserved.
+ auto dict_type = dictionary(uint8(), utf8());
+ ASSERT_OK_AND_ASSIGN(auto boxed_builder, MakeBuilder(dict_type));
+ auto& builder = checked_cast<DictionaryBuilder<StringType>&>(*boxed_builder);
+
+ // More distinct values than a uint8 index start width accommodates.
+ for (int i = 0; i < 200; ++i) {
+ ASSERT_OK(builder.Append(std::to_string(i)));
+ }
+
+ std::shared_ptr<Array> result;
+ ASSERT_OK(builder.Finish(&result));
+ ASSERT_OK(result->ValidateFull());
+
+ const auto& result_type = checked_cast<const
DictionaryType&>(*result->type());
+ // Widened, but still unsigned rather than falling back to a signed type.
+ AssertTypeEqual(*uint16(), *result_type.index_type());
+}
+
+TEST(TestDictionaryBuilderIndexType, NullValueTypeUnsignedIndexType) {
+ auto dict_type = dictionary(uint32(), null());
+ ASSERT_OK_AND_ASSIGN(auto boxed_builder, MakeBuilder(dict_type));
+ auto& builder = checked_cast<DictionaryBuilder<NullType>&>(*boxed_builder);
+
+ ASSERT_OK(builder.AppendNull());
+
+ std::shared_ptr<Array> result;
+ ASSERT_OK(builder.Finish(&result));
+ ASSERT_OK(result->ValidateFull());
+
+ const auto& result_type = checked_cast<const
DictionaryType&>(*result->type());
+ AssertTypeEqual(*uint32(), *result_type.index_type());
+}
+
+TEST(TestDictionaryBuilderIndexType, FinishDeltaPreservesUnsignedIndexType) {
+ auto dict_type = dictionary(uint32(), utf8());
+ ASSERT_OK_AND_ASSIGN(auto boxed_builder, MakeBuilder(dict_type));
+ auto& builder = checked_cast<DictionaryBuilder<StringType>&>(*boxed_builder);
+
+ ASSERT_OK(builder.Append("a"));
+ ASSERT_OK(builder.Append("b"));
+
+ std::shared_ptr<Array> result_indices, result_delta;
+ ASSERT_OK(builder.FinishDelta(&result_indices, &result_delta));
+ ASSERT_OK(result_indices->ValidateFull());
+
+ // The delta indices must carry the requested index type too, not the signed
+ // type that the adaptive indices builder produces internally.
+ AssertTypeEqual(*uint32(), *result_indices->type());
+ AssertArraysEqual(*ArrayFromJSON(uint32(), "[0, 1]"), *result_indices);
+}
+
+TEST(TestDictionaryBuilderIndexType,
SuppliedDictionaryPreservesUnsignedIndexType) {
+ // The supplied-dictionary constructor starts the adaptive indices builder
at its
+ // default width rather than at the requested one, so the requested *width*
is not
+ // honoured on this path. That predates this change: a requested int32
reports int8
+ // too. What must hold is that the *signedness* survives, i.e. uint8 and not
int8.
+ auto dict_values = ArrayFromJSON(utf8(), R"(["a", "b"])");
+
+ ASSERT_OK_AND_ASSIGN(auto unsigned_builder,
+ MakeDictionaryBuilder(dictionary(uint32(), utf8()),
dict_values));
+ auto unsigned_type = unsigned_builder->type();
+ AssertTypeEqual(*uint8(),
+ *checked_cast<const
DictionaryType&>(*unsigned_type).index_type());
+
+ ASSERT_OK_AND_ASSIGN(auto signed_builder,
+ MakeDictionaryBuilder(dictionary(int32(), utf8()),
dict_values));
+ auto signed_type = signed_builder->type();
+ AssertTypeEqual(*int8(),
+ *checked_cast<const
DictionaryType&>(*signed_type).index_type());
+}
+
+TEST(TestDictionaryBuilderIndexType, FixedSizeBinaryUnsignedIndexType) {
+ auto dict_type = dictionary(uint16(), fixed_size_binary(2));
+ ASSERT_OK_AND_ASSIGN(auto boxed_builder, MakeBuilder(dict_type));
+ auto& builder =
checked_cast<DictionaryBuilder<FixedSizeBinaryType>&>(*boxed_builder);
+
+ ASSERT_OK(builder.Append("ab"));
+ ASSERT_OK(builder.Append("cd"));
+ ASSERT_OK(builder.Append("ab"));
+
+ std::shared_ptr<Array> result;
+ ASSERT_OK(builder.Finish(&result));
+ ASSERT_OK(result->ValidateFull());
+
+ const auto& result_type = checked_cast<const
DictionaryType&>(*result->type());
+ AssertTypeEqual(*uint16(), *result_type.index_type());
+}
+
+TEST(TestDictionaryBuilderIndexType, OrderedAndUnsignedIndexType) {
+ // The ordered flag (GH-49689) and the index type must both survive.
Review Comment:
Is it useful? Do we expect potential issues here?
##########
cpp/src/arrow/array/builder_dict.h:
##########
@@ -134,6 +134,33 @@ class ARROW_EXPORT DictionaryMemoTable {
namespace internal {
+/// \brief Return the unsigned integer type of the same width when an unsigned
+/// dictionary index type was requested.
+///
+/// The adaptive indices builder only ever produces signed integer types.
Dictionary
+/// indices are non-negative, so the signed and unsigned integer types of a
given
+/// width have identical memory layout and reporting one as the other is
+/// value-preserving (GH-37476). The width itself stays adaptive, exactly as
it is
+/// for signed index types; only the signedness of the requested type is
preserved.
Review Comment:
Well, yes, except that we're missing 1 bit of information in the unsigned
index type. Not necessarily a deal-breaker, but a bit of a pity.
##########
cpp/src/arrow/array/builder_dict.h:
##########
@@ -154,14 +181,16 @@ class DictionaryBuilderBase : public ArrayBuilder {
const std::shared_ptr<DataType>&>
value_type,
MemoryPool* pool = default_memory_pool(),
- int64_t alignment = kDefaultBufferAlignment, bool
ordered = false)
+ int64_t alignment = kDefaultBufferAlignment, bool
ordered = false,
+ bool unsigned_index = false)
Review Comment:
Can we call this `use_unsigned_index`?
##########
python/pyarrow/tests/test_array.py:
##########
@@ -4527,3 +4527,48 @@ def test_dunders_checked_overflow():
arr ** pa.scalar(2, type=pa.int8())
with pytest.raises(pa.ArrowInvalid, match=error_match):
arr / (-arr)
+
+
[email protected]("index_type", [pa.uint8(), pa.uint16(),
+ pa.uint32(), pa.uint64()])
+def test_dictionary_array_preserves_unsigned_index_type(index_type):
+ # GH-37476: an unsigned dictionary index type used to be silently replaced
+ # by the signed integer type of the same width.
+ dict_type = pa.dictionary(index_type, pa.string())
+
+ arr = pa.array(["a", "b", None, "a"], type=dict_type)
+ assert arr.type == dict_type
+ assert arr.to_pylist() == ["a", "b", None, "a"]
+ arr.validate(full=True)
+
+ chunked = pa.chunked_array([["a", "b", "a"]], dict_type)
+ assert chunked.type == dict_type
+
+
[email protected]("index_type", [pa.int8(), pa.int16(),
+ pa.int32(), pa.int64()])
+def test_dictionary_array_signed_index_type_unchanged(index_type):
Review Comment:
Can you fold this into the above test?
##########
python/pyarrow/tests/test_array.py:
##########
@@ -4527,3 +4527,48 @@ def test_dunders_checked_overflow():
arr ** pa.scalar(2, type=pa.int8())
with pytest.raises(pa.ArrowInvalid, match=error_match):
arr / (-arr)
+
+
[email protected]("index_type", [pa.uint8(), pa.uint16(),
+ pa.uint32(), pa.uint64()])
+def test_dictionary_array_preserves_unsigned_index_type(index_type):
+ # GH-37476: an unsigned dictionary index type used to be silently replaced
+ # by the signed integer type of the same width.
+ dict_type = pa.dictionary(index_type, pa.string())
+
+ arr = pa.array(["a", "b", None, "a"], type=dict_type)
+ assert arr.type == dict_type
+ assert arr.to_pylist() == ["a", "b", None, "a"]
+ arr.validate(full=True)
+
+ chunked = pa.chunked_array([["a", "b", "a"]], dict_type)
+ assert chunked.type == dict_type
+
+
[email protected]("index_type", [pa.int8(), pa.int16(),
+ pa.int32(), pa.int64()])
+def test_dictionary_array_signed_index_type_unchanged(index_type):
+ dict_type = pa.dictionary(index_type, pa.string())
+ arr = pa.array(["a", "b", "a"], type=dict_type)
+ assert arr.type == dict_type
+
+
+def test_dictionary_array_index_width_still_adapts():
+ # The index width remains adaptive, as it is for signed index types; only
+ # the signedness of the requested index type is preserved.
Review Comment:
Same comment wrt comments being too much context-dependent ("still adapts",
"remains adaptive").
Also, can we check both signed and unsigned here?
##########
cpp/src/arrow/array/array_dict_test.cc:
##########
@@ -1040,6 +1040,177 @@ TYPED_TEST(TestDictionaryBuilderIndexByteWidth,
MakeBuilder) {
AssertIndexByteWidth<TypeParam, NullType>();
}
+// ----------------------------------------------------------------------
+// GH-37476: the requested index type's signedness must be preserved
+
+TEST(TestDictionaryBuilderIndexType, TypePreservesUnsignedIndexType) {
+ for (auto index_type : {uint8(), uint16(), uint32(), uint64()}) {
+ ARROW_SCOPED_TRACE("index_type = ", index_type->ToString());
+ auto dict_type = dictionary(index_type, utf8());
+ ASSERT_OK_AND_ASSIGN(auto boxed_builder, MakeBuilder(dict_type));
+
+ auto builder_type = boxed_builder->type();
+ const auto& dict_builder_type = checked_cast<const
DictionaryType&>(*builder_type);
+ AssertTypeEqual(*index_type, *dict_builder_type.index_type());
+ }
+}
+
+TEST(TestDictionaryBuilderIndexType, TypePreservesSignedIndexType) {
+ for (auto index_type : {int8(), int16(), int32(), int64()}) {
+ ARROW_SCOPED_TRACE("index_type = ", index_type->ToString());
+ auto dict_type = dictionary(index_type, utf8());
+ ASSERT_OK_AND_ASSIGN(auto boxed_builder, MakeBuilder(dict_type));
+
+ auto builder_type = boxed_builder->type();
+ const auto& dict_builder_type = checked_cast<const
DictionaryType&>(*builder_type);
+ AssertTypeEqual(*index_type, *dict_builder_type.index_type());
+ }
+}
+
+TEST(TestDictionaryBuilderIndexType, FinishPreservesUnsignedIndexType) {
+ for (auto index_type : {uint8(), uint16(), uint32(), uint64()}) {
+ ARROW_SCOPED_TRACE("index_type = ", index_type->ToString());
+ auto dict_type = dictionary(index_type, utf8());
+ ASSERT_OK_AND_ASSIGN(auto boxed_builder, MakeBuilder(dict_type));
+ auto& builder =
checked_cast<DictionaryBuilder<StringType>&>(*boxed_builder);
+
+ ASSERT_OK(builder.Append("a"));
+ ASSERT_OK(builder.Append("b"));
+ ASSERT_OK(builder.AppendNull());
+ ASSERT_OK(builder.Append("a"));
+
+ std::shared_ptr<Array> result;
+ ASSERT_OK(builder.Finish(&result));
+ ASSERT_OK(result->ValidateFull());
+
+ auto ex_dict = ArrayFromJSON(utf8(), R"(["a", "b"])");
+ auto ex_indices = ArrayFromJSON(index_type, "[0, 1, null, 0]");
+ DictionaryArray expected(dict_type, ex_indices, ex_dict);
+ AssertTypeEqual(*dict_type, *result->type());
+ AssertArraysEqual(expected, *result);
+ }
+}
+
+TEST(TestDictionaryBuilderIndexType, UnsignedIndexWidthStillAdapts) {
+ // The index width remains adaptive, exactly as it is for signed index types:
+ // only the signedness of the requested type is preserved.
+ auto dict_type = dictionary(uint8(), utf8());
+ ASSERT_OK_AND_ASSIGN(auto boxed_builder, MakeBuilder(dict_type));
+ auto& builder = checked_cast<DictionaryBuilder<StringType>&>(*boxed_builder);
+
+ // More distinct values than a uint8 index start width accommodates.
+ for (int i = 0; i < 200; ++i) {
+ ASSERT_OK(builder.Append(std::to_string(i)));
+ }
+
+ std::shared_ptr<Array> result;
+ ASSERT_OK(builder.Finish(&result));
+ ASSERT_OK(result->ValidateFull());
+
+ const auto& result_type = checked_cast<const
DictionaryType&>(*result->type());
+ // Widened, but still unsigned rather than falling back to a signed type.
+ AssertTypeEqual(*uint16(), *result_type.index_type());
+}
+
+TEST(TestDictionaryBuilderIndexType, NullValueTypeUnsignedIndexType) {
+ auto dict_type = dictionary(uint32(), null());
+ ASSERT_OK_AND_ASSIGN(auto boxed_builder, MakeBuilder(dict_type));
+ auto& builder = checked_cast<DictionaryBuilder<NullType>&>(*boxed_builder);
+
+ ASSERT_OK(builder.AppendNull());
+
+ std::shared_ptr<Array> result;
+ ASSERT_OK(builder.Finish(&result));
+ ASSERT_OK(result->ValidateFull());
+
+ const auto& result_type = checked_cast<const
DictionaryType&>(*result->type());
+ AssertTypeEqual(*uint32(), *result_type.index_type());
+}
+
+TEST(TestDictionaryBuilderIndexType, FinishDeltaPreservesUnsignedIndexType) {
+ auto dict_type = dictionary(uint32(), utf8());
+ ASSERT_OK_AND_ASSIGN(auto boxed_builder, MakeBuilder(dict_type));
+ auto& builder = checked_cast<DictionaryBuilder<StringType>&>(*boxed_builder);
+
+ ASSERT_OK(builder.Append("a"));
+ ASSERT_OK(builder.Append("b"));
+
+ std::shared_ptr<Array> result_indices, result_delta;
+ ASSERT_OK(builder.FinishDelta(&result_indices, &result_delta));
+ ASSERT_OK(result_indices->ValidateFull());
+
+ // The delta indices must carry the requested index type too, not the signed
+ // type that the adaptive indices builder produces internally.
+ AssertTypeEqual(*uint32(), *result_indices->type());
+ AssertArraysEqual(*ArrayFromJSON(uint32(), "[0, 1]"), *result_indices);
+}
+
+TEST(TestDictionaryBuilderIndexType,
SuppliedDictionaryPreservesUnsignedIndexType) {
+ // The supplied-dictionary constructor starts the adaptive indices builder
at its
+ // default width rather than at the requested one, so the requested *width*
is not
+ // honoured on this path. That predates this change: a requested int32
reports int8
+ // too. What must hold is that the *signedness* survives, i.e. uint8 and not
int8.
+ auto dict_values = ArrayFromJSON(utf8(), R"(["a", "b"])");
+
+ ASSERT_OK_AND_ASSIGN(auto unsigned_builder,
+ MakeDictionaryBuilder(dictionary(uint32(), utf8()),
dict_values));
+ auto unsigned_type = unsigned_builder->type();
+ AssertTypeEqual(*uint8(),
+ *checked_cast<const
DictionaryType&>(*unsigned_type).index_type());
+
+ ASSERT_OK_AND_ASSIGN(auto signed_builder,
+ MakeDictionaryBuilder(dictionary(int32(), utf8()),
dict_values));
+ auto signed_type = signed_builder->type();
+ AssertTypeEqual(*int8(),
+ *checked_cast<const
DictionaryType&>(*signed_type).index_type());
+}
+
+TEST(TestDictionaryBuilderIndexType, FixedSizeBinaryUnsignedIndexType) {
+ auto dict_type = dictionary(uint16(), fixed_size_binary(2));
+ ASSERT_OK_AND_ASSIGN(auto boxed_builder, MakeBuilder(dict_type));
+ auto& builder =
checked_cast<DictionaryBuilder<FixedSizeBinaryType>&>(*boxed_builder);
+
+ ASSERT_OK(builder.Append("ab"));
+ ASSERT_OK(builder.Append("cd"));
+ ASSERT_OK(builder.Append("ab"));
+
+ std::shared_ptr<Array> result;
+ ASSERT_OK(builder.Finish(&result));
+ ASSERT_OK(result->ValidateFull());
+
+ const auto& result_type = checked_cast<const
DictionaryType&>(*result->type());
+ AssertTypeEqual(*uint16(), *result_type.index_type());
+}
+
+TEST(TestDictionaryBuilderIndexType, OrderedAndUnsignedIndexType) {
+ // The ordered flag (GH-49689) and the index type must both survive.
+ auto dict_type = dictionary(uint32(), utf8(), /*ordered=*/true);
+ ASSERT_OK_AND_ASSIGN(auto boxed_builder, MakeBuilder(dict_type));
+ auto& builder = checked_cast<DictionaryBuilder<StringType>&>(*boxed_builder);
+
+ ASSERT_OK(builder.Append("a"));
+ ASSERT_OK(builder.Append("b"));
+
+ std::shared_ptr<Array> result;
+ ASSERT_OK(builder.Finish(&result));
+ ASSERT_OK(result->ValidateFull());
+
+ const auto& result_type = checked_cast<const
DictionaryType&>(*result->type());
+ AssertTypeEqual(*uint32(), *result_type.index_type());
+ ASSERT_TRUE(result_type.ordered());
+}
+
+TEST(TestDictionaryBuilderIndexType, ExactIndexTypeStillHonoursUnsigned) {
+ // The exact-index builder already honoured unsigned index types; guard it.
Review Comment:
"Still" and "already" are context-dependent, reword this?
##########
cpp/src/arrow/array/array_dict_test.cc:
##########
@@ -1040,6 +1040,177 @@ TYPED_TEST(TestDictionaryBuilderIndexByteWidth,
MakeBuilder) {
AssertIndexByteWidth<TypeParam, NullType>();
}
+// ----------------------------------------------------------------------
+// GH-37476: the requested index type's signedness must be preserved
+
+TEST(TestDictionaryBuilderIndexType, TypePreservesUnsignedIndexType) {
+ for (auto index_type : {uint8(), uint16(), uint32(), uint64()}) {
+ ARROW_SCOPED_TRACE("index_type = ", index_type->ToString());
+ auto dict_type = dictionary(index_type, utf8());
+ ASSERT_OK_AND_ASSIGN(auto boxed_builder, MakeBuilder(dict_type));
+
+ auto builder_type = boxed_builder->type();
+ const auto& dict_builder_type = checked_cast<const
DictionaryType&>(*builder_type);
+ AssertTypeEqual(*index_type, *dict_builder_type.index_type());
+ }
+}
+
+TEST(TestDictionaryBuilderIndexType, TypePreservesSignedIndexType) {
+ for (auto index_type : {int8(), int16(), int32(), int64()}) {
+ ARROW_SCOPED_TRACE("index_type = ", index_type->ToString());
+ auto dict_type = dictionary(index_type, utf8());
+ ASSERT_OK_AND_ASSIGN(auto boxed_builder, MakeBuilder(dict_type));
+
+ auto builder_type = boxed_builder->type();
+ const auto& dict_builder_type = checked_cast<const
DictionaryType&>(*builder_type);
+ AssertTypeEqual(*index_type, *dict_builder_type.index_type());
+ }
+}
+
+TEST(TestDictionaryBuilderIndexType, FinishPreservesUnsignedIndexType) {
+ for (auto index_type : {uint8(), uint16(), uint32(), uint64()}) {
+ ARROW_SCOPED_TRACE("index_type = ", index_type->ToString());
+ auto dict_type = dictionary(index_type, utf8());
+ ASSERT_OK_AND_ASSIGN(auto boxed_builder, MakeBuilder(dict_type));
+ auto& builder =
checked_cast<DictionaryBuilder<StringType>&>(*boxed_builder);
+
+ ASSERT_OK(builder.Append("a"));
+ ASSERT_OK(builder.Append("b"));
+ ASSERT_OK(builder.AppendNull());
+ ASSERT_OK(builder.Append("a"));
+
+ std::shared_ptr<Array> result;
+ ASSERT_OK(builder.Finish(&result));
+ ASSERT_OK(result->ValidateFull());
+
+ auto ex_dict = ArrayFromJSON(utf8(), R"(["a", "b"])");
+ auto ex_indices = ArrayFromJSON(index_type, "[0, 1, null, 0]");
+ DictionaryArray expected(dict_type, ex_indices, ex_dict);
+ AssertTypeEqual(*dict_type, *result->type());
+ AssertArraysEqual(expected, *result);
+ }
+}
+
+TEST(TestDictionaryBuilderIndexType, UnsignedIndexWidthStillAdapts) {
+ // The index width remains adaptive, exactly as it is for signed index types:
+ // only the signedness of the requested type is preserved.
+ auto dict_type = dictionary(uint8(), utf8());
+ ASSERT_OK_AND_ASSIGN(auto boxed_builder, MakeBuilder(dict_type));
+ auto& builder = checked_cast<DictionaryBuilder<StringType>&>(*boxed_builder);
+
+ // More distinct values than a uint8 index start width accommodates.
+ for (int i = 0; i < 200; ++i) {
+ ASSERT_OK(builder.Append(std::to_string(i)));
+ }
+
+ std::shared_ptr<Array> result;
+ ASSERT_OK(builder.Finish(&result));
+ ASSERT_OK(result->ValidateFull());
+
+ const auto& result_type = checked_cast<const
DictionaryType&>(*result->type());
+ // Widened, but still unsigned rather than falling back to a signed type.
+ AssertTypeEqual(*uint16(), *result_type.index_type());
+}
+
+TEST(TestDictionaryBuilderIndexType, NullValueTypeUnsignedIndexType) {
+ auto dict_type = dictionary(uint32(), null());
+ ASSERT_OK_AND_ASSIGN(auto boxed_builder, MakeBuilder(dict_type));
+ auto& builder = checked_cast<DictionaryBuilder<NullType>&>(*boxed_builder);
+
+ ASSERT_OK(builder.AppendNull());
+
+ std::shared_ptr<Array> result;
+ ASSERT_OK(builder.Finish(&result));
+ ASSERT_OK(result->ValidateFull());
+
+ const auto& result_type = checked_cast<const
DictionaryType&>(*result->type());
+ AssertTypeEqual(*uint32(), *result_type.index_type());
+}
+
+TEST(TestDictionaryBuilderIndexType, FinishDeltaPreservesUnsignedIndexType) {
+ auto dict_type = dictionary(uint32(), utf8());
+ ASSERT_OK_AND_ASSIGN(auto boxed_builder, MakeBuilder(dict_type));
+ auto& builder = checked_cast<DictionaryBuilder<StringType>&>(*boxed_builder);
+
+ ASSERT_OK(builder.Append("a"));
+ ASSERT_OK(builder.Append("b"));
+
+ std::shared_ptr<Array> result_indices, result_delta;
+ ASSERT_OK(builder.FinishDelta(&result_indices, &result_delta));
+ ASSERT_OK(result_indices->ValidateFull());
+
+ // The delta indices must carry the requested index type too, not the signed
+ // type that the adaptive indices builder produces internally.
+ AssertTypeEqual(*uint32(), *result_indices->type());
+ AssertArraysEqual(*ArrayFromJSON(uint32(), "[0, 1]"), *result_indices);
+}
+
+TEST(TestDictionaryBuilderIndexType,
SuppliedDictionaryPreservesUnsignedIndexType) {
+ // The supplied-dictionary constructor starts the adaptive indices builder
at its
+ // default width rather than at the requested one, so the requested *width*
is not
+ // honoured on this path. That predates this change: a requested int32
reports int8
Review Comment:
Also, if we already have a test for this on the signed index side, I wonder
what it brings to add this new test.
AIs tend to produce very verbose testing, which gives the illusion of better
quality, but also makes the code less maintainable in the long term (why all
those very similar test functions)?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
