GH-35815: [C++] Provided dictionary support for utf8_trim - #51134
Open
NathanChung4 wants to merge 2 commits into
Open
GH-35815: [C++] Provided dictionary support for utf8_trim#51134NathanChung4 wants to merge 2 commits into
NathanChung4 wants to merge 2 commits into
Conversation
What was broken: utf8_trim/utf8_ltrim/utf8_rtrim errored out on dictionary encoded string input instead of working because the functions only had kernels that registered for plain and not for dictionary so the dispatch didn't have anywhere to go. My fix was to add a small ScalarFunction subclass that unwraps a dictionary to its value type and tries again if no exact type kernel matches. I registered the three trim functions with this subclass instead of the default one. To test I added a dictionary input case to the existing TrimUTF8 test.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The new behavior for dictionary inputs is only partially covered by tests (missing ltrim/rtrim and the issue-reported int64 dictionary indices).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds dictionary-encoded input support for the UTF-8 trim compute functions by introducing a custom ScalarFunction dispatch path that unwraps dictionary types to their value type, allowing existing string kernels to run after implicit decoding/casting.
Changes:
- Introduce
UnaryStringWithDictionaryFunctionwith aDispatchBestoverride that retries dispatch afterEnsureDictionaryDecoded. - Switch
utf8_trim,utf8_ltrim, andutf8_rtrimto use the new function type viaMakeUnaryStringBatchKernelWithState. - Add a new test exercising dictionary input for
utf8_trim.
File summaries
| File | Description |
|---|---|
| cpp/src/arrow/compute/kernels/scalar_string_utf8.cc | Adds a ScalarFunction subclass to unwrap dictionary inputs during dispatch and applies it to UTF-8 trim functions. |
| cpp/src/arrow/compute/kernels/scalar_string_internal.h | Generalizes MakeUnaryStringBatchKernelWithState to allow constructing a custom ScalarFunction subclass. |
| cpp/src/arrow/compute/kernels/scalar_string_test.cc | Adds a regression test for dictionary input support in utf8_trim. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+2474
to
+2477
| this->CheckUnary("utf8_trim", | ||
| ArrayFromJSON(dictionary(int32(), this->type()), | ||
| R"(["azȺz矢ba", null, "bab", "zȺz"])"), | ||
| this->type(), R"(["zȺz矢", null, "", "zȺz"])", &options); |
Responding to Copilot's automated review on PR apache#51134. I added ltrim/rtrim dictionary cases and switched all three to int64 indices. Verified through arrow-compute-scalar-type-test with 253/253 tests still passing and the pre-commit being clean.
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The dispatch change follows an established pattern (EnsureDictionaryDecoded + redispatch) and is covered by new tests for dictionary-encoded inputs.
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
- Review effort level: Lite
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rationale for this change
utf8_trim, utf8_ltrim, and utf8_rtrim error out on dictionary encoded string arrays instead of working.
What changes are included in this PR?
A new ScalarFunction subclass with a DispatchBest mechanism that unwraps a dictionary
to its value type and retries dispatch, and the three trim functions (utf8_trim,
utf8_ltrim, utf8_rtrim) switched over to use it instead of the default.
Are these changes tested?
These changes are tested. arrow-compute-scalar-type-test (253 tests, including the
new one) passed, and I ran the entire arrow::compute test suite (15 test binaries)
to make sure nothing else broke. pre-commit (C++ Format + C++ Lint) came back clean.
Are there any user-facing changes?
Yes. The three trim functions now accept dictionary encoded string arrays instead
of raising NotImplemented.
AI Disclosure
Claude helped and guided me through most of the work: it helped trace the dispatch
mechanism, identified the existing pattern to follow (from scalar_compare.cc), wrote
the implementation and the added test case, and ran the builds and test suites. Claude
also walked me through the mechanism until I understood it. I decided to scope this
to just the three trim functions rather than the whole file, reviewed the diff, and
wrote the commit message and this PR description myself.