You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "mkzung (via GitHub)" <gi...@apache.org> on 2026/07/11 15:17:29 UTC
[PR] GH-37476: [C++][Python] Preserve unsigned dictionary index types when building from values [arrow]
mkzung opened a new pull request, #50475:
URL: https://github.com/apache/arrow/pull/50475
### Rationale for this change
An unsigned dictionary index type is silently replaced by the signed one of the same width:
```python
>>> pa.array(["a", "b"], type=pa.dictionary(pa.uint32(), pa.string())).type
dictionary<values=string, indices=int32, ordered=0>
```
It also makes `pa.chunked_array(values, dict_type)` fail with `ArrowTypeError: Array chunks must all be same type`, because the chunk it builds internally comes back with a different type than the one requested.
The cause is in `DictionaryBuilderCase::CreateFor()` in `builder.cc`. The requested index type is reduced to `index_type->byte_width()` and handed to `AdaptiveIntBuilder` as a starting width, so everything except the width is dropped, and the dictionary type is then rebuilt from whatever the indices builder reports. That builder only ever reports signed types.
@jorisvandenbossche suggested using `AdaptiveUIntBuilder` for unsigned index types. I tried that first and it does not work: it makes `MakeDictionaryBuilder` return a different builder class, and `util/converter.h`, `json/from_string.cc` and the R binding all cast the result to `DictionaryBuilder<T>`. `TestDictionaryUnifier.ChunkedArrayNestedDict` dies on a `checked_pointer_cast` DCHECK as soon as you try it.
So this keeps one builder class and preserves the requested signedness where the index type is reported. Indices are non-negative and the signed and unsigned types of a given width have the same layout, so reporting one as the other is value-preserving and free. The width stays adaptive, as it is for signed index types.
### What changes are included in this PR?
`DictionaryBuilderBase` and its `NullType` specialization record whether an unsigned index type was requested and map the indices builder's signed type to the unsigned one of the same width in `type()`, `FinishInternal()` and `FinishDelta()`. `CreateFor()` passes the signedness through instead of discarding it.
`util/converter.h`, `python_to_arrow.cc` and `r_to_arrow.cpp` are not touched, so R is fixed rather than broken.
### Are these changes tested?
Ten tests in `array_dict_test.cc` and four parametrized ones in `test_array.py`, covering all four unsigned index types, a signed regression guard, nulls, `FinishDelta`, the supplied-dictionary constructor, fixed-size-binary values, `ordered` together with an unsigned index, and the exact-index builder. Eight of the ten C++ tests fail without the change.
The width still adapts: a requested `uint8` grows to `uint16` and then `uint32` as the dictionary grows, and stays unsigned at each step.
`arrow-array-test` passes (1056), as do `arrow-compute-scalar-cast-test` and `arrow-c-bridge-test`, and the pyarrow suites are unchanged from main.
One thing I noticed while in here: the supplied-dictionary constructor starts the indices builder at its default width rather than the requested one, so the requested width is dropped on that path. That is not new, a requested `int32` reports `int8` there on main too. This preserves the signedness there and leaves the width alone.
### Are there any user-facing changes?
`pa.array` and `pa.chunked_array` now return the unsigned index type that was requested.
One consequence is worth flagging. `dictionary(uint64(), ...)` now genuinely produces uint64 indices, and `arrow_to_pandas.cc` deliberately refuses to convert those ("Converting UInt64 dictionary indices to pandas is not supported", since pandas categorical codes are signed). So `to_pandas()` on such an array raises now, where before it worked by accident on int32 indices. uint8, uint16 and uint32 are unaffected. I think that is the bug surfacing rather than a new one, but adding uint64 support to the pandas conversion may be worth a follow-up, and there is a test pinning the current behaviour.
I checked the rest of the surface for every unsigned width: `take`, `filter`, `cast`, `dictionary_decode`, IPC, a Parquet write and read back, the compute kernels and an Acero `group_by` all work and keep the index type. Pandas conversion of uint64 is the only thing that changes.
### AI usage
I used an AI coding assistant while working on this, to help track down the root cause and to draft tests. I built Arrow and pyarrow from source, ran the C++ and Python suites and the lint hooks myself, and I understand and take responsibility for every line.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37476: [C++][Python] Preserve unsigned dictionary index types when building from values [arrow]
Posted by "mkzung (via GitHub)" <gi...@apache.org>.
mkzung commented on PR #50475:
URL: https://github.com/apache/arrow/pull/50475#issuecomment-5071370165
Thanks for the review. I folded the index-type tests down to the distinct code paths - type and finish preservation now run in one loop over signed and unsigned, and I dropped the ordered and fixed-size-binary ones since they don't exercise a separate path. Switched to ASSERT_OK_AND_ASSIGN and reworded the context-dependent comments. Moved MaybeUnsignedIndexType into builder.cc and renamed the flag to use_unsigned_index.
On uint64 to pandas: I added support for it instead of keeping the error. uint64 indices map to int64 codes, which is safe because the indices are bounds-checked below the dictionary length, so they never reach the int64 limit. WriteIndicesUniform handles it the same way as uint32, and I updated test_dictionary_with_pandas to check the conversion instead of the raise.
On the missing top bit: you're right, the adaptive builder is signed so a uint8 index widens after 128 rather than 256. I left that as-is and noted it in the builder.cc comment - happy to change the builder to track the unsigned range if you'd prefer.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37476: [C++][Python] Preserve unsigned dictionary index types when building from values [arrow]
Posted by "mkzung (via GitHub)" <gi...@apache.org>.
mkzung commented on PR #50475:
URL: https://github.com/apache/arrow/pull/50475#issuecomment-5106270549
Thanks, done:
- `MaybeUnsignedIndexType`: the default case is unreachable (the adaptive builder only produces
signed int8/16/32/64), so it's `arrow::Unreachable` now.
- Both tests now build the array and check the finished array's index type, not just the
builder's.
- Renamed to `WidthAdaptsWhenUnsigned`.
The uint8-widens-at-128 bit is still a documented limitation; happy to do the unsigned-range
follow-up separately if you want it. The red R/Windows checks look like infra, not this change
(bioconductor was down for R, MinGW timed out in the deps).
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37476: [C++][Python] Preserve unsigned dictionary index types when building from values [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #50475:
URL: https://github.com/apache/arrow/pull/50475#discussion_r3658535879
##########
cpp/src/arrow/builder.cc:
##########
@@ -38,6 +38,38 @@ class MemoryPool;
using arrow::internal::checked_cast;
+namespace internal {
+
+/// Return the unsigned integer type of the same width when an unsigned dictionary
+/// index type was requested (GH-37476).
+///
+/// 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. The
+/// width stays adaptive, as it is for signed index types, and it widens on the signed
+/// threshold: a uint8 index widens after 128 distinct values rather than the 256 a real
+/// uint8 could hold, so the extra bit does not delay widening.
+std::shared_ptr<DataType> MaybeUnsignedIndexType(
+ const std::shared_ptr<DataType>& index_type, bool use_unsigned_index) {
+ if (!use_unsigned_index) {
+ return index_type;
+ }
+ switch (index_type->id()) {
+ case Type::INT8:
+ return ::arrow::uint8();
+ case Type::INT16:
+ return ::arrow::uint16();
+ case Type::INT32:
+ return ::arrow::uint32();
+ case Type::INT64:
+ return ::arrow::uint64();
+ default:
+ return index_type;
Review Comment:
Can we use `arrow::Unreachable` here? Or can the `index_type` argument actually be unsigned?
##########
cpp/src/arrow/array/array_dict_test.cc:
##########
@@ -1040,6 +1040,110 @@ TYPED_TEST(TestDictionaryBuilderIndexByteWidth, MakeBuilder) {
AssertIndexByteWidth<TypeParam, NullType>();
}
+// ----------------------------------------------------------------------
+// GH-37476: the requested dictionary index type's signedness must be preserved.
+
+TEST(TestDictionaryBuilderIndexType, PreservesRequestedIndexType) {
+ // Both the builder's reported type and the finished array must carry the requested
+ // index type, for signed and unsigned widths alike.
+ for (auto index_type :
+ {int8(), int16(), int32(), int64(), uint8(), uint16(), uint32(), uint64()}) {
+ ARROW_SCOPED_TRACE("index_type = ", index_type->ToString());
+ auto dict_type = dictionary(index_type, utf8());
+ ASSERT_OK_AND_ASSIGN(auto builder, MakeBuilder(dict_type));
+ AssertTypeEqual(*index_type,
+ *checked_cast<const DictionaryType&>(*builder->type()).index_type());
+
+ auto& dict_builder = checked_cast<DictionaryBuilder<StringType>&>(*builder);
+ ASSERT_OK(dict_builder.Append("a"));
+ ASSERT_OK(dict_builder.Append("b"));
+ ASSERT_OK(dict_builder.AppendNull());
+ ASSERT_OK(dict_builder.Append("a"));
+ ASSERT_OK_AND_ASSIGN(auto result, dict_builder.Finish());
+ 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, WidthStillAdaptsWhenUnsigned) {
+ // The width stays adaptive, as it does for signed indices. The underlying builder is
+ // signed, so it widens after 128 distinct values rather than the 256 a uint8 could
+ // hold, but the widened type stays unsigned rather than falling back to a signed type.
+ auto dict_type = dictionary(uint8(), utf8());
+ ASSERT_OK_AND_ASSIGN(auto boxed_builder, MakeBuilder(dict_type));
+ auto& builder = checked_cast<DictionaryBuilder<StringType>&>(*boxed_builder);
+
+ for (int i = 0; i < 200; ++i) {
+ ASSERT_OK(builder.Append(std::to_string(i)));
+ }
+
+ ASSERT_OK_AND_ASSIGN(auto result, builder.Finish());
+ ASSERT_OK(result->ValidateFull());
+ AssertTypeEqual(*uint16(),
+ *checked_cast<const DictionaryType&>(*result->type()).index_type());
+}
+
+TEST(TestDictionaryBuilderIndexType, NullValueTypePreservesUnsignedIndexType) {
+ // The NullType value builder is a separate specialization with its own type() and
+ // FinishInternal, so it needs its own guard.
+ auto dict_type = dictionary(uint32(), null());
+ ASSERT_OK_AND_ASSIGN(auto boxed_builder, MakeBuilder(dict_type));
+ auto& builder = checked_cast<DictionaryBuilder<NullType>&>(*boxed_builder);
+ AssertTypeEqual(*uint32(),
+ *checked_cast<const DictionaryType&>(*builder.type()).index_type());
+
+ ASSERT_OK(builder.AppendNull());
+ ASSERT_OK_AND_ASSIGN(auto result, builder.Finish());
+ ASSERT_OK(result->ValidateFull());
+ AssertTypeEqual(*uint32(),
+ *checked_cast<const DictionaryType&>(*result->type()).index_type());
+}
+
+TEST(TestDictionaryBuilderIndexType, FinishDeltaPreservesUnsignedIndexType) {
+ // FinishDelta is a distinct path from Finish and must carry the requested index type
+ // too, not the signed type the adaptive builder produces internally.
+ 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());
+ AssertTypeEqual(*uint32(), *result_indices->type());
+ AssertArraysEqual(*ArrayFromJSON(uint32(), "[0, 1]"), *result_indices);
+}
+
+TEST(TestDictionaryBuilderIndexType, SuppliedDictionaryPreservesUnsignedIndexType) {
+ // The supplied-dictionary constructor starts the adaptive builder at its default width
+ // rather than the requested one, so a requested uint32 reports uint8. The width is not
+ // honoured on this path (a signed request behaves the same way), but the signedness
+ // must survive regardless: uint8, not int8.
+ auto dict_values = ArrayFromJSON(utf8(), R"(["a", "b"])");
+ ASSERT_OK_AND_ASSIGN(auto builder,
+ MakeDictionaryBuilder(dictionary(uint32(), utf8()), dict_values));
+ AssertTypeEqual(*uint8(),
+ *checked_cast<const DictionaryType&>(*builder->type()).index_type());
Review Comment:
Can you build an array and check that built array retains the expected index type?
##########
cpp/src/arrow/array/array_dict_test.cc:
##########
@@ -1040,6 +1040,110 @@ TYPED_TEST(TestDictionaryBuilderIndexByteWidth, MakeBuilder) {
AssertIndexByteWidth<TypeParam, NullType>();
}
+// ----------------------------------------------------------------------
+// GH-37476: the requested dictionary index type's signedness must be preserved.
+
+TEST(TestDictionaryBuilderIndexType, PreservesRequestedIndexType) {
+ // Both the builder's reported type and the finished array must carry the requested
+ // index type, for signed and unsigned widths alike.
+ for (auto index_type :
+ {int8(), int16(), int32(), int64(), uint8(), uint16(), uint32(), uint64()}) {
+ ARROW_SCOPED_TRACE("index_type = ", index_type->ToString());
+ auto dict_type = dictionary(index_type, utf8());
+ ASSERT_OK_AND_ASSIGN(auto builder, MakeBuilder(dict_type));
+ AssertTypeEqual(*index_type,
+ *checked_cast<const DictionaryType&>(*builder->type()).index_type());
+
+ auto& dict_builder = checked_cast<DictionaryBuilder<StringType>&>(*builder);
+ ASSERT_OK(dict_builder.Append("a"));
+ ASSERT_OK(dict_builder.Append("b"));
+ ASSERT_OK(dict_builder.AppendNull());
+ ASSERT_OK(dict_builder.Append("a"));
+ ASSERT_OK_AND_ASSIGN(auto result, dict_builder.Finish());
+ 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, WidthStillAdaptsWhenUnsigned) {
+ // The width stays adaptive, as it does for signed indices. The underlying builder is
+ // signed, so it widens after 128 distinct values rather than the 256 a uint8 could
+ // hold, but the widened type stays unsigned rather than falling back to a signed type.
+ auto dict_type = dictionary(uint8(), utf8());
+ ASSERT_OK_AND_ASSIGN(auto boxed_builder, MakeBuilder(dict_type));
+ auto& builder = checked_cast<DictionaryBuilder<StringType>&>(*boxed_builder);
+
+ for (int i = 0; i < 200; ++i) {
+ ASSERT_OK(builder.Append(std::to_string(i)));
+ }
+
+ ASSERT_OK_AND_ASSIGN(auto result, builder.Finish());
+ ASSERT_OK(result->ValidateFull());
+ AssertTypeEqual(*uint16(),
+ *checked_cast<const DictionaryType&>(*result->type()).index_type());
+}
+
+TEST(TestDictionaryBuilderIndexType, NullValueTypePreservesUnsignedIndexType) {
+ // The NullType value builder is a separate specialization with its own type() and
+ // FinishInternal, so it needs its own guard.
+ auto dict_type = dictionary(uint32(), null());
+ ASSERT_OK_AND_ASSIGN(auto boxed_builder, MakeBuilder(dict_type));
+ auto& builder = checked_cast<DictionaryBuilder<NullType>&>(*boxed_builder);
+ AssertTypeEqual(*uint32(),
+ *checked_cast<const DictionaryType&>(*builder.type()).index_type());
+
+ ASSERT_OK(builder.AppendNull());
+ ASSERT_OK_AND_ASSIGN(auto result, builder.Finish());
+ ASSERT_OK(result->ValidateFull());
+ AssertTypeEqual(*uint32(),
+ *checked_cast<const DictionaryType&>(*result->type()).index_type());
+}
+
+TEST(TestDictionaryBuilderIndexType, FinishDeltaPreservesUnsignedIndexType) {
+ // FinishDelta is a distinct path from Finish and must carry the requested index type
+ // too, not the signed type the adaptive builder produces internally.
+ 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());
+ AssertTypeEqual(*uint32(), *result_indices->type());
+ AssertArraysEqual(*ArrayFromJSON(uint32(), "[0, 1]"), *result_indices);
+}
+
+TEST(TestDictionaryBuilderIndexType, SuppliedDictionaryPreservesUnsignedIndexType) {
+ // The supplied-dictionary constructor starts the adaptive builder at its default width
+ // rather than the requested one, so a requested uint32 reports uint8. The width is not
+ // honoured on this path (a signed request behaves the same way), but the signedness
+ // must survive regardless: uint8, not int8.
+ auto dict_values = ArrayFromJSON(utf8(), R"(["a", "b"])");
+ ASSERT_OK_AND_ASSIGN(auto builder,
+ MakeDictionaryBuilder(dictionary(uint32(), utf8()), dict_values));
+ AssertTypeEqual(*uint8(),
+ *checked_cast<const DictionaryType&>(*builder->type()).index_type());
+}
+
+TEST(TestDictionaryBuilderIndexType, ExactIndexBuilderPreservesUnsignedIndexType) {
+ // MakeBuilderExactIndex is a separate builder that honours unsigned index types on its
+ // own; guard that it keeps doing so.
+ auto dict_type = dictionary(uint16(), utf8());
+ std::unique_ptr<ArrayBuilder> boxed_builder;
+ ASSERT_OK(MakeBuilderExactIndex(default_memory_pool(), dict_type, &boxed_builder));
+ AssertTypeEqual(
+ *uint16(),
+ *checked_cast<const DictionaryType&>(*boxed_builder->type()).index_type());
Review Comment:
Here as well, can you build an array and check that built array retains the expected index type?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37476: [C++][Python] Preserve unsigned dictionary index types when building from values [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #50475:
URL: https://github.com/apache/arrow/pull/50475#issuecomment-5107238704
The remaining CI failures are unrelated, I'll merge.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37476: [C++][Python] Preserve unsigned dictionary index types when building from values [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #50475:
URL: https://github.com/apache/arrow/pull/50475#issuecomment-5106700872
> The uint8-widens-at-128 bit is still a documented limitation; happy to do the unsigned-range
> follow-up separately if you want it.
It's probably desirable though not critical, so feel free to work on it if you'd like to.
> The uint8-widens-at-128 bit is still a documented limitation; happy to do the unsigned-range follow-up separately if you want it. The red R/Windows checks look like infra, not this change (bioconductor was down for R, MinGW timed out in the deps).
Probably, let's wait for the updated CI results.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37476: [C++][Python] Preserve unsigned dictionary index types when building from values [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou merged PR #50475:
URL: https://github.com/apache/arrow/pull/50475
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37476: [C++][Python] Preserve unsigned dictionary index types when building from values [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #50475:
URL: https://github.com/apache/arrow/pull/50475#issuecomment-5129336841
After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit bda98c4d8c4dbf52c6b52186e07a7bb0578c0ac4.
There were no benchmark performance regressions. 🎉
The [full Conbench report](https://github.com/apache/arrow/runs/90838613116) has more details. It also includes information about 20 possible false positives for unstable benchmarks that are known to sometimes produce them.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37476: [C++][Python] Preserve unsigned dictionary index types when building from values [arrow]
Posted by "mkzung (via GitHub)" <gi...@apache.org>.
mkzung commented on PR #50475:
URL: https://github.com/apache/arrow/pull/50475#issuecomment-5029457220
Friendly ping on this one. The workflow runs look like they're still waiting on
approval, so none of the C++ or Python jobs have actually executed here yet.
@jorisvandenbossche you diagnosed this back in 2023 and suggested letting it pick
AdaptiveIntBuilder vs AdaptiveUIntBuilder depending on the signedness of the original
index type, so you may want to see why I didn't go that way. It changes which class
MakeDictionaryBuilder returns, and util/converter.h, json/from_string.cc and the R
binding all cast that result to DictionaryBuilder<T>.
TestDictionaryUnifier.ChunkedArrayNestedDict trips a checked_pointer_cast DCHECK
straight away. This keeps one builder class instead and preserves the requested
signedness where the index type is reported.
Happy to rebase if it's gone stale.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37476: [C++][Python] Preserve unsigned dictionary index types when building from values [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
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)
+
+
+@pytest.mark.parametrize("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
+
+
+@pytest.mark.parametrize("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
+
+
+@pytest.mark.pandas
+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)
+
+
+@pytest.mark.parametrize("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
+
+
+@pytest.mark.parametrize("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)
+
+
+@pytest.mark.parametrize("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
+
+
+@pytest.mark.parametrize("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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org