kou commented on code in PR #38873:
URL: https://github.com/apache/arrow/pull/38873#discussion_r1408620598
##########
cpp/src/gandiva/regex_functions_holder.cc:
##########
@@ -154,12 +152,11 @@ Status LikeHolder::Make(const std::string& sql_pattern,
const std::string& escap
Status::Invalid("Building RE2 pattern '", pcre_pattern,
"' failed with: ", lholder->regex_.error()));
- *holder = std::move(lholder);
- return Status::OK();
+ return std::move(lholder);
Review Comment:
```suggestion
return lholder;
```
##########
cpp/src/gandiva/regex_functions_holder.cc:
##########
@@ -190,18 +185,17 @@ Status ReplaceHolder::Make(const FunctionNode& node,
Status::Invalid(
"'replace' function requires a string literal as the second
parameter"));
- return Make(std::get<std::string>(literal->holder()), holder);
+ return Make(std::get<std::string>(literal->holder()));
}
-Status ReplaceHolder::Make(const std::string& sql_pattern,
- std::shared_ptr<ReplaceHolder>* holder) {
+Result<std::shared_ptr<ReplaceHolder>> ReplaceHolder::Make(
+ const std::string& sql_pattern) {
auto lholder = std::shared_ptr<ReplaceHolder>(new
ReplaceHolder(sql_pattern));
Review Comment:
```suggestion
auto lholder = std::make_shared<ReplaceHolder>(sql_pattern);
```
##########
cpp/src/gandiva/regex_functions_holder_test.cc:
##########
@@ -698,9 +629,9 @@ TEST_F(TestExtractHolder, TestErrorWhileBuildingHolder) {
function_node =
FunctionNode("regexp_extract", {field, pattern_as_node, index_node},
arrow::utf8());
- status = ExtractHolder::Make(function_node, &extract_holder);
- EXPECT_EQ(status.ok(), false);
- EXPECT_THAT(status.message(),
+ extract_holder = ExtractHolder::Make(function_node);
+ EXPECT_EQ(extract_holder.ok(), false);
+ EXPECT_THAT(extract_holder.status().message(),
::testing::HasSubstr(
"'extract' function requires a literal as the second
parameter"));
Review Comment:
ditto.
##########
cpp/src/gandiva/regex_functions_holder_test.cc:
##########
@@ -650,27 +586,22 @@ TEST_F(TestExtractHolder, TestInvalidRange) {
}
TEST_F(TestExtractHolder, TestExtractInvalidPattern) {
- std::shared_ptr<ExtractHolder> extract_holder;
-
- auto status = ExtractHolder::Make("+", &extract_holder);
- EXPECT_EQ(status.ok(), false) << status.message();
-
+ auto extract_holder = ExtractHolder::Make("+");
+ EXPECT_EQ(extract_holder.ok(), false) << extract_holder.status().message();
execution_context_.Reset();
}
TEST_F(TestExtractHolder, TestErrorWhileBuildingHolder) {
- std::shared_ptr<ExtractHolder> extract_holder;
-
// Create function with incorrect number of params
auto field = std::make_shared<FieldNode>(arrow::field("in", arrow::utf8()));
auto pattern_node = std::make_shared<LiteralNode>(
arrow::utf8(), LiteralHolder(R"((\w+) (\w+))"), false);
auto function_node =
FunctionNode("regexp_extract", {field, pattern_node}, arrow::utf8());
- auto status = ExtractHolder::Make(function_node, &extract_holder);
- EXPECT_EQ(status.ok(), false);
- EXPECT_THAT(status.message(),
+ auto extract_holder = ExtractHolder::Make(function_node);
+ EXPECT_EQ(extract_holder.ok(), false);
+ EXPECT_THAT(extract_holder.status().message(),
::testing::HasSubstr("'extract' function requires three
parameters"));
Review Comment:
Can we use `EXPECT_RAISES_WITH_MESSAGE_THAT()` here?
##########
cpp/src/gandiva/to_date_holder.cc:
##########
@@ -66,17 +65,15 @@ Status ToDateHolder::Make(const FunctionNode& node,
suppress_errors = std::get<int>(literal_suppress_errors->holder());
}
- return Make(pattern, suppress_errors, holder);
+ return Make(pattern, suppress_errors);
}
-Status ToDateHolder::Make(const std::string& sql_pattern, int32_t
suppress_errors,
- std::shared_ptr<ToDateHolder>* holder) {
+Result<std::shared_ptr<ToDateHolder>> ToDateHolder::Make(const std::string&
sql_pattern,
+ int32_t
suppress_errors) {
std::shared_ptr<std::string> transformed_pattern;
ARROW_RETURN_NOT_OK(DateUtils::ToInternalFormat(sql_pattern,
&transformed_pattern));
- auto lholder = std::shared_ptr<ToDateHolder>(
- new ToDateHolder(*(transformed_pattern.get()), suppress_errors));
- *holder = lholder;
- return Status::OK();
+ return std::shared_ptr<ToDateHolder>(
+ new ToDateHolder(*transformed_pattern, suppress_errors));
Review Comment:
```suggestion
return std::make_shared<ToDateHolder>(*transformed_pattern,
suppress_errors);
```
##########
cpp/src/gandiva/regex_functions_holder_test.cc:
##########
@@ -650,27 +586,22 @@ TEST_F(TestExtractHolder, TestInvalidRange) {
}
TEST_F(TestExtractHolder, TestExtractInvalidPattern) {
- std::shared_ptr<ExtractHolder> extract_holder;
-
- auto status = ExtractHolder::Make("+", &extract_holder);
- EXPECT_EQ(status.ok(), false) << status.message();
-
+ auto extract_holder = ExtractHolder::Make("+");
+ EXPECT_EQ(extract_holder.ok(), false) << extract_holder.status().message();
Review Comment:
Can we use `EXPECT_OK_AND_ASSIGN()` here too?
##########
cpp/src/gandiva/regex_functions_holder_test.cc:
##########
@@ -682,9 +613,9 @@ TEST_F(TestExtractHolder, TestErrorWhileBuildingHolder) {
function_node =
FunctionNode("regexp_extract", {field, pattern_node, index_node},
arrow::utf8());
- status = ExtractHolder::Make(function_node, &extract_holder);
- EXPECT_EQ(status.ok(), false);
- EXPECT_THAT(status.message(),
+ extract_holder = ExtractHolder::Make(function_node);
+ EXPECT_EQ(extract_holder.ok(), false);
+ EXPECT_THAT(extract_holder.status().message(),
::testing::HasSubstr(
"'extract' function requires a literal as the second
parameter"));
Review Comment:
ditto.
##########
cpp/src/gandiva/regex_functions_holder.cc:
##########
@@ -221,18 +214,17 @@ Status ExtractHolder::Make(const FunctionNode& node,
literal == nullptr ||
!IsArrowStringLiteral(literal->return_type()->id()),
Status::Invalid("'extract' function requires a literal as the second
parameter"));
- return ExtractHolder::Make(std::get<std::string>(literal->holder()), holder);
+ return ExtractHolder::Make(std::get<std::string>(literal->holder()));
}
-Status ExtractHolder::Make(const std::string& sql_pattern,
- std::shared_ptr<ExtractHolder>* holder) {
+Result<std::shared_ptr<ExtractHolder>> ExtractHolder::Make(
+ const std::string& sql_pattern) {
auto lholder = std::shared_ptr<ExtractHolder>(new
ExtractHolder(sql_pattern));
Review Comment:
```suggestion
auto lholder = std::make_shared<ExtractHolder>(sql_pattern);
```
##########
cpp/src/gandiva/to_date_holder_test.cc:
##########
@@ -143,15 +136,13 @@ TEST_F(TestToDateHolder, TestSimpleDateTimeError) {
}
TEST_F(TestToDateHolder, TestSimpleDateTimeMakeError) {
- std::shared_ptr<ToDateHolder> to_date_holder;
// reject time stamps for now.
- auto status = ToDateHolder::Make("YYYY-MM-DD HH:MI:SS tzo", 0,
&to_date_holder);
+ auto const status = ToDateHolder::Make("YYYY-MM-DD HH:MI:SS tzo",
0).status();
EXPECT_EQ(status.IsInvalid(), true) << status.message();
Review Comment:
Can we use `ASSERT_RAISES()` here?
--
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]
