You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "Reranko05 (via GitHub)" <gi...@apache.org> on 2026/04/28 06:12:32 UTC
[PR] GH-19667: [C++][Gandiva] Replace RegexUtil out references with pointer out args [arrow]
Reranko05 opened a new pull request, #49879:
URL: https://github.com/apache/arrow/pull/49879
### Rationale for this change
Replaced mutable reference out arguments with pointer out arguments to align with Gandiva style guidance. This PR addresses a scoped `RegexUtil` slice of that issue.
### What changes are included in this PR?
- Replaced `RegexUtil::SqlLikePatternToPcre` mutable reference out arguments with pointer out arguments.
- Updated related local call sites in `regex_functions_holder.cc`.
- Updated affected test in `regex_functions_holder_test.cc`.
### Are these changes tested?
- Rebuilt with `ninja gandiva`
- Ran regex-related tests successfully
### Are there any user-facing changes?
No
--
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-19667: [C++][Gandiva] Use arrow::Result for RegexUtil::SqlLikePatternToPcre [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #49879:
URL: https://github.com/apache/arrow/pull/49879#issuecomment-4450926923
After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit c13f3bc96c5d9985e49ef31cc0c232e1ffec379a.
There were no benchmark performance regressions. 🎉
The [full Conbench report](https://github.com/apache/arrow/runs/75993104729) has more details. It also includes information about 17 possible false positives for unstable benchmarks that are known to sometimes produce them.
--
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-19667: [C++][Gandiva] Replace RegexUtil out references with pointer out args [arrow]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #49879:
URL: https://github.com/apache/arrow/pull/49879#discussion_r3170765804
##########
cpp/src/gandiva/regex_util.cc:
##########
@@ -22,11 +22,11 @@ namespace gandiva {
const std::set<char> RegexUtil::pcre_regex_specials_ = {
'[', ']', '(', ')', '|', '^', '-', '+', '*', '?', '{', '}', '$', '\\', '.'};
-Status RegexUtil::SqlLikePatternToPcre(const std::string& sql_pattern, char escape_char,
- std::string& pcre_pattern) {
+arrow::Result<std::string> RegexUtil::SqlLikePatternToPcre(const std::string& sql_pattern,
+ char escape_char) {
/// Characters that are considered special by pcre regex. These needs to be
Review Comment:
Grammar in the comment is off: “These needs to be” should be “These need to be” (or rephrase to singular).
```suggestion
/// Characters that are considered special by pcre regex. These need to be
```
--
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-19667: [C++][Gandiva] Use arrow::Result for RegexUtil::SqlLikePatternToPcre [arrow]
Posted by "Reranko05 (via GitHub)" <gi...@apache.org>.
Reranko05 commented on PR #49879:
URL: https://github.com/apache/arrow/pull/49879#issuecomment-4357704615
@kou updated the PR title and description as suggested. Also addressed the minor comment in the code.
Thanks for the review!
--
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-19667: [C++][Gandiva] Replace RegexUtil out references with pointer out args [arrow]
Posted by "Reranko05 (via GitHub)" <gi...@apache.org>.
Reranko05 commented on code in PR #49879:
URL: https://github.com/apache/arrow/pull/49879#discussion_r3165586837
##########
cpp/src/gandiva/regex_util.h:
##########
@@ -31,10 +31,10 @@ class GANDIVA_EXPORT RegexUtil {
public:
// Convert an sql pattern to a pcre pattern
static Status SqlLikePatternToPcre(const std::string& like_pattern, char escape_char,
- std::string& pcre_pattern);
+ std::string* pcre_pattern);
Review Comment:
Thanks for the suggestion! Since this PR is scoped to address the mutable out argument pattern in RegexUtil as part of GH-19667, I kept the change minimal.
Would you prefer extending this PR to use `arrow::Result<std::string>`, or opening a follow-up PR for that?
--
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-19667: [C++][Gandiva] Replace RegexUtil out references with pointer out args [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #49879:
URL: https://github.com/apache/arrow/pull/49879#discussion_r3165056267
##########
cpp/src/gandiva/regex_util.h:
##########
@@ -31,10 +31,10 @@ class GANDIVA_EXPORT RegexUtil {
public:
// Convert an sql pattern to a pcre pattern
static Status SqlLikePatternToPcre(const std::string& like_pattern, char escape_char,
- std::string& pcre_pattern);
+ std::string* pcre_pattern);
Review Comment:
How about using `arrow::Result<std::string>` instead of `Status` and `std::string*` for this case?
--
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-19667: [C++][Gandiva] Replace RegexUtil out references with pointer out args [arrow]
Posted by "Reranko05 (via GitHub)" <gi...@apache.org>.
Reranko05 commented on PR #49879:
URL: https://github.com/apache/arrow/pull/49879#issuecomment-4335234830
@kou opened this PR for discussion.
--
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-19667: [C++][Gandiva] Replace RegexUtil out references with pointer out args [arrow]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #49879:
URL: https://github.com/apache/arrow/pull/49879#discussion_r3167666298
##########
cpp/src/gandiva/regex_functions_holder_test.cc:
##########
@@ -88,8 +88,9 @@ TEST_F(TestLikeHolder, TestPcreSpecialWithNewLine) {
}
TEST_F(TestLikeHolder, TestRegexEscape) {
- std::string res;
- ARROW_EXPECT_OK(RegexUtil::SqlLikePatternToPcre("#%hello#_abc_def##", '#', res));
+ auto maybe_res = RegexUtil::SqlLikePatternToPcre("#%hello#_abc_def##", '#');
+ ASSERT_TRUE(maybe_res.ok());
+ auto res = maybe_res.ValueOrDie();
Review Comment:
Prefer using the existing Arrow test helpers (e.g., `ASSERT_OK_AND_ASSIGN` / `EXPECT_OK_AND_ASSIGN`) instead of manually checking `.ok()` and then calling `ValueOrDie()`. This gives better failure messages and matches the style used elsewhere in this test file.
```suggestion
ASSERT_OK_AND_ASSIGN(auto res,
RegexUtil::SqlLikePatternToPcre("#%hello#_abc_def##", '#'));
```
##########
cpp/src/gandiva/regex_util.h:
##########
@@ -27,15 +27,15 @@
namespace gandiva {
/// \brief Utility class for converting sql patterns to pcre patterns.
-class GANDIVA_EXPORT RegexUtil {
+class RegexUtil {
public:
// Convert an sql pattern to a pcre pattern
- static Status SqlLikePatternToPcre(const std::string& like_pattern, char escape_char,
- std::string& pcre_pattern);
+ static GANDIVA_EXPORT arrow::Result<std::string> SqlLikePatternToPcre(
+ const std::string& like_pattern, char escape_char);
- static Status SqlLikePatternToPcre(const std::string& like_pattern,
- std::string& pcre_pattern) {
- return SqlLikePatternToPcre(like_pattern, 0 /*escape_char*/, pcre_pattern);
+ static GANDIVA_EXPORT arrow::Result<std::string> SqlLikePatternToPcre(
Review Comment:
The inline overload `SqlLikePatternToPcre(const std::string&)` is marked `GANDIVA_EXPORT` but also defined in the header. On Windows builds where `GANDIVA_EXPORT` expands to `__declspec(dllimport)`, MSVC does not allow providing a definition for a `dllimport` function, which can break consumers. Consider removing `GANDIVA_EXPORT` from the inline wrapper (it doesn’t need to be exported), or move the wrapper definition to the .cc / export the class instead.
```suggestion
static arrow::Result<std::string> SqlLikePatternToPcre(
```
##########
cpp/src/gandiva/regex_functions_holder.cc:
##########
@@ -147,14 +146,16 @@ Result<std::shared_ptr<LikeHolder>> LikeHolder::Make(const std::string& sql_patt
ARROW_RETURN_IF(escape_char.length() > 1,
Status::Invalid("The length of escape char ", escape_char,
" in 'like' function is greater than 1"));
- std::string pcre_pattern;
+ arrow::Result<std::string> result;
+
if (escape_char.length() == 1) {
- ARROW_RETURN_NOT_OK(
- RegexUtil::SqlLikePatternToPcre(sql_pattern, escape_char.at(0), pcre_pattern));
+ result = RegexUtil::SqlLikePatternToPcre(sql_pattern, escape_char.at(0));
} else {
- ARROW_RETURN_NOT_OK(RegexUtil::SqlLikePatternToPcre(sql_pattern, pcre_pattern));
+ result = RegexUtil::SqlLikePatternToPcre(sql_pattern);
}
+ ARROW_ASSIGN_OR_RAISE(auto pcre_pattern, std::move(result));
+
Review Comment:
This can be simplified by avoiding the intermediate default-constructed `arrow::Result<std::string> result` and the subsequent `std::move` into `ARROW_ASSIGN_OR_RAISE`. Doing the `ARROW_ASSIGN_OR_RAISE` directly in each branch will reduce control-flow and avoid relying on `Result` default construction.
##########
cpp/src/gandiva/regex_util.h:
##########
@@ -27,15 +27,15 @@
namespace gandiva {
/// \brief Utility class for converting sql patterns to pcre patterns.
-class GANDIVA_EXPORT RegexUtil {
+class RegexUtil {
public:
// Convert an sql pattern to a pcre pattern
- static Status SqlLikePatternToPcre(const std::string& like_pattern, char escape_char,
- std::string& pcre_pattern);
+ static GANDIVA_EXPORT arrow::Result<std::string> SqlLikePatternToPcre(
+ const std::string& like_pattern, char escape_char);
Review Comment:
This change removes the previous `Status SqlLikePatternToPcre(..., std::string& out)` API and replaces it with `arrow::Result<std::string>`. Since Gandiva installs its headers (`arrow_install_all_headers("gandiva")`), this is a public API break and contradicts the PR description’s “No user-facing changes”. If the intent is only to replace mutable reference out-args with pointer out-args, consider keeping a backwards-compatible overload (e.g., `Status ... (std::string* out)` delegating to the new API) or update the PR description to reflect the breaking change.
--
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-19667: [C++][Gandiva] Use arrow::Result for RegexUtil::SqlLikePatternToPcre [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #49879:
URL: https://github.com/apache/arrow/pull/49879#issuecomment-4359280915
After merging your PR, Conbench analyzed the 0 benchmarking runs that have been run so far on merge-commit c13f3bc96c5d9985e49ef31cc0c232e1ffec379a.
None of the specified runs were found on the Conbench server.
The [full Conbench report](https://github.com/apache/arrow/runs/73931547786) has more details.
--
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-19667: [C++][Gandiva] Use arrow::Result for RegexUtil::SqlLikePatternToPcre [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #49879:
URL: https://github.com/apache/arrow/pull/49879#issuecomment-4359159871
Thanks.
--
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-19667: [C++][Gandiva] Replace RegexUtil out references with pointer out args [arrow]
Posted by "lriggs (via GitHub)" <gi...@apache.org>.
lriggs commented on code in PR #49879:
URL: https://github.com/apache/arrow/pull/49879#discussion_r3165003270
##########
cpp/src/gandiva/regex_util.cc:
##########
@@ -23,16 +23,16 @@ const std::set<char> RegexUtil::pcre_regex_specials_ = {
'[', ']', '(', ')', '|', '^', '-', '+', '*', '?', '{', '}', '$', '\\', '.'};
Status RegexUtil::SqlLikePatternToPcre(const std::string& sql_pattern, char escape_char,
- std::string& pcre_pattern) {
+ std::string* pcre_pattern) {
/// Characters that are considered special by pcre regex. These needs to be
Review Comment:
I think you should check for null now that its possible to pass a null pointer.
--
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-19667: [C++][Gandiva] Replace RegexUtil out references with pointer out args [arrow]
Posted by "lriggs (via GitHub)" <gi...@apache.org>.
lriggs commented on code in PR #49879:
URL: https://github.com/apache/arrow/pull/49879#discussion_r3165001632
##########
cpp/src/gandiva/regex_util.cc:
##########
@@ -23,16 +23,16 @@ const std::set<char> RegexUtil::pcre_regex_specials_ = {
'[', ']', '(', ')', '|', '^', '-', '+', '*', '?', '{', '}', '$', '\\', '.'};
Status RegexUtil::SqlLikePatternToPcre(const std::string& sql_pattern, char escape_char,
- std::string& pcre_pattern) {
+ std::string* pcre_pattern) {
Review Comment:
std::string* const pcre_pattern
--
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-19667: [C++][Gandiva] Use arrow::Result for RegexUtil::SqlLikePatternToPcre [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou merged PR #49879:
URL: https://github.com/apache/arrow/pull/49879
--
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-19667: [C++][Gandiva] Replace RegexUtil out references with pointer out args [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #49879:
URL: https://github.com/apache/arrow/pull/49879#issuecomment-4332786412
:warning: GitHub issue #19667 **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-19667: [C++][Gandiva] Replace RegexUtil out references with pointer out args [arrow]
Posted by "Reranko05 (via GitHub)" <gi...@apache.org>.
Reranko05 commented on code in PR #49879:
URL: https://github.com/apache/arrow/pull/49879#discussion_r3165570020
##########
cpp/src/gandiva/regex_util.cc:
##########
@@ -23,16 +23,16 @@ const std::set<char> RegexUtil::pcre_regex_specials_ = {
'[', ']', '(', ')', '|', '^', '-', '+', '*', '?', '{', '}', '$', '\\', '.'};
Status RegexUtil::SqlLikePatternToPcre(const std::string& sql_pattern, char escape_char,
- std::string& pcre_pattern) {
+ std::string* pcre_pattern) {
/// Characters that are considered special by pcre regex. These needs to be
Review Comment:
Updated to use `std::string* const` and added a null check for `pcre_pattern`.
--
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-19667: [C++][Gandiva] Replace RegexUtil out references with pointer out args [arrow]
Posted by "Reranko05 (via GitHub)" <gi...@apache.org>.
Reranko05 commented on code in PR #49879:
URL: https://github.com/apache/arrow/pull/49879#discussion_r3171986310
##########
cpp/src/gandiva/regex_util.cc:
##########
@@ -22,11 +22,11 @@ namespace gandiva {
const std::set<char> RegexUtil::pcre_regex_specials_ = {
'[', ']', '(', ')', '|', '^', '-', '+', '*', '?', '{', '}', '$', '\\', '.'};
-Status RegexUtil::SqlLikePatternToPcre(const std::string& sql_pattern, char escape_char,
- std::string& pcre_pattern) {
+arrow::Result<std::string> RegexUtil::SqlLikePatternToPcre(const std::string& sql_pattern,
+ char escape_char) {
/// Characters that are considered special by pcre regex. These needs to be
Review Comment:
fixed
--
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-19667: [C++][Gandiva] Replace RegexUtil out references with pointer out args [arrow]
Posted by "Reranko05 (via GitHub)" <gi...@apache.org>.
Reranko05 commented on code in PR #49879:
URL: https://github.com/apache/arrow/pull/49879#discussion_r3167747270
##########
cpp/src/gandiva/regex_util.h:
##########
@@ -27,15 +27,15 @@
namespace gandiva {
/// \brief Utility class for converting sql patterns to pcre patterns.
-class GANDIVA_EXPORT RegexUtil {
+class RegexUtil {
public:
// Convert an sql pattern to a pcre pattern
- static Status SqlLikePatternToPcre(const std::string& like_pattern, char escape_char,
- std::string& pcre_pattern);
+ static GANDIVA_EXPORT arrow::Result<std::string> SqlLikePatternToPcre(
+ const std::string& like_pattern, char escape_char);
Review Comment:
Thanks for the suggestions!
* Updated the test to use ASSERT_OK_AND_ASSIGN
* Removed GANDIVA_EXPORT from the inline wrapper to avoid Windows issues
* Clarified in the PR description that this introduces a breaking API change
* Simplified the control flow in `regex_functions_holder.cc` by using ARROW_ASSIGN_OR_RAISE directly in each branch
Keeping the Result-based API as suggested by @kou.
--
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-19667: [C++][Gandiva] Replace RegexUtil out references with pointer out args [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #49879:
URL: https://github.com/apache/arrow/pull/49879#discussion_r3167105660
##########
cpp/src/gandiva/regex_util.h:
##########
@@ -31,10 +31,10 @@ class GANDIVA_EXPORT RegexUtil {
public:
// Convert an sql pattern to a pcre pattern
static Status SqlLikePatternToPcre(const std::string& like_pattern, char escape_char,
- std::string& pcre_pattern);
+ std::string* pcre_pattern);
Review Comment:
The former. If we use `arrow::Result<std::string>`, we don't need most changes in this PR. So I feel that the follow-up PR style isn't suitable for this case.
--
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-19667: [C++][Gandiva] Replace RegexUtil out references with pointer out args [arrow]
Posted by "Reranko05 (via GitHub)" <gi...@apache.org>.
Reranko05 commented on code in PR #49879:
URL: https://github.com/apache/arrow/pull/49879#discussion_r3167634521
##########
cpp/src/gandiva/regex_util.h:
##########
@@ -31,10 +31,10 @@ class GANDIVA_EXPORT RegexUtil {
public:
// Convert an sql pattern to a pcre pattern
static Status SqlLikePatternToPcre(const std::string& like_pattern, char escape_char,
- std::string& pcre_pattern);
+ std::string* pcre_pattern);
Review Comment:
Updated the PR to use `arrow::Result<std::string>` instead of pointer out arguments and adjusted the call sites and tests accordingly.
--
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