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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
