Uh oh!
There was an error while loading. Please reload this page.
GH-19667: [C++][Gandiva] Use arrow::Result<std::string> for RegexUtil::SqlLikePatternToPcre - #49879
Conversation
Reranko05
commented
Apr 28, 2026
@kou opened this PR for discussion. |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Pull request overview
This PR updates Gandiva’s RegexUtil::SqlLikePatternToPcre API to avoid mutable reference out-parameters and propagates the change through local LIKE holder call sites and tests.
Changes:
- Changed
RegexUtil::SqlLikePatternToPcreto returnarrow::Result<std::string>instead of writing into a mutable out-parameter. - Updated
LikeHoldercreation paths to useARROW_ASSIGN_OR_RAISEwith the new API. - Updated the affected unit test to consume the new
Result<std::string>return type.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| cpp/src/gandiva/regex_util.h | Updates the public API signature and export annotations for SqlLikePatternToPcre. |
| cpp/src/gandiva/regex_util.cc | Implements the new Result<std::string>-returning conversion function. |
| cpp/src/gandiva/regex_functions_holder.cc | Adapts LIKE holder construction to the new RegexUtil API. |
| cpp/src/gandiva/regex_functions_holder_test.cc | Updates a test to use the new Result<std::string> return value. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
kou
left a comment
There was a problem hiding this comment.
+1
Could you update the PR title and description?
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Uh oh!
There was an error while loading. Please reload this page.
Reranko05
commented
May 1, 2026
@kou updated the PR title and description as suggested. Also addressed the minor comment in the code. Thanks for the review! |
kou
commented
May 1, 2026
Thanks. |
After merging your PR, Conbench analyzed the 0 benchmarking runs that have been run so far on merge-commit c13f3bc. None of the specified runs were found on the Conbench server. The full Conbench report has more details. |
After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit c13f3bc. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 17 possible false positives for unstable benchmarks that are known to sometimes produce them. |
…exUtil::SqlLikePatternToPcre (apache#49879) ### Rationale for this change Replace the `Status` + out-parameter pattern in `RegexUtil::SqlLikePatternToPcre` with `arrow::Result<std::string>`, aligning with modern Arrow API practices. ### What changes are included in this PR? * Updated `RegexUtil::SqlLikePatternToPcre` to return `arrow::Result<std::string>` instead of using an out-parameter * Updated call sites in `regex_functions_holder.cc` to use `ARROW_ASSIGN_OR_RAISE` * Updated tests in `regex_functions_holder_test.cc` to use `ASSERT_OK_AND_ASSIGN` * Removed `GANDIVA_EXPORT` from the inline wrapper to avoid Windows `dllimport` issues * Simplified control flow in `regex_functions_holder.cc` ### Are these changes tested? * Rebuilt with `ninja gandiva` * Ran Gandiva tests successfully ### Are there any user-facing changes? Yes. * GitHub Issue: apache#19667 Authored-by: Aaditya Srinivasan <aadityasri03@gmail.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>
Rationale for this change
Replace the
Status+ out-parameter pattern inRegexUtil::SqlLikePatternToPcrewitharrow::Result<std::string>, aligning with modern Arrow API practices.What changes are included in this PR?
RegexUtil::SqlLikePatternToPcreto returnarrow::Result<std::string>instead of using an out-parameterregex_functions_holder.ccto useARROW_ASSIGN_OR_RAISEregex_functions_holder_test.ccto useASSERT_OK_AND_ASSIGNGANDIVA_EXPORTfrom the inline wrapper to avoid Windowsdllimportissuesregex_functions_holder.ccAre these changes tested?
ninja gandivaAre there any user-facing changes?
Yes.