You are viewing a plain text version of this content. The canonical link for it is here.

Posted to github@arrow.apache.org by "LarryHu0217 (via GitHub)" <gi...@apache.org> on 2026/07/13 20:36:20 UTC

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

LarryHu0217 opened a new pull request, #50501:
URL: https://github.com/apache/arrow/pull/50501

   ### Rationale for this change
   
   The UTF-8 trim functions only registered kernels for string and large-string inputs. Their exact dispatch therefore rejected dictionary-encoded strings even though the dictionary value type was supported.
   
   ### What changes are included in this PR?
   
   - Allow the stateful unary string-kernel helper to use a specialized function type.
   - Decode dictionary input types during dispatch for `utf8_trim`, `utf8_ltrim`, and `utf8_rtrim`.
   - Add regression coverage for dictionary-encoded string and large-string arrays, including nulls.
   
   ### Are these changes tested?
   
   - `arrow-compute-scalar-type-test --gtest_filter='*TrimUTF8*'` (4 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` 18.1.8 and `cpplint` 1.6.1
   
   ### Are there any user-facing changes?
   
   Yes. The three UTF-8 trim functions now accept dictionary-encoded string inputs and return decoded string arrays. There are no public API signature changes.
   
   ### 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 existing dictionary-dispatch patterns and validated with the commands above.
   


-- 
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-35815: [C++][Compute] Support dictionary inputs in UTF-8 trim [arrow]

Posted by "pitrou (via GitHub)" <gi...@apache.org>.

pitrou commented on PR #50501:
URL: https://github.com/apache/arrow/pull/50501#issuecomment-5093153024

   @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.


-- 
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-35815: [C++][Compute] Support dictionary inputs in UTF-8 trim [arrow]

Posted by "zanmato1984 (via GitHub)" <gi...@apache.org>.

zanmato1984 commented on PR #50501:
URL: https://github.com/apache/arrow/pull/50501#issuecomment-5247537812

@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.

--
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-35815: [C++][Compute] Support dictionary inputs in UTF-8 trim [arrow]

Posted by "LarryHu0217 (via GitHub)" <gi...@apache.org>.

LarryHu0217 commented on PR #50501:
URL: https://github.com/apache/arrow/pull/50501#issuecomment-5462968681

   @pitrou, following up on the executor-level direction in the Aug 11 maintainer comment: would you prefer the current `Function::GetBestExecutor` fallback with `DictionaryUnaryFunctionExecutor`, or a different layer before I iterate further? The only red check is ARM64 macOS GLib & Ruby, which fails while compiling the unrelated google-cloud-cpp dependency because a deprecation warning is promoted to an error; all other hosted checks pass. I’m happy to adjust the structure or run any focused validation you recommend.
   


-- 
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-35815: [C++][Compute] Support dictionary inputs in UTF-8 trim [arrow]

Posted by "pitrou (via GitHub)" <gi...@apache.org>.

pitrou commented on PR #50501:
URL: https://github.com/apache/arrow/pull/50501#issuecomment-5043536819

   > * 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.


-- 
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-35815: [C++][Compute] Support dictionary inputs in UTF-8 trim [arrow]

Posted by "zanmato1984 (via GitHub)" <gi...@apache.org>.

zanmato1984 commented on PR #50501:
URL: https://github.com/apache/arrow/pull/50501#issuecomment-5248568886

@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.

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.

--
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-35815: [C++][Compute] Support dictionary inputs in UTF-8 trim [arrow]

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.

github-actions[bot] commented on PR #50501:
URL: https://github.com/apache/arrow/pull/50501#issuecomment-4962449132

   :warning: GitHub issue #35815 **has been automatically assigned in GitHub** to PR creator.


-- 
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-35815: [C++][Compute] Support dictionary inputs in UTF-8 trim [arrow]

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.

github-actions[bot] commented on PR #50501:
URL: https://github.com/apache/arrow/pull/50501#issuecomment-5074429569

   :warning: GitHub issue #35815 **has been automatically assigned in GitHub** to PR creator.


-- 
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-35815: [C++][Compute] Support dictionary inputs in UTF-8 trim [arrow]

Posted by "LarryHu0217 (via GitHub)" <gi...@apache.org>.

LarryHu0217 commented on PR #50501:
URL: https://github.com/apache/arrow/pull/50501#issuecomment-5240919852

   @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.


-- 
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-35815: [C++][Compute] Support dictionary inputs in UTF-8 trim [arrow]

Posted by "zanmato1984 (via GitHub)" <gi...@apache.org>.

zanmato1984 commented on PR #50501:
URL: https://github.com/apache/arrow/pull/50501#issuecomment-5248435721

   @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.


-- 
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-35815: [C++][Compute] Support dictionary inputs in UTF-8 trim [arrow]

Posted by "LarryHu0217 (via GitHub)" <gi...@apache.org>.

LarryHu0217 commented on PR #50501:
URL: https://github.com/apache/arrow/pull/50501#issuecomment-5094638484

   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.


-- 
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-35815: [C++][Compute] Support dictionary inputs in UTF-8 trim [arrow]

Posted by "LarryHu0217 (via GitHub)" <gi...@apache.org>.

LarryHu0217 commented on PR #50501:
URL: https://github.com/apache/arrow/pull/50501#issuecomment-5074428304

   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.


-- 
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