GH-40128: [C++][Python] Preserve dictionaries in empty casts - #50837
GH-40128: [C++][Python] Preserve dictionaries in empty casts#50837LarryHu0217 wants to merge 2 commits into
Conversation
|
Fix looks narrowly scoped and properly tested, thank you @LarryHu0217! |
|
@uros-b We like to have PR reviews as much as possible, but since you are not a contributor to this codebase I wonder if it wouldn't be more productive to start contributing (assuming you're interested in doing so, of course). In general, I would recommend exercising caution if you're not familiar with the codebase and feel the desire to post a PR review. |
| const bool changes_dictionary_type = | ||
| batch.num_values() == 1 && batch.values[0].type() != nullptr && | ||
| is_dictionary(batch.values[0].type()->id()) && | ||
| is_dictionary(output_type_.type->id()) && | ||
| !batch.values[0].type()->Equals(*output_type_.type); |
There was a problem hiding this comment.
This looks... weird. Why the type unequality condition?
Also, this will trigger on every scalar compute function, not just the cast function.
|
@zanmato1984 Could you perhaps take a look? I'm not sure how to make this focused enough to apply to the Cast function, not every other scalar function that takes a dict as input. (also it's not obvious we want to fix this, though it would be useful for some use cases) |
|
Thank you @pitrou! Makes sense, sorry for the noise here. I'll also continue monitoring this PR too, and see how it continues evolving with additional feedback / comments |
What changed
The generic zero-length scalar fast path creates an empty output dictionary and discards categories that are still attached to the input array. Other zero-length scalar operations continue to use the existing fast path.
Testing
cpp/build-40128-auto/debug/arrow-compute-scalar-cast-test(109 tests passed)clang-format --dry-run --Werroron both changed filesgit diff --checkAI assistance
I used Codex to help inspect the executor path, prepare the focused change, and draft the regression test. I reviewed the final diff, reproduced the failure before the fix, and ran the tests above locally.
Closes #40128