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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
