Skip to content

GH-35815: [C++][Compute] Support dictionary inputs in UTF-8 trim - #50501

Open
LarryHu0217 wants to merge 5 commits into
apache:mainfrom
LarryHu0217:codex/arrow-dictionary-utf8-trim-35815
Open

GH-35815: [C++][Compute] Support dictionary inputs in UTF-8 trim#50501
LarryHu0217 wants to merge 5 commits into
apache:mainfrom
LarryHu0217:codex/arrow-dictionary-utf8-trim-35815

Conversation

@LarryHu0217

@LarryHu0217 LarryHu0217 commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

Rationale for this change

Pure unary scalar functions produce the same result for repeated input values. For dictionary-encoded input, running the function against every logical row is therefore unnecessary: the function can run once against the dictionary values and the original indices can expand the result.

What changes are included in this PR?

  • Add a generic dictionary adapter for fixed-arity, pure unary ScalarFunctions.
  • Dispatch the underlying function against the dictionary value type, execute it on the dictionary values, then expand the transformed values with the original indices.
  • Only advertise the adapter when the dictionary value type has a matching underlying kernel; impure unary functions remain unsupported.
  • Add generic coverage for type-changing output, unique-value execution, unsupported value types, and impure functions.
  • Keep issue-specific regression coverage for utf8_trim, utf8_ltrim, and utf8_rtrim with dictionary-encoded string and large-string inputs, including nulls.

Are these changes tested?

  • arrow-compute-internals-test --gtest_brief=1 (73 passed)
  • arrow-compute-scalar-type-test --gtest_brief=1 (344 passed, 1 pre-existing disabled test)
  • C++ Debug build with warnings treated as errors
  • clang-format --dry-run --Werror on all touched C++ files

Are there any user-facing changes?

Yes. Pure unary scalar functions can now accept dictionary-encoded inputs whenever their dictionary value type has a matching kernel. Results retain the function's existing decoded output type, so no public API signature changes are introduced.

AI-assisted development

OpenAI Codex assisted with reproducing the issue, implementing the patch, and drafting the regression tests. The change was checked against Arrow's scalar execution and dictionary dispatch paths and validated with the commands above.

@github-actions

Copy link
Copy Markdown

⚠️ GitHub issue #35815 has been automatically assigned in GitHub to PR creator.

@github-actions github-actions Bot added awaiting review Awaiting review Component: C++ and removed awaiting review Awaiting review labels Jul 13, 2026
@LarryHu0217
LarryHu0217 marked this pull request as ready for review July 16, 2026 01:15
@LarryHu0217
LarryHu0217 requested a review from pitrou as a code owner July 16, 2026 01:15
@github-actions github-actions Bot added the awaiting review Awaiting review label Jul 16, 2026
@pitrou

pitrou commented Jul 22, 2026

Copy link
Copy Markdown
Member
  • Decode dictionary input types during dispatch for utf8_trim, utf8_ltrim, and utf8_rtrim.

This is wasteful, isn't it? The scalar function could just be applied to the dictionary values.

Also I don't understand why we would only tackle these three specific functions, while this could apply to all scalar unary functions.

cc @zanmato1984 for opinions.

@LarryHu0217

Copy link
Copy Markdown
Contributor Author

Thanks, addressed both points in d2f3763. The trim-specific dispatch subclass is gone. The new adapter applies generically to fixed-arity pure unary ScalarFunctions when the dictionary value type has a matching kernel, executes the function against the dictionary values, and expands the result with the original indices while retaining the existing decoded output type. I added generic tests for type-changing output, unique-value execution, unsupported value types, and impure functions, plus the trim regressions. Local results: 73/73 compute-internals tests and 344/344 scalar-type tests passed.

@github-actions

Copy link
Copy Markdown

⚠️ GitHub issue #35815 has been automatically assigned in GitHub to PR creator.

@pitrou

pitrou commented Jul 27, 2026

Copy link
Copy Markdown
Member

@zanmato1984 What do you think about this? I don't know if this should be handled at the function execution level like in this PR, or somewhere else.

@LarryHu0217

Copy link
Copy Markdown
Contributor Author

The failed C++ jobs exposed that registering a synthetic dictionary kernel changed the public dispatch contract for every pure unary ScalarFunction, which also affected cast and expression behavior. I moved the generic support to the eager ScalarFunction::Execute path in fdfa405.

Existing dictionary kernels still take precedence. When no dictionary kernel matches, a fixed-arity pure unary function is applied to the dictionary values and expanded through the original indices. The fallback covers array, scalar, chunked-array, and empty-chunked inputs while leaving DispatchBest/DispatchExact unchanged.

This restores the previously failing TestUnaryArithmetic.DispatchBest, TestUnaryRound.DispatchBestRound, and Cast.ExtensionDictToExtension tests. Local validation passed: compute internals 73/73, scalar cast 108/108, scalar type/string 255/255, scalar math 526 passed with 26 existing half-float skips, and expression 47/47.

@LarryHu0217

Copy link
Copy Markdown
Contributor Author

@zanmato1984, when you have a chance, would you mind advising whether the eager ScalarFunction::Execute fallback in fdfa405 is the right layer for this behavior? The branch is mergeable and the targeted compute, cast, string, math, and expression tests listed above pass. I am happy to adjust the approach or run additional validation.

@zanmato1984

Copy link
Copy Markdown
Contributor

@pitrou @LarryHu0217 I think the generic direction is the right one, but I don't think the eager ScalarFunction::Execute fallback is the right layer for it.

My preference would be to keep DispatchBest / DispatchExact expressing only real kernels, and implement this as an internal executor-level adapter instead: first try the normal executor so existing dictionary kernels still take precedence; if that fails for a fixed-arity pure unary dictionary input whose value type has a matching kernel, construct a specialized dictionary-unary executor that runs the value kernel and remaps the result through the indices. That keeps the dictionary-specific execution logic out of the top-level eager Execute path while still avoiding full logical decoding.

The other point I would want to nail down before merging is semantic equivalence with the decoded-input path. If the fallback executes over the entire dictionary, it can evaluate unused dictionary values and change error behavior. For example, strptime with error_is_null=false should not fail because of an invalid dictionary value that is never referenced by the indices. So I think the adapter needs to process only referenced dictionary values, or otherwise preserve decoded-input behavior, and add a regression test for that case.

So my answer is: generic support makes sense, but I would move it out of ScalarFunction::Execute into a dedicated executor-level adapter and tighten the semantics/tests before merging.

@zanmato1984 zanmato1984 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the latest update, this feels much closer to the right shape to me: the generic dictionary fallback now lives in a dedicated function executor instead of the eager ScalarFunction::Execute path, while still leaving the existing dispatch contract unchanged.

Setting smaller follow-up details aside for the moment, does this look like the right overall direction now? If not, it would be helpful to understand what layer or shape you would prefer before iterating further on the implementation.

@github-actions github-actions Bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Aug 11, 2026
@zanmato1984

Copy link
Copy Markdown
Contributor

@pitrou Following up on my earlier review comment: with the latest update, this feels much closer to the right shape to me. The generic dictionary fallback now lives in a dedicated function executor instead of the eager ScalarFunction::Execute path, while still leaving the existing dispatch contract unchanged.

Setting smaller follow-up details aside for the moment, does this look like the right overall direction to you now? If not, it would be helpful to understand what layer or shape you would prefer before iterating further on the implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants