You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "niyue (via GitHub)" <gi...@apache.org> on 2023/11/24 12:57:45 UTC
[PR] MINOR: [C++][Gandiva] Refactor gandiva function holder to return arrow Result [arrow]
niyue opened a new pull request, #38873:
URL: https://github.com/apache/arrow/pull/38873
### Rationale for this change
This PR tries to make Gandiva `FunctionHolder` classes to return `arrow::Result` instead of using output parameters, and this tries to address the follow up task mentioned in https://github.com/apache/arrow/pull/38632#discussion_r1388576545 and makes the code slightly simpler
### What changes are included in this PR?
A refactoring task to return `arrow::Result` in Gandiva FunctionHolder classes
### Are these changes tested?
Yes, covered by existing unit tests
### 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-38920: [C++][Gandiva] Refactor function holder to return arrow Result [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou merged PR #38873:
URL: https://github.com/apache/arrow/pull/38873
--
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-38879: [C++][Gandiva] Fix to_date_utf8_utf8_int32 function param validation and refactor gandiva function holder to return arrow Result [arrow]
Posted by "niyue (via GitHub)" <gi...@apache.org>.
niyue commented on PR #38873:
URL: https://github.com/apache/arrow/pull/38873#issuecomment-1826231764
> Could you open an issue for this?
Sure. I opened a bug https://github.com/apache/arrow/issues/38879, which described the to_date_utf8_utf8_int32 invalid input bug I fixed in this PR. And the refactoring task is described in this PR description. Let me know if I need to create a separated issue 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-38920: [C++][Gandiva] Refactor gandiva function holder to return arrow Result [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #38873:
URL: https://github.com/apache/arrow/pull/38873#issuecomment-1829775977
:warning: GitHub issue #38920 **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] MINOR: [C++][Gandiva] Refactor gandiva function holder to return arrow Result [arrow]
Posted by "niyue (via GitHub)" <gi...@apache.org>.
niyue commented on code in PR #38873:
URL: https://github.com/apache/arrow/pull/38873#discussion_r1404335496
##########
cpp/src/gandiva/to_date_holder.cc:
##########
@@ -51,7 +50,7 @@ Status ToDateHolder::Make(const FunctionNode& node,
if (node.children().size() == 3) {
auto literal_suppress_errors =
dynamic_cast<LiteralNode*>(node.children().at(2).get());
- if (literal_pattern == nullptr) {
+ if (literal_suppress_errors == nullptr) {
Review Comment:
This is a bug I found accidentally during the refactoring. The `literal_pattern` is always **not** `nullptr` since line 37 already check if it is `nullptr`. And the code here would like to check the third parameter (according to the error reported below), which should be `literal_suppress_errors` I believe
--
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-38920: [C++][Gandiva] Refactor function holder to return arrow Result [arrow]
Posted by "niyue (via GitHub)" <gi...@apache.org>.
niyue commented on code in PR #38873:
URL: https://github.com/apache/arrow/pull/38873#discussion_r1408790273
##########
cpp/src/gandiva/random_generator_holder.cc:
##########
@@ -38,8 +37,7 @@ Status RandomGeneratorHolder::Make(const FunctionNode& node,
literal_type != arrow::Type::INT32,
Status::Invalid("'random' function requires an int32 literal as parameter"));
- *holder = std::shared_ptr<RandomGeneratorHolder>(new RandomGeneratorHolder(
+ return std::shared_ptr<RandomGeneratorHolder>(new RandomGeneratorHolder(
Review Comment:
Explained 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-38879: [C++][Gandiva] Fix to_date_utf8_utf8_int32 function param validation and refactor gandiva function holder to return arrow Result [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #38873:
URL: https://github.com/apache/arrow/pull/38873#issuecomment-1826231338
:warning: GitHub issue #38879 **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-38879: [C++][Gandiva] Fix to_date_utf8_utf8_int32 function param validation and refactor gandiva function holder to return arrow Result [arrow]
Posted by "niyue (via GitHub)" <gi...@apache.org>.
niyue commented on code in PR #38873:
URL: https://github.com/apache/arrow/pull/38873#discussion_r1407674648
##########
cpp/src/gandiva/regex_functions_holder_test.cc:
##########
@@ -330,16 +289,14 @@ class TestReplaceHolder : public ::testing::Test {
};
TEST_F(TestReplaceHolder, TestMultipleReplace) {
- std::shared_ptr<ReplaceHolder> replace_holder;
-
- auto status = ReplaceHolder::Make("ana", &replace_holder);
- EXPECT_EQ(status.ok(), true) << status.message();
+ auto replace_holder = ReplaceHolder::Make("ana");
+ EXPECT_EQ(replace_holder.ok(), true) << replace_holder.status().message();
Review Comment:
Sure. I changed some of such usages in these several test files now I change more such usages I find.
--
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-38920: [C++][Gandiva] Refactor gandiva function holder to return arrow Result [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #38873:
URL: https://github.com/apache/arrow/pull/38873#discussion_r1408145751
##########
cpp/src/gandiva/random_generator_holder.cc:
##########
@@ -38,8 +37,7 @@ Status RandomGeneratorHolder::Make(const FunctionNode& node,
literal_type != arrow::Type::INT32,
Status::Invalid("'random' function requires an int32 literal as parameter"));
- *holder = std::shared_ptr<RandomGeneratorHolder>(new RandomGeneratorHolder(
+ return std::shared_ptr<RandomGeneratorHolder>(new RandomGeneratorHolder(
Review Comment:
Same here.
##########
cpp/src/gandiva/random_generator_holder.cc:
##########
@@ -19,14 +19,13 @@
#include "gandiva/node.h"
namespace gandiva {
-Status RandomGeneratorHolder::Make(const FunctionNode& node,
- std::shared_ptr<RandomGeneratorHolder>* holder) {
+Result<std::shared_ptr<RandomGeneratorHolder>> RandomGeneratorHolder::Make(
+ const FunctionNode& node) {
ARROW_RETURN_IF(node.children().size() > 1,
Status::Invalid("'random' function requires at most one parameter"));
if (node.children().size() == 0) {
- *holder = std::shared_ptr<RandomGeneratorHolder>(new RandomGeneratorHolder());
- return Status::OK();
+ return std::shared_ptr<RandomGeneratorHolder>(new RandomGeneratorHolder());
Review Comment:
Can probably use `std::make_shared`?
--
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-38920: [C++][Gandiva] Refactor function holder to return arrow Result [arrow]
Posted by "niyue (via GitHub)" <gi...@apache.org>.
niyue commented on code in PR #38873:
URL: https://github.com/apache/arrow/pull/38873#discussion_r1408791594
##########
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:
Explained above.
##########
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:
Updated.
--
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-38920: [C++][Gandiva] Refactor function holder to return arrow Result [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #38873:
URL: https://github.com/apache/arrow/pull/38873#issuecomment-1833342943
After merging your PR, Conbench analyzed the 5 benchmarking runs that have been run so far on merge-commit 6f497ecc942cb1b2b7b0cdded650006be8cf9896.
There were 4 benchmark results indicating a performance regression:
- Commit Run on `ursa-i9-9960x` at [2023-11-30 06:38:39Z](https://conbench.ursa.dev/compare/runs/e7114a8e657c489fa3b49b1228d92732...b156d4739f9d45de98937f731bd4b574/)
- [`file-read` (R) with compression=uncompressed, dataset=fanniemae_2016Q4, file_type=feather, language=R, output_type=dataframe](https://conbench.ursa.dev/compare/benchmarks/0656813668b37bfe8000383d4f7a11f0...065683c2431375ac8000f4d8e4070303)
- [`file-read` (R) with compression=lz4, dataset=fanniemae_2016Q4, file_type=feather, language=R, output_type=table](https://conbench.ursa.dev/compare/benchmarks/06568137285770f28000093693d954f8...065683c3693b7f808000b30f79224ad8)
- and 2 more (see the report linked below)
The [full Conbench report](https://github.com/apache/arrow/runs/19171189135) has more details. It also includes information about 5 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-38879: [C++][Gandiva] Fix to_date_utf8_utf8_int32 function param validation and refactor gandiva function holder to return arrow Result [arrow]
Posted by "niyue (via GitHub)" <gi...@apache.org>.
niyue commented on code in PR #38873:
URL: https://github.com/apache/arrow/pull/38873#discussion_r1407673258
##########
cpp/src/gandiva/regex_functions_holder_test.cc:
##########
@@ -252,29 +224,25 @@ TEST_F(TestLikeHolder, TestEmptyEscapeChar) {
}
TEST_F(TestLikeHolder, TestMultipleEscapeChar) {
- std::shared_ptr<LikeHolder> like_holder;
-
- auto status = LikeHolder::Make("ab\\_", "\\\\", &like_holder);
+ auto status = LikeHolder::Make("ab\\_", "\\\\").status();
EXPECT_EQ(status.ok(), false) << status.message();
}
class TestILikeHolder : public ::testing::Test {
public:
RE2::Options regex_op;
- FunctionNode BuildILike(std::string pattern) {
+ static FunctionNode BuildILike(std::string pattern) {
Review Comment:
I follow clang-tidy's suggestion to make this change because this function uses none of the member variable of class `testILikeHolder`, which means it can simply be a static function but not a member function, and the `TEST_F()` can still use them. But it is probably easier to follow if we move them out of the class entirely. Anyway, I change this part back since it is not critical in this PR.
--
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-38920: [C++][Gandiva] Refactor function holder to return arrow Result [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #38873:
URL: https://github.com/apache/arrow/pull/38873#issuecomment-1831551945
Ci failures look unrelated, I'll merge.
Thanks a lot for doing this @niyue !
--
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-38920: [C++][Gandiva] Refactor gandiva function holder to return arrow Result [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: [C++][Gandiva] Refactor gandiva function holder to return arrow Result [arrow]
Posted by "niyue (via GitHub)" <gi...@apache.org>.
niyue commented on code in PR #38873:
URL: https://github.com/apache/arrow/pull/38873#discussion_r1404335496
##########
cpp/src/gandiva/to_date_holder.cc:
##########
@@ -51,7 +50,7 @@ Status ToDateHolder::Make(const FunctionNode& node,
if (node.children().size() == 3) {
auto literal_suppress_errors =
dynamic_cast<LiteralNode*>(node.children().at(2).get());
- if (literal_pattern == nullptr) {
+ if (literal_suppress_errors == nullptr) {
Review Comment:
This is a bug for `to_date_holder` I found accidentally during the refactoring. The `literal_pattern` is always **not** `nullptr` since line 37 already check if it is `nullptr`. And the code here would like to check the third parameter (according to the error reported below), which should be `literal_suppress_errors` I believe
--
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-38879: [C++][Gandiva] Fix to_date_utf8_utf8_int32 function param validation and refactor gandiva function holder to return arrow Result [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #38873:
URL: https://github.com/apache/arrow/pull/38873#discussion_r1405548776
##########
cpp/src/gandiva/regex_functions_holder_test.cc:
##########
@@ -252,29 +224,25 @@ TEST_F(TestLikeHolder, TestEmptyEscapeChar) {
}
TEST_F(TestLikeHolder, TestMultipleEscapeChar) {
- std::shared_ptr<LikeHolder> like_holder;
-
- auto status = LikeHolder::Make("ab\\_", "\\\\", &like_holder);
+ auto status = LikeHolder::Make("ab\\_", "\\\\").status();
EXPECT_EQ(status.ok(), false) << status.message();
}
class TestILikeHolder : public ::testing::Test {
public:
RE2::Options regex_op;
- FunctionNode BuildILike(std::string pattern) {
+ static FunctionNode BuildILike(std::string pattern) {
Review Comment:
Why do we need this `static`?
I think that we can call this as an instance method in `TEST_F()` without `static`.
##########
cpp/src/gandiva/regex_functions_holder_test.cc:
##########
@@ -330,16 +289,14 @@ class TestReplaceHolder : public ::testing::Test {
};
TEST_F(TestReplaceHolder, TestMultipleReplace) {
- std::shared_ptr<ReplaceHolder> replace_holder;
-
- auto status = ReplaceHolder::Make("ana", &replace_holder);
- EXPECT_EQ(status.ok(), true) << status.message();
+ auto replace_holder = ReplaceHolder::Make("ana");
+ EXPECT_EQ(replace_holder.ok(), true) << replace_holder.status().message();
Review Comment:
Can we use `EXPECT_OK_AND_ASSIGN()` here?
##########
cpp/src/gandiva/regex_functions_holder.cc:
##########
@@ -132,12 +131,11 @@ Status LikeHolder::Make(const std::string& sql_pattern,
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:
I think that `std::move()` for local variable in `return` isn't needed.
I think that it'll prevent NRVO (Named Return Value Optimization). See also: https://en.cppreference.com/w/cpp/language/copy_elision#Non-mandatory_copy.2Fmove.28since_C.2B.2B11.29_elision
--
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-38920: [C++][Gandiva] Refactor gandiva function holder to return arrow Result [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #38873:
URL: https://github.com/apache/arrow/pull/38873#issuecomment-1830370261
cc @js8544
--
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-38920: [C++][Gandiva] Refactor function holder to return arrow Result [arrow]
Posted by "niyue (via GitHub)" <gi...@apache.org>.
niyue commented on code in PR #38873:
URL: https://github.com/apache/arrow/pull/38873#discussion_r1408790446
##########
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:
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] MINOR: [C++][Gandiva] Refactor gandiva function holder to return arrow Result [arrow]
Posted by "niyue (via GitHub)" <gi...@apache.org>.
niyue commented on code in PR #38873:
URL: https://github.com/apache/arrow/pull/38873#discussion_r1404335496
##########
cpp/src/gandiva/to_date_holder.cc:
##########
@@ -51,7 +50,7 @@ Status ToDateHolder::Make(const FunctionNode& node,
if (node.children().size() == 3) {
auto literal_suppress_errors =
dynamic_cast<LiteralNode*>(node.children().at(2).get());
- if (literal_pattern == nullptr) {
+ if (literal_suppress_errors == nullptr) {
Review Comment:
This is a bug I found accidentally during the refactoring. The `literal_pattern` is always **not** `nullptr` since line 37 already check if it is `nullptr`. And the code here would like to check the third parameter, which should be `literal_suppress_errors` I believe
--
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] MINOR: [C++][Gandiva] Refactor gandiva function holder to return arrow Result [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #38873:
URL: https://github.com/apache/arrow/pull/38873#issuecomment-1826100214
Thanks. Could you open an issue for this?
This is not a MINOR task now. :-)
Our MINOR definition: https://github.com/apache/arrow/blob/main/CONTRIBUTING.md#minor-fixes
--
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-38879: [C++][Gandiva] Fix to_date_utf8_utf8_int32 function param validation and refactor gandiva function holder to return arrow Result [arrow]
Posted by "niyue (via GitHub)" <gi...@apache.org>.
niyue commented on code in PR #38873:
URL: https://github.com/apache/arrow/pull/38873#discussion_r1407651953
##########
cpp/src/gandiva/regex_functions_holder.cc:
##########
@@ -132,12 +131,11 @@ Status LikeHolder::Make(const std::string& sql_pattern,
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:
Thanks for pointing it out. I change several usages I can find in the codebase (all in the same file)
--
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] MINOR: [C++][Gandiva] Refactor gandiva function holder to return arrow Result [arrow]
Posted by "niyue (via GitHub)" <gi...@apache.org>.
niyue commented on code in PR #38873:
URL: https://github.com/apache/arrow/pull/38873#discussion_r1404335496
##########
cpp/src/gandiva/to_date_holder.cc:
##########
@@ -51,7 +50,7 @@ Status ToDateHolder::Make(const FunctionNode& node,
if (node.children().size() == 3) {
auto literal_suppress_errors =
dynamic_cast<LiteralNode*>(node.children().at(2).get());
- if (literal_pattern == nullptr) {
+ if (literal_suppress_errors == nullptr) {
Review Comment:
This is a bug I found accidentally during the refactoring. The `literal_pattern` is always non `nullptr` since line 37 already check if it is `nullptr`. And the code here would like to check the third parameter, which should be `literal_suppress_errors` I believe
--
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-38920: [C++][Gandiva] Refactor function holder to return arrow Result [arrow]
Posted by "niyue (via GitHub)" <gi...@apache.org>.
niyue commented on code in PR #38873:
URL: https://github.com/apache/arrow/pull/38873#discussion_r1408790941
##########
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:
Updated.
##########
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:
Sure, updated
--
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-38920: [C++][Gandiva] Refactor function holder to return arrow Result [arrow]
Posted by "niyue (via GitHub)" <gi...@apache.org>.
niyue commented on code in PR #38873:
URL: https://github.com/apache/arrow/pull/38873#discussion_r1408790026
##########
cpp/src/gandiva/random_generator_holder.cc:
##########
@@ -19,14 +19,13 @@
#include "gandiva/node.h"
namespace gandiva {
-Status RandomGeneratorHolder::Make(const FunctionNode& node,
- std::shared_ptr<RandomGeneratorHolder>* holder) {
+Result<std::shared_ptr<RandomGeneratorHolder>> RandomGeneratorHolder::Make(
+ const FunctionNode& node) {
ARROW_RETURN_IF(node.children().size() > 1,
Status::Invalid("'random' function requires at most one parameter"));
if (node.children().size() == 0) {
- *holder = std::shared_ptr<RandomGeneratorHolder>(new RandomGeneratorHolder());
- return Status::OK();
+ return std::shared_ptr<RandomGeneratorHolder>(new RandomGeneratorHolder());
Review Comment:
This is not possible since this is called inside a factory function `Make` and the constructor is intentionally marked as `private` so that users can only use the factory function for creating instances, and `make_shared` can only call public constructor.
--
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-38920: [C++][Gandiva] Refactor function holder to return arrow Result [arrow]
Posted by "niyue (via GitHub)" <gi...@apache.org>.
niyue commented on code in PR #38873:
URL: https://github.com/apache/arrow/pull/38873#discussion_r1408791169
##########
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:
Updated
##########
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:
Updated
--
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-38920: [C++][Gandiva] Refactor function holder to return arrow Result [arrow]
Posted by "niyue (via GitHub)" <gi...@apache.org>.
niyue commented on code in PR #38873:
URL: https://github.com/apache/arrow/pull/38873#discussion_r1408790772
##########
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:
This is not possible since this is called inside a factory function `Make` and the constructor is intentionally marked as `private` so that users can only use the factory function for creating instances, and `make_shared` can only call public constructor.
--
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-38920: [C++][Gandiva] Refactor function holder to return arrow Result [arrow]
Posted by "niyue (via GitHub)" <gi...@apache.org>.
niyue commented on code in PR #38873:
URL: https://github.com/apache/arrow/pull/38873#discussion_r1408793757
##########
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:
Explained 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] MINOR: [C++][Gandiva] Refactor gandiva function holder to return arrow Result [arrow]
Posted by "niyue (via GitHub)" <gi...@apache.org>.
niyue commented on PR #38873:
URL: https://github.com/apache/arrow/pull/38873#issuecomment-1825648726
@kou I submit this PR to address the follow up task mentioned in https://github.com/apache/arrow/pull/38632#discussion_r1388576545.
But after I finish most of the refactoring, I realize the template function may still be needed since the function holder map requires returning a parent class shared_ptr (FunctionHolderPtr) but each FunctionHolder sub class's `Make` returns a concrete class shared_ptr.
But this PR at least reduces 80 lines of code, so I think it may still be useful and submit it 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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38879: [C++][Gandiva] Fix to_date_utf8_utf8_int32 function param validation and refactor gandiva function holder to return arrow Result [arrow]
Posted by "niyue (via GitHub)" <gi...@apache.org>.
niyue commented on PR #38873:
URL: https://github.com/apache/arrow/pull/38873#issuecomment-1829734442
> Could you open one more issue for refactoring?
Okay. Do you think if I need to submit a separated PR, this one for bug fixing and the other one for refactoring?
--
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