This is an automated email from the ASF dual-hosted git repository.
apitrou pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/arrow.git
The following commit(s) were added to refs/heads/main by this push:
new 6f497ecc94 GH-38920: [C++][Gandiva] Refactor function holder to return
arrow Result (#38873)
6f497ecc94 is described below
commit 6f497ecc942cb1b2b7b0cdded650006be8cf9896
Author: Yue <[email protected]>
AuthorDate: Wed Nov 29 17:44:28 2023 +0800
GH-38920: [C++][Gandiva] Refactor function holder to return arrow Result
(#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?
It should be covered by existing unit tests.
### Are there any user-facing changes?
No
* Closes: #38920
Authored-by: Yue Ni <[email protected]>
Signed-off-by: Antoine Pitrou <[email protected]>
---
cpp/src/gandiva/function_holder_maker_registry.cc | 4 +-
cpp/src/gandiva/interval_holder.cc | 24 +--
cpp/src/gandiva/interval_holder.h | 25 ++--
cpp/src/gandiva/interval_holder_test.cc | 44 ++----
cpp/src/gandiva/random_generator_holder.cc | 10 +-
cpp/src/gandiva/random_generator_holder.h | 3 +-
cpp/src/gandiva/random_generator_holder_test.cc | 39 ++---
cpp/src/gandiva/regex_functions_holder.cc | 61 ++++----
cpp/src/gandiva/regex_functions_holder.h | 22 ++-
cpp/src/gandiva/regex_functions_holder_test.cc | 170 +++++++---------------
cpp/src/gandiva/to_date_holder.cc | 15 +-
cpp/src/gandiva/to_date_holder.h | 6 +-
cpp/src/gandiva/to_date_holder_test.cc | 25 +---
13 files changed, 151 insertions(+), 297 deletions(-)
diff --git a/cpp/src/gandiva/function_holder_maker_registry.cc
b/cpp/src/gandiva/function_holder_maker_registry.cc
index bb93402475..37ca1fbf6c 100644
--- a/cpp/src/gandiva/function_holder_maker_registry.cc
+++ b/cpp/src/gandiva/function_holder_maker_registry.cc
@@ -41,9 +41,7 @@ arrow::Status FunctionHolderMakerRegistry::Register(const
std::string& name,
template <typename HolderType>
static arrow::Result<FunctionHolderPtr> HolderMaker(const FunctionNode& node) {
- std::shared_ptr<HolderType> derived_instance;
- ARROW_RETURN_NOT_OK(HolderType::Make(node, &derived_instance));
- return derived_instance;
+ return HolderType::Make(node);
}
arrow::Result<FunctionHolderPtr> FunctionHolderMakerRegistry::Make(
diff --git a/cpp/src/gandiva/interval_holder.cc
b/cpp/src/gandiva/interval_holder.cc
index d63a11a10d..70f7792635 100644
--- a/cpp/src/gandiva/interval_holder.cc
+++ b/cpp/src/gandiva/interval_holder.cc
@@ -258,26 +258,26 @@ int64_t IntervalDaysHolder::operator()(ExecutionContext*
ctx, const char* data,
return 0;
}
-Status IntervalDaysHolder::Make(const FunctionNode& node,
- std::shared_ptr<IntervalDaysHolder>* holder) {
+Result<std::shared_ptr<IntervalDaysHolder>> IntervalDaysHolder::Make(
+ const FunctionNode& node) {
const std::string function_name("castINTERVALDAY");
- return IntervalHolder<IntervalDaysHolder>::Make(node, holder, function_name);
+ return IntervalHolder<IntervalDaysHolder>::Make(node, function_name);
}
-Status IntervalDaysHolder::Make(int32_t suppress_errors,
- std::shared_ptr<IntervalDaysHolder>* holder) {
- return IntervalHolder<IntervalDaysHolder>::Make(suppress_errors, holder);
+Result<std::shared_ptr<IntervalDaysHolder>> IntervalDaysHolder::Make(
+ int32_t suppress_errors) {
+ return IntervalHolder<IntervalDaysHolder>::Make(suppress_errors);
}
-Status IntervalYearsHolder::Make(const FunctionNode& node,
- std::shared_ptr<IntervalYearsHolder>* holder)
{
+Result<std::shared_ptr<IntervalYearsHolder>> IntervalYearsHolder::Make(
+ const FunctionNode& node) {
const std::string function_name("castINTERVALYEAR");
- return IntervalHolder<IntervalYearsHolder>::Make(node, holder,
function_name);
+ return IntervalHolder<IntervalYearsHolder>::Make(node, function_name);
}
-Status IntervalYearsHolder::Make(int32_t suppress_errors,
- std::shared_ptr<IntervalYearsHolder>* holder)
{
- return IntervalHolder<IntervalYearsHolder>::Make(suppress_errors, holder);
+Result<std::shared_ptr<IntervalYearsHolder>> IntervalYearsHolder::Make(
+ int32_t suppress_errors) {
+ return IntervalHolder<IntervalYearsHolder>::Make(suppress_errors);
}
// The operator will cast a generic string defined by the user into an
interval of months.
diff --git a/cpp/src/gandiva/interval_holder.h
b/cpp/src/gandiva/interval_holder.h
index 38d8e9f86a..0a6a988025 100644
--- a/cpp/src/gandiva/interval_holder.h
+++ b/cpp/src/gandiva/interval_holder.h
@@ -39,8 +39,8 @@ class GANDIVA_EXPORT IntervalHolder : public FunctionHolder {
~IntervalHolder() override = default;
protected:
- static Status Make(const FunctionNode& node, std::shared_ptr<INTERVAL_TYPE>*
holder,
- const std::string& function_name) {
+ static Result<std::shared_ptr<INTERVAL_TYPE>> Make(const FunctionNode& node,
+ const std::string&
function_name) {
ARROW_RETURN_IF(node.children().size() != 1 && node.children().size() != 2,
Status::Invalid(function_name + " requires one or two
parameters"));
@@ -63,14 +63,11 @@ class GANDIVA_EXPORT IntervalHolder : public FunctionHolder
{
suppress_errors = std::get<int>(literal_suppress_errors->holder());
}
- return Make(suppress_errors, holder);
+ return Make(suppress_errors);
}
- static Status Make(int32_t suppress_errors, std::shared_ptr<INTERVAL_TYPE>*
holder) {
- auto lholder = std::make_shared<INTERVAL_TYPE>(suppress_errors);
-
- *holder = lholder;
- return Status::OK();
+ static Result<std::shared_ptr<INTERVAL_TYPE>> Make(int32_t suppress_errors) {
+ return std::make_shared<INTERVAL_TYPE>(suppress_errors);
}
explicit IntervalHolder(int32_t supress_errors) :
suppress_errors_(supress_errors) {}
@@ -94,11 +91,9 @@ class GANDIVA_EXPORT IntervalDaysHolder : public
IntervalHolder<IntervalDaysHold
public:
~IntervalDaysHolder() override = default;
- static Status Make(const FunctionNode& node,
- std::shared_ptr<IntervalDaysHolder>* holder);
+ static Result<std::shared_ptr<IntervalDaysHolder>> Make(const FunctionNode&
node);
- static Status Make(int32_t suppress_errors,
- std::shared_ptr<IntervalDaysHolder>* holder);
+ static Result<std::shared_ptr<IntervalDaysHolder>> Make(int32_t
suppress_errors);
/// Cast a generic string to an interval
int64_t operator()(ExecutionContext* ctx, const char* data, int32_t data_len,
@@ -131,11 +126,9 @@ class GANDIVA_EXPORT IntervalYearsHolder : public
IntervalHolder<IntervalYearsHo
public:
~IntervalYearsHolder() override = default;
- static Status Make(const FunctionNode& node,
- std::shared_ptr<IntervalYearsHolder>* holder);
+ static Result<std::shared_ptr<IntervalYearsHolder>> Make(const FunctionNode&
node);
- static Status Make(int32_t suppress_errors,
- std::shared_ptr<IntervalYearsHolder>* holder);
+ static Result<std::shared_ptr<IntervalYearsHolder>> Make(int32_t
suppress_errors);
/// Cast a generic string to an interval
int32_t operator()(ExecutionContext* ctx, const char* data, int32_t data_len,
diff --git a/cpp/src/gandiva/interval_holder_test.cc
b/cpp/src/gandiva/interval_holder_test.cc
index fbfd6335f4..59a10cace8 100644
--- a/cpp/src/gandiva/interval_holder_test.cc
+++ b/cpp/src/gandiva/interval_holder_test.cc
@@ -20,8 +20,8 @@
#include <gtest/gtest.h>
#include <memory>
-#include <vector>
+#include "arrow/testing/gtest_util.h"
#include "gandiva/execution_context.h"
namespace gandiva {
@@ -32,14 +32,8 @@ class TestIntervalHolder : public ::testing::Test {
};
TEST_F(TestIntervalHolder, TestMatchAllPeriods) {
- std::shared_ptr<IntervalDaysHolder> interval_days_holder;
- std::shared_ptr<IntervalYearsHolder> interval_years_holder;
-
- auto status = IntervalDaysHolder::Make(0, &interval_days_holder);
- EXPECT_EQ(status.ok(), true) << status.message();
-
- status = IntervalYearsHolder::Make(0, &interval_years_holder);
- EXPECT_EQ(status.ok(), true) << status.message();
+ EXPECT_OK_AND_ASSIGN(auto interval_days_holder, IntervalDaysHolder::Make(0));
+ EXPECT_OK_AND_ASSIGN(auto interval_years_holder,
IntervalYearsHolder::Make(0));
auto& cast_interval_day = *interval_days_holder;
auto& cast_interval_year = *interval_years_holder;
@@ -289,14 +283,8 @@ TEST_F(TestIntervalHolder, TestMatchAllPeriods) {
}
TEST_F(TestIntervalHolder, TestMatchErrorsForCastIntervalDay) {
- std::shared_ptr<IntervalDaysHolder> interval_days_holder;
- std::shared_ptr<IntervalYearsHolder> interval_years_holder;
-
- auto status = IntervalDaysHolder::Make(0, &interval_days_holder);
- EXPECT_EQ(status.ok(), true) << status.message();
-
- status = IntervalYearsHolder::Make(0, &interval_years_holder);
- EXPECT_EQ(status.ok(), true) << status.message();
+ EXPECT_OK_AND_ASSIGN(auto interval_days_holder, IntervalDaysHolder::Make(0));
+ EXPECT_OK_AND_ASSIGN(auto interval_years_holder,
IntervalYearsHolder::Make(0));
auto& cast_interval_day = *interval_days_holder;
auto& cast_interval_year = *interval_years_holder;
@@ -440,12 +428,8 @@ TEST_F(TestIntervalHolder,
TestMatchErrorsForCastIntervalDay) {
}
TEST_F(TestIntervalHolder, TestUsingWeekFormatterForCastIntervalDay) {
- std::shared_ptr<IntervalDaysHolder> interval_holder;
-
- auto status = IntervalDaysHolder::Make(0, &interval_holder);
- EXPECT_EQ(status.ok(), true) << status.message();
-
- auto& cast_interval_day = *interval_holder;
+ EXPECT_OK_AND_ASSIGN(auto interval_days_holder, IntervalDaysHolder::Make(0));
+ auto& cast_interval_day = *interval_days_holder;
bool out_valid;
std::string data("P1W");
@@ -465,12 +449,8 @@ TEST_F(TestIntervalHolder,
TestUsingWeekFormatterForCastIntervalDay) {
}
TEST_F(TestIntervalHolder, TestUsingCompleteFormatterForCastIntervalDay) {
- std::shared_ptr<IntervalDaysHolder> interval_holder;
-
- auto status = IntervalDaysHolder::Make(0, &interval_holder);
- EXPECT_EQ(status.ok(), true) << status.message();
-
- auto& cast_interval_day = *interval_holder;
+ EXPECT_OK_AND_ASSIGN(auto interval_days_holder, IntervalDaysHolder::Make(0));
+ auto& cast_interval_day = *interval_days_holder;
bool out_valid;
std::string data("1742461111");
@@ -528,11 +508,7 @@ TEST_F(TestIntervalHolder,
TestUsingCompleteFormatterForCastIntervalDay) {
}
TEST_F(TestIntervalHolder, TestUsingCompleteFormatterForCastIntervalYear) {
- std::shared_ptr<IntervalYearsHolder> interval_years_holder;
-
- auto status = IntervalYearsHolder::Make(0, &interval_years_holder);
- EXPECT_EQ(status.ok(), true) << status.message();
-
+ EXPECT_OK_AND_ASSIGN(auto interval_years_holder,
IntervalYearsHolder::Make(0));
auto& cast_interval_years = *interval_years_holder;
bool out_valid;
diff --git a/cpp/src/gandiva/random_generator_holder.cc
b/cpp/src/gandiva/random_generator_holder.cc
index 3d395741d7..8f80c5826d 100644
--- a/cpp/src/gandiva/random_generator_holder.cc
+++ b/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());
}
auto literal = dynamic_cast<LiteralNode*>(node.children().at(0).get());
@@ -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(
literal->is_null() ? 0 : std::get<int32_t>(literal->holder())));
- return Status::OK();
}
} // namespace gandiva
diff --git a/cpp/src/gandiva/random_generator_holder.h
b/cpp/src/gandiva/random_generator_holder.h
index 65b6607e87..ffab725aa7 100644
--- a/cpp/src/gandiva/random_generator_holder.h
+++ b/cpp/src/gandiva/random_generator_holder.h
@@ -34,8 +34,7 @@ class GANDIVA_EXPORT RandomGeneratorHolder : public
FunctionHolder {
public:
~RandomGeneratorHolder() override = default;
- static Status Make(const FunctionNode& node,
- std::shared_ptr<RandomGeneratorHolder>* holder);
+ static Result<std::shared_ptr<RandomGeneratorHolder>> Make(const
FunctionNode& node);
double operator()() { return distribution_(generator_); }
diff --git a/cpp/src/gandiva/random_generator_holder_test.cc
b/cpp/src/gandiva/random_generator_holder_test.cc
index 4b16c1b7d0..77b2750f2e 100644
--- a/cpp/src/gandiva/random_generator_holder_test.cc
+++ b/cpp/src/gandiva/random_generator_holder_test.cc
@@ -21,38 +21,35 @@
#include <gtest/gtest.h>
+#include "arrow/testing/gtest_util.h"
+
namespace gandiva {
class TestRandGenHolder : public ::testing::Test {
public:
- FunctionNode BuildRandFunc() { return FunctionNode("random", {},
arrow::float64()); }
+ FunctionNode BuildRandFunc() { return {"random", {}, arrow::float64()}; }
FunctionNode BuildRandWithSeedFunc(int32_t seed, bool seed_is_null) {
auto seed_node =
std::make_shared<LiteralNode>(arrow::int32(), LiteralHolder(seed),
seed_is_null);
- return FunctionNode("rand", {seed_node}, arrow::float64());
+ return {"rand", {seed_node}, arrow::float64()};
}
};
TEST_F(TestRandGenHolder, NoSeed) {
- std::shared_ptr<RandomGeneratorHolder> rand_gen_holder;
FunctionNode rand_func = BuildRandFunc();
- auto status = RandomGeneratorHolder::Make(rand_func, &rand_gen_holder);
- EXPECT_EQ(status.ok(), true) << status.message();
+ EXPECT_OK_AND_ASSIGN(auto rand_gen_holder,
RandomGeneratorHolder::Make(rand_func));
auto& random = *rand_gen_holder;
EXPECT_NE(random(), random());
}
TEST_F(TestRandGenHolder, WithValidEqualSeeds) {
- std::shared_ptr<RandomGeneratorHolder> rand_gen_holder_1;
- std::shared_ptr<RandomGeneratorHolder> rand_gen_holder_2;
FunctionNode rand_func_1 = BuildRandWithSeedFunc(12, false);
FunctionNode rand_func_2 = BuildRandWithSeedFunc(12, false);
- auto status = RandomGeneratorHolder::Make(rand_func_1, &rand_gen_holder_1);
- EXPECT_EQ(status.ok(), true) << status.message();
- status = RandomGeneratorHolder::Make(rand_func_2, &rand_gen_holder_2);
- EXPECT_EQ(status.ok(), true) << status.message();
+
+ EXPECT_OK_AND_ASSIGN(auto rand_gen_holder_1,
RandomGeneratorHolder::Make(rand_func_1));
+ EXPECT_OK_AND_ASSIGN(auto rand_gen_holder_2,
RandomGeneratorHolder::Make(rand_func_2));
auto& random_1 = *rand_gen_holder_1;
auto& random_2 = *rand_gen_holder_2;
@@ -65,18 +62,12 @@ TEST_F(TestRandGenHolder, WithValidEqualSeeds) {
}
TEST_F(TestRandGenHolder, WithValidSeeds) {
- std::shared_ptr<RandomGeneratorHolder> rand_gen_holder_1;
- std::shared_ptr<RandomGeneratorHolder> rand_gen_holder_2;
- std::shared_ptr<RandomGeneratorHolder> rand_gen_holder_3;
FunctionNode rand_func_1 = BuildRandWithSeedFunc(11, false);
FunctionNode rand_func_2 = BuildRandWithSeedFunc(12, false);
FunctionNode rand_func_3 = BuildRandWithSeedFunc(-12, false);
- auto status = RandomGeneratorHolder::Make(rand_func_1, &rand_gen_holder_1);
- EXPECT_EQ(status.ok(), true) << status.message();
- status = RandomGeneratorHolder::Make(rand_func_2, &rand_gen_holder_2);
- EXPECT_EQ(status.ok(), true) << status.message();
- status = RandomGeneratorHolder::Make(rand_func_3, &rand_gen_holder_3);
- EXPECT_EQ(status.ok(), true) << status.message();
+ EXPECT_OK_AND_ASSIGN(auto rand_gen_holder_1,
RandomGeneratorHolder::Make(rand_func_1));
+ EXPECT_OK_AND_ASSIGN(auto rand_gen_holder_2,
RandomGeneratorHolder::Make(rand_func_2));
+ EXPECT_OK_AND_ASSIGN(auto rand_gen_holder_3,
RandomGeneratorHolder::Make(rand_func_3));
auto& random_1 = *rand_gen_holder_1;
auto& random_2 = *rand_gen_holder_2;
@@ -86,14 +77,10 @@ TEST_F(TestRandGenHolder, WithValidSeeds) {
}
TEST_F(TestRandGenHolder, WithInValidSeed) {
- std::shared_ptr<RandomGeneratorHolder> rand_gen_holder_1;
- std::shared_ptr<RandomGeneratorHolder> rand_gen_holder_2;
FunctionNode rand_func_1 = BuildRandWithSeedFunc(12, true);
FunctionNode rand_func_2 = BuildRandWithSeedFunc(0, false);
- auto status = RandomGeneratorHolder::Make(rand_func_1, &rand_gen_holder_1);
- EXPECT_EQ(status.ok(), true) << status.message();
- status = RandomGeneratorHolder::Make(rand_func_2, &rand_gen_holder_2);
- EXPECT_EQ(status.ok(), true) << status.message();
+ EXPECT_OK_AND_ASSIGN(auto rand_gen_holder_1,
RandomGeneratorHolder::Make(rand_func_1));
+ EXPECT_OK_AND_ASSIGN(auto rand_gen_holder_2,
RandomGeneratorHolder::Make(rand_func_2));
auto& random_1 = *rand_gen_holder_1;
auto& random_2 = *rand_gen_holder_2;
diff --git a/cpp/src/gandiva/regex_functions_holder.cc
b/cpp/src/gandiva/regex_functions_holder.cc
index 30d68cbc87..03a4af90d8 100644
--- a/cpp/src/gandiva/regex_functions_holder.cc
+++ b/cpp/src/gandiva/regex_functions_holder.cc
@@ -48,9 +48,9 @@ const FunctionNode LikeHolder::TryOptimize(const
FunctionNode& node) {
global_checked = true;
}
- std::shared_ptr<LikeHolder> holder;
- auto status = Make(node, &holder);
- if (status.ok()) {
+ auto maybe_holder = Make(node);
+ if (maybe_holder.ok()) {
+ auto holder = *maybe_holder;
std::string& pattern = holder->pattern_;
auto literal_type = node.children().at(1)->return_type();
@@ -83,7 +83,7 @@ const FunctionNode LikeHolder::TryOptimize(const
FunctionNode& node) {
return node;
}
-Status LikeHolder::Make(const FunctionNode& node, std::shared_ptr<LikeHolder>*
holder) {
+Result<std::shared_ptr<LikeHolder>> LikeHolder::Make(const FunctionNode& node)
{
ARROW_RETURN_IF(node.children().size() != 2 && node.children().size() != 3,
Status::Invalid("'like' function requires two or three
parameters"));
@@ -102,10 +102,10 @@ Status LikeHolder::Make(const FunctionNode& node,
std::shared_ptr<LikeHolder>* h
if (node.descriptor()->name() == "ilike") {
regex_op.set_case_sensitive(false); // set case-insensitive for ilike
function.
- return Make(std::get<std::string>(literal->holder()), holder, regex_op);
+ return Make(std::get<std::string>(literal->holder()), regex_op);
}
if (node.children().size() == 2) {
- return Make(std::get<std::string>(literal->holder()), holder);
+ return Make(std::get<std::string>(literal->holder()));
} else {
auto escape_char = dynamic_cast<LiteralNode*>(node.children().at(2).get());
ARROW_RETURN_IF(
@@ -118,12 +118,11 @@ Status LikeHolder::Make(const FunctionNode& node,
std::shared_ptr<LikeHolder>* h
Status::Invalid(
"'like' function requires a string literal as the third
parameter"));
return Make(std::get<std::string>(literal->holder()),
- std::get<std::string>(escape_char->holder()), holder);
+ std::get<std::string>(escape_char->holder()));
}
}
-Status LikeHolder::Make(const std::string& sql_pattern,
- std::shared_ptr<LikeHolder>* holder) {
+Result<std::shared_ptr<LikeHolder>> LikeHolder::Make(const std::string&
sql_pattern) {
std::string pcre_pattern;
ARROW_RETURN_NOT_OK(RegexUtil::SqlLikePatternToPcre(sql_pattern,
pcre_pattern));
@@ -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 lholder;
}
-Status LikeHolder::Make(const std::string& sql_pattern, const std::string&
escape_char,
- std::shared_ptr<LikeHolder>* holder) {
+Result<std::shared_ptr<LikeHolder>> LikeHolder::Make(const std::string&
sql_pattern,
+ const std::string&
escape_char) {
ARROW_RETURN_IF(escape_char.length() > 1,
Status::Invalid("The length of escape char ", escape_char,
" in 'like' function is greater than 1"));
@@ -154,28 +152,24 @@ 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 lholder;
}
-Status LikeHolder::Make(const std::string& sql_pattern,
- std::shared_ptr<LikeHolder>* holder, RE2::Options
regex_op) {
+Result<std::shared_ptr<LikeHolder>> LikeHolder::Make(const std::string&
sql_pattern,
+ RE2::Options regex_op) {
std::string pcre_pattern;
ARROW_RETURN_NOT_OK(RegexUtil::SqlLikePatternToPcre(sql_pattern,
pcre_pattern));
- std::shared_ptr<LikeHolder> lholder;
- lholder = std::shared_ptr<LikeHolder>(new LikeHolder(pcre_pattern,
regex_op));
+ auto lholder = std::shared_ptr<LikeHolder>(new LikeHolder(pcre_pattern,
regex_op));
ARROW_RETURN_IF(!lholder->regex_.ok(),
Status::Invalid("Building RE2 pattern '", pcre_pattern,
"' failed with: ", lholder->regex_.error()));
- *holder = std::move(lholder);
- return Status::OK();
+ return lholder;
}
-Status ReplaceHolder::Make(const FunctionNode& node,
- std::shared_ptr<ReplaceHolder>* holder) {
+Result<std::shared_ptr<ReplaceHolder>> ReplaceHolder::Make(const FunctionNode&
node) {
ARROW_RETURN_IF(node.children().size() != 3,
Status::Invalid("'replace' function requires three
parameters"));
@@ -190,18 +184,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));
ARROW_RETURN_IF(!lholder->regex_.ok(),
Status::Invalid("Building RE2 pattern '", sql_pattern,
"' failed with: ", lholder->regex_.error()));
- *holder = std::move(lholder);
- return Status::OK();
+ return lholder;
}
void ReplaceHolder::return_error(ExecutionContext* context, std::string& data,
@@ -211,8 +204,7 @@ void ReplaceHolder::return_error(ExecutionContext* context,
std::string& data,
context->set_error_msg(err_msg.c_str());
}
-Status ExtractHolder::Make(const FunctionNode& node,
- std::shared_ptr<ExtractHolder>* holder) {
+Result<std::shared_ptr<ExtractHolder>> ExtractHolder::Make(const FunctionNode&
node) {
ARROW_RETURN_IF(node.children().size() != 3,
Status::Invalid("'extract' function requires three
parameters"));
@@ -221,18 +213,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));
ARROW_RETURN_IF(!lholder->regex_.ok(),
Status::Invalid("Building RE2 pattern '", sql_pattern,
"' failed with: ", lholder->regex_.error()));
- *holder = std::move(lholder);
- return Status::OK();
+ return lholder;
}
const char* ExtractHolder::operator()(ExecutionContext* ctx, const char*
user_input,
diff --git a/cpp/src/gandiva/regex_functions_holder.h
b/cpp/src/gandiva/regex_functions_holder.h
index ecf4095f3d..36d942510b 100644
--- a/cpp/src/gandiva/regex_functions_holder.h
+++ b/cpp/src/gandiva/regex_functions_holder.h
@@ -35,15 +35,15 @@ class GANDIVA_EXPORT LikeHolder : public FunctionHolder {
public:
~LikeHolder() override = default;
- static Status Make(const FunctionNode& node, std::shared_ptr<LikeHolder>*
holder);
+ static Result<std::shared_ptr<LikeHolder>> Make(const FunctionNode& node);
- static Status Make(const std::string& sql_pattern,
std::shared_ptr<LikeHolder>* holder);
+ static Result<std::shared_ptr<LikeHolder>> Make(const std::string&
sql_pattern);
- static Status Make(const std::string& sql_pattern, const std::string&
escape_char,
- std::shared_ptr<LikeHolder>* holder);
+ static Result<std::shared_ptr<LikeHolder>> Make(const std::string&
sql_pattern,
+ const std::string&
escape_char);
- static Status Make(const std::string& sql_pattern,
std::shared_ptr<LikeHolder>* holder,
- RE2::Options regex_op);
+ static Result<std::shared_ptr<LikeHolder>> Make(const std::string&
sql_pattern,
+ RE2::Options regex_op);
// Try and optimise a function node with a "like" pattern.
static const FunctionNode TryOptimize(const FunctionNode& node);
@@ -66,10 +66,9 @@ class GANDIVA_EXPORT ReplaceHolder : public FunctionHolder {
public:
~ReplaceHolder() override = default;
- static Status Make(const FunctionNode& node, std::shared_ptr<ReplaceHolder>*
holder);
+ static Result<std::shared_ptr<ReplaceHolder>> Make(const FunctionNode& node);
- static Status Make(const std::string& sql_pattern,
- std::shared_ptr<ReplaceHolder>* holder);
+ static Result<std::shared_ptr<ReplaceHolder>> Make(const std::string&
sql_pattern);
/// Return a new string with the pattern that matched the regex replaced for
/// the replace_input parameter.
@@ -130,10 +129,9 @@ class GANDIVA_EXPORT ExtractHolder : public FunctionHolder
{
public:
~ExtractHolder() override = default;
- static Status Make(const FunctionNode& node, std::shared_ptr<ExtractHolder>*
holder);
+ static Result<std::shared_ptr<ExtractHolder>> Make(const FunctionNode& node);
- static Status Make(const std::string& sql_pattern,
- std::shared_ptr<ExtractHolder>* holder);
+ static Result<std::shared_ptr<ExtractHolder>> Make(const std::string&
sql_pattern);
/// Extracts the matching text from a string using a regex
const char* operator()(ExecutionContext* ctx, const char* user_input,
diff --git a/cpp/src/gandiva/regex_functions_holder_test.cc
b/cpp/src/gandiva/regex_functions_holder_test.cc
index 930f3a7ade..534be5987a 100644
--- a/cpp/src/gandiva/regex_functions_holder_test.cc
+++ b/cpp/src/gandiva/regex_functions_holder_test.cc
@@ -19,7 +19,8 @@
#include <gmock/gmock.h>
#include <gtest/gtest.h>
#include <memory>
-#include <vector>
+
+#include "arrow/testing/gtest_util.h"
#include "gandiva/regex_util.h"
namespace gandiva {
@@ -31,7 +32,7 @@ class TestLikeHolder : public ::testing::Test {
auto field = std::make_shared<FieldNode>(arrow::field("in",
arrow::utf8()));
auto pattern_node =
std::make_shared<LiteralNode>(arrow::utf8(), LiteralHolder(pattern),
false);
- return FunctionNode("like", {field, pattern_node}, arrow::boolean());
+ return {"like", {field, pattern_node}, arrow::boolean()};
}
FunctionNode BuildLike(std::string pattern, char escape_char) {
@@ -40,16 +41,12 @@ class TestLikeHolder : public ::testing::Test {
std::make_shared<LiteralNode>(arrow::utf8(), LiteralHolder(pattern),
false);
auto escape_char_node = std::make_shared<LiteralNode>(
arrow::utf8(), LiteralHolder(std::string(1, escape_char)), false);
- return FunctionNode("like", {field, pattern_node, escape_char_node},
- arrow::boolean());
+ return {"like", {field, pattern_node, escape_char_node}, arrow::boolean()};
}
};
TEST_F(TestLikeHolder, TestMatchAny) {
- std::shared_ptr<LikeHolder> like_holder;
-
- auto status = LikeHolder::Make("ab%", &like_holder, regex_op);
- EXPECT_EQ(status.ok(), true) << status.message();
+ EXPECT_OK_AND_ASSIGN(auto const like_holder, LikeHolder::Make("ab%",
regex_op));
auto& like = *like_holder;
EXPECT_TRUE(like("ab"));
@@ -61,10 +58,7 @@ TEST_F(TestLikeHolder, TestMatchAny) {
}
TEST_F(TestLikeHolder, TestMatchOne) {
- std::shared_ptr<LikeHolder> like_holder;
-
- auto status = LikeHolder::Make("ab_", &like_holder, regex_op);
- EXPECT_EQ(status.ok(), true) << status.message();
+ EXPECT_OK_AND_ASSIGN(auto const like_holder, LikeHolder::Make("ab_",
regex_op));
auto& like = *like_holder;
EXPECT_TRUE(like("abc"));
@@ -76,10 +70,7 @@ TEST_F(TestLikeHolder, TestMatchOne) {
}
TEST_F(TestLikeHolder, TestPcreSpecial) {
- std::shared_ptr<LikeHolder> like_holder;
-
- auto status = LikeHolder::Make(".*ab_", &like_holder, regex_op);
- EXPECT_EQ(status.ok(), true) << status.message();
+ EXPECT_OK_AND_ASSIGN(auto const like_holder, LikeHolder::Make(".*ab_",
regex_op));
auto& like = *like_holder;
EXPECT_TRUE(like(".*abc")); // . and * aren't special in sql regex
@@ -88,34 +79,26 @@ TEST_F(TestLikeHolder, TestPcreSpecial) {
TEST_F(TestLikeHolder, TestRegexEscape) {
std::string res;
- auto status = RegexUtil::SqlLikePatternToPcre("#%hello#_abc_def##", '#',
res);
- EXPECT_TRUE(status.ok()) << status.message();
+ ARROW_EXPECT_OK(RegexUtil::SqlLikePatternToPcre("#%hello#_abc_def##", '#',
res));
EXPECT_EQ(res, "%hello_abc.def#");
}
TEST_F(TestLikeHolder, TestDot) {
- std::shared_ptr<LikeHolder> like_holder;
-
- auto status = LikeHolder::Make("abc.", &like_holder, regex_op);
- EXPECT_EQ(status.ok(), true) << status.message();
+ EXPECT_OK_AND_ASSIGN(auto const like_holder, LikeHolder::Make("abc.",
regex_op));
auto& like = *like_holder;
EXPECT_FALSE(like("abcd"));
}
TEST_F(TestLikeHolder, TestMatchSubString) {
- std::shared_ptr<LikeHolder> like_holder;
-
- auto status = LikeHolder::Make("%abc%", "\\", &like_holder);
- EXPECT_EQ(status.ok(), true) << status.message();
+ EXPECT_OK_AND_ASSIGN(auto like_holder, LikeHolder::Make("%abc%", "\\"));
auto& like = *like_holder;
EXPECT_TRUE(like("abc"));
EXPECT_FALSE(like("xxabdc"));
- status = LikeHolder::Make("%ab-.^$*+?()[]{}|—/c\\%%", "\\", &like_holder);
- EXPECT_EQ(status.ok(), true) << status.message();
+ EXPECT_OK_AND_ASSIGN(like_holder,
LikeHolder::Make("%ab-.^$*+?()[]{}|—/c\\%%", "\\"));
auto& like_reserved_char = *like_holder;
EXPECT_TRUE(like_reserved_char("XXab-.^$*+?()[]{}|—/c%d"));
@@ -190,10 +173,7 @@ TEST_F(TestLikeHolder, TestOptimise) {
}
TEST_F(TestLikeHolder, TestMatchOneEscape) {
- std::shared_ptr<LikeHolder> like_holder;
-
- auto status = LikeHolder::Make("ab\\_", "\\", &like_holder);
- EXPECT_EQ(status.ok(), true) << status.message();
+ EXPECT_OK_AND_ASSIGN(auto const like_holder, LikeHolder::Make("ab\\_",
"\\"));
auto& like = *like_holder;
@@ -207,10 +187,7 @@ TEST_F(TestLikeHolder, TestMatchOneEscape) {
}
TEST_F(TestLikeHolder, TestMatchManyEscape) {
- std::shared_ptr<LikeHolder> like_holder;
-
- auto status = LikeHolder::Make("ab\\%", "\\", &like_holder);
- EXPECT_EQ(status.ok(), true) << status.message();
+ EXPECT_OK_AND_ASSIGN(auto const like_holder, LikeHolder::Make("ab\\%",
"\\"));
auto& like = *like_holder;
@@ -224,10 +201,7 @@ TEST_F(TestLikeHolder, TestMatchManyEscape) {
}
TEST_F(TestLikeHolder, TestMatchEscape) {
- std::shared_ptr<LikeHolder> like_holder;
-
- auto status = LikeHolder::Make("ab\\\\", "\\", &like_holder);
- EXPECT_EQ(status.ok(), true) << status.message();
+ EXPECT_OK_AND_ASSIGN(auto const like_holder, LikeHolder::Make("ab\\\\",
"\\"));
auto& like = *like_holder;
@@ -237,10 +211,7 @@ TEST_F(TestLikeHolder, TestMatchEscape) {
}
TEST_F(TestLikeHolder, TestEmptyEscapeChar) {
- std::shared_ptr<LikeHolder> like_holder;
-
- auto status = LikeHolder::Make("ab\\_", "", &like_holder);
- EXPECT_EQ(status.ok(), true) << status.message();
+ EXPECT_OK_AND_ASSIGN(auto const like_holder, LikeHolder::Make("ab\\_", ""));
auto& like = *like_holder;
@@ -252,10 +223,7 @@ TEST_F(TestLikeHolder, TestEmptyEscapeChar) {
}
TEST_F(TestLikeHolder, TestMultipleEscapeChar) {
- std::shared_ptr<LikeHolder> like_holder;
-
- auto status = LikeHolder::Make("ab\\_", "\\\\", &like_holder);
- EXPECT_EQ(status.ok(), false) << status.message();
+ ASSERT_RAISES(Invalid, LikeHolder::Make("ab\\_", "\\\\").status());
}
class TestILikeHolder : public ::testing::Test {
@@ -265,16 +233,14 @@ class TestILikeHolder : public ::testing::Test {
auto field = std::make_shared<FieldNode>(arrow::field("in",
arrow::utf8()));
auto pattern_node =
std::make_shared<LiteralNode>(arrow::utf8(), LiteralHolder(pattern),
false);
- return FunctionNode("ilike", {field, pattern_node}, arrow::boolean());
+ return {"ilike", {field, pattern_node}, arrow::boolean()};
}
};
TEST_F(TestILikeHolder, TestMatchAny) {
- std::shared_ptr<LikeHolder> like_holder;
-
regex_op.set_case_sensitive(false);
- auto status = LikeHolder::Make("ab%", &like_holder, regex_op);
- EXPECT_EQ(status.ok(), true) << status.message();
+
+ EXPECT_OK_AND_ASSIGN(auto const like_holder, LikeHolder::Make("ab%",
regex_op));
auto& like = *like_holder;
EXPECT_TRUE(like("ab"));
@@ -286,11 +252,8 @@ TEST_F(TestILikeHolder, TestMatchAny) {
}
TEST_F(TestILikeHolder, TestMatchOne) {
- std::shared_ptr<LikeHolder> like_holder;
-
regex_op.set_case_sensitive(false);
- auto status = LikeHolder::Make("Ab_", &like_holder, regex_op);
- EXPECT_EQ(status.ok(), true) << status.message();
+ EXPECT_OK_AND_ASSIGN(auto const like_holder, LikeHolder::Make("Ab_",
regex_op));
auto& like = *like_holder;
EXPECT_TRUE(like("abc"));
@@ -302,11 +265,8 @@ TEST_F(TestILikeHolder, TestMatchOne) {
}
TEST_F(TestILikeHolder, TestPcreSpecial) {
- std::shared_ptr<LikeHolder> like_holder;
-
regex_op.set_case_sensitive(false);
- auto status = LikeHolder::Make(".*aB_", &like_holder, regex_op);
- EXPECT_EQ(status.ok(), true) << status.message();
+ EXPECT_OK_AND_ASSIGN(auto const like_holder, LikeHolder::Make(".*aB_",
regex_op));
auto& like = *like_holder;
EXPECT_TRUE(like(".*Abc")); // . and * aren't special in sql regex
@@ -314,11 +274,8 @@ TEST_F(TestILikeHolder, TestPcreSpecial) {
}
TEST_F(TestILikeHolder, TestDot) {
- std::shared_ptr<LikeHolder> like_holder;
-
regex_op.set_case_sensitive(false);
- auto status = LikeHolder::Make("aBc.", &like_holder, regex_op);
- EXPECT_EQ(status.ok(), true) << status.message();
+ EXPECT_OK_AND_ASSIGN(auto const like_holder, LikeHolder::Make("aBc.",
regex_op));
auto& like = *like_holder;
EXPECT_FALSE(like("abcd"));
@@ -330,10 +287,7 @@ 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();
+ EXPECT_OK_AND_ASSIGN(auto const replace_holder, ReplaceHolder::Make("ana"));
std::string input_string = "banana";
std::string replace_string;
@@ -378,10 +332,7 @@ TEST_F(TestReplaceHolder, TestMultipleReplace) {
}
TEST_F(TestReplaceHolder, TestNoMatchPattern) {
- std::shared_ptr<ReplaceHolder> replace_holder;
-
- auto status = ReplaceHolder::Make("ana", &replace_holder);
- EXPECT_EQ(status.ok(), true) << status.message();
+ EXPECT_OK_AND_ASSIGN(auto const replace_holder, ReplaceHolder::Make("ana"));
std::string input_string = "apple";
std::string replace_string;
@@ -398,10 +349,7 @@ TEST_F(TestReplaceHolder, TestNoMatchPattern) {
}
TEST_F(TestReplaceHolder, TestReplaceSameSize) {
- std::shared_ptr<ReplaceHolder> replace_holder;
-
- auto status = ReplaceHolder::Make("a", &replace_holder);
- EXPECT_EQ(status.ok(), true) << status.message();
+ EXPECT_OK_AND_ASSIGN(auto const replace_holder, ReplaceHolder::Make("a"));
std::string input_string = "ananindeua";
std::string replace_string = "b";
@@ -418,11 +366,7 @@ TEST_F(TestReplaceHolder, TestReplaceSameSize) {
}
TEST_F(TestReplaceHolder, TestReplaceInvalidPattern) {
- std::shared_ptr<ReplaceHolder> replace_holder;
-
- auto status = ReplaceHolder::Make("+", &replace_holder);
- EXPECT_EQ(status.ok(), false) << status.message();
-
+ ASSERT_RAISES(Invalid, ReplaceHolder::Make("+"));
execution_context_.Reset();
}
@@ -433,11 +377,8 @@ class TestExtractHolder : public ::testing::Test {
};
TEST_F(TestExtractHolder, TestSimpleExtract) {
- std::shared_ptr<ExtractHolder> extract_holder;
-
// Pattern to match of two group of letters
- auto status = ExtractHolder::Make(R"((\w+) (\w+))", &extract_holder);
- EXPECT_EQ(status.ok(), true) << status.message();
+ EXPECT_OK_AND_ASSIGN(auto extract_holder, ExtractHolder::Make(R"((\w+)
(\w+))"));
std::string input_string = "John Doe";
int32_t extract_index = 2; // Retrieve the surname
@@ -469,8 +410,7 @@ TEST_F(TestExtractHolder, TestSimpleExtract) {
EXPECT_EQ(out_length, 9);
EXPECT_EQ(ret_as_str, "Paul Test");
- status = ExtractHolder::Make(R"((\w+) (\w+) - (\d+))", &extract_holder);
- EXPECT_EQ(status.ok(), true) << status.message();
+ EXPECT_OK_AND_ASSIGN(extract_holder, ExtractHolder::Make(R"((\w+) (\w+) -
(\d+))"));
auto& extract2 = *extract_holder;
@@ -502,8 +442,7 @@ TEST_F(TestExtractHolder, TestSimpleExtract) {
EXPECT_EQ(ret_as_str, "John Doe - 124");
// Pattern to match only numbers
- status = ExtractHolder::Make(R"(((\w+)))", &extract_holder);
- EXPECT_EQ(status.ok(), true) << status.message();
+ EXPECT_OK_AND_ASSIGN(extract_holder, ExtractHolder::Make(R"(((\w+)))"));
auto& extract_numbers = *extract_holder;
@@ -569,11 +508,8 @@ TEST_F(TestExtractHolder, TestSimpleExtract) {
}
TEST_F(TestExtractHolder, TestNoMatches) {
- std::shared_ptr<ExtractHolder> extract_holder;
-
// Pattern to match of two group of letters
- auto status = ExtractHolder::Make(R"((\w+) (\w+))", &extract_holder);
- EXPECT_EQ(status.ok(), true) << status.message();
+ EXPECT_OK_AND_ASSIGN(auto extract_holder, ExtractHolder::Make(R"((\w+)
(\w+))"));
std::string input_string = "John";
int32_t extract_index = 2; // The regex will not match with the input string
@@ -588,8 +524,7 @@ TEST_F(TestExtractHolder, TestNoMatches) {
EXPECT_FALSE(execution_context_.has_error());
// Pattern to match only numbers
- status = ExtractHolder::Make(R"(\d+)", &extract_holder);
- EXPECT_EQ(status.ok(), true) << status.message();
+ EXPECT_OK_AND_ASSIGN(extract_holder, ExtractHolder::Make(R"(\d+)"));
auto& extract_numbers = *extract_holder;
@@ -616,11 +551,8 @@ TEST_F(TestExtractHolder, TestNoMatches) {
}
TEST_F(TestExtractHolder, TestInvalidRange) {
- std::shared_ptr<ExtractHolder> extract_holder;
-
// Pattern to match of two group of letters
- auto status = ExtractHolder::Make(R"((\w+) (\w+))", &extract_holder);
- EXPECT_EQ(status.ok(), true) << status.message();
+ EXPECT_OK_AND_ASSIGN(auto const extract_holder, ExtractHolder::Make(R"((\w+)
(\w+))"));
std::string input_string = "John Doe";
int32_t extract_index = -1;
@@ -650,17 +582,11 @@ 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();
-
+ ASSERT_RAISES(Invalid, ExtractHolder::Make("+"));
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>(
@@ -668,10 +594,10 @@ TEST_F(TestExtractHolder, TestErrorWhileBuildingHolder) {
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(),
- ::testing::HasSubstr("'extract' function requires three
parameters"));
+ auto extract_holder = ExtractHolder::Make(function_node);
+ EXPECT_RAISES_WITH_MESSAGE_THAT(
+ Invalid, ::testing::HasSubstr("'extract' function requires three
parameters"),
+ extract_holder.status());
execution_context_.Reset();
@@ -682,11 +608,12 @@ 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(),
- ::testing::HasSubstr(
- "'extract' function requires a literal as the second
parameter"));
+ extract_holder = ExtractHolder::Make(function_node);
+ EXPECT_RAISES_WITH_MESSAGE_THAT(
+ Invalid,
+ ::testing::HasSubstr(
+ "'extract' function requires a literal as the second parameter"),
+ extract_holder.status());
execution_context_.Reset();
@@ -698,11 +625,12 @@ 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(),
- ::testing::HasSubstr(
- "'extract' function requires a literal as the second
parameter"));
+ extract_holder = ExtractHolder::Make(function_node);
+ EXPECT_RAISES_WITH_MESSAGE_THAT(
+ Invalid,
+ ::testing::HasSubstr(
+ "'extract' function requires a literal as the second parameter"),
+ extract_holder.status());
execution_context_.Reset();
}
diff --git a/cpp/src/gandiva/to_date_holder.cc
b/cpp/src/gandiva/to_date_holder.cc
index 27a16d1779..0778c39b4c 100644
--- a/cpp/src/gandiva/to_date_holder.cc
+++ b/cpp/src/gandiva/to_date_holder.cc
@@ -28,8 +28,7 @@
namespace gandiva {
-Status ToDateHolder::Make(const FunctionNode& node,
- std::shared_ptr<ToDateHolder>* holder) {
+Result<std::shared_ptr<ToDateHolder>> ToDateHolder::Make(const FunctionNode&
node) {
if (node.children().size() != 2 && node.children().size() != 3) {
return Status::Invalid("'to_date' function requires two or three
parameters");
}
@@ -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));
}
int64_t ToDateHolder::operator()(ExecutionContext* context, const char* data,
diff --git a/cpp/src/gandiva/to_date_holder.h b/cpp/src/gandiva/to_date_holder.h
index 1211b6a304..ac13f7a31b 100644
--- a/cpp/src/gandiva/to_date_holder.h
+++ b/cpp/src/gandiva/to_date_holder.h
@@ -35,10 +35,10 @@ class GANDIVA_EXPORT ToDateHolder : public FunctionHolder {
public:
~ToDateHolder() override = default;
- static Status Make(const FunctionNode& node, std::shared_ptr<ToDateHolder>*
holder);
+ static Result<std::shared_ptr<ToDateHolder>> Make(const FunctionNode& node);
- static Status Make(const std::string& sql_pattern, int32_t suppress_errors,
- std::shared_ptr<ToDateHolder>* holder);
+ static Result<std::shared_ptr<ToDateHolder>> Make(const std::string&
sql_pattern,
+ int32_t suppress_errors);
/// Return true if the data matches the pattern.
int64_t operator()(ExecutionContext* context, const char* data, int data_len,
diff --git a/cpp/src/gandiva/to_date_holder_test.cc
b/cpp/src/gandiva/to_date_holder_test.cc
index 99036817d8..1e0f2e1578 100644
--- a/cpp/src/gandiva/to_date_holder_test.cc
+++ b/cpp/src/gandiva/to_date_holder_test.cc
@@ -16,7 +16,6 @@
// under the License.
#include <memory>
-#include <vector>
#include "arrow/testing/gtest_util.h"
@@ -45,8 +44,7 @@ class TestToDateHolder : public ::testing::Test {
};
TEST_F(TestToDateHolder, TestSimpleDateTime) {
- std::shared_ptr<ToDateHolder> to_date_holder;
- ASSERT_OK(ToDateHolder::Make("YYYY-MM-DD HH:MI:SS", 1, &to_date_holder));
+ EXPECT_OK_AND_ASSIGN(auto to_date_holder, ToDateHolder::Make("YYYY-MM-DD
HH:MI:SS", 1));
auto& to_date = *to_date_holder;
bool out_valid;
@@ -86,8 +84,7 @@ TEST_F(TestToDateHolder, TestSimpleDateTime) {
}
TEST_F(TestToDateHolder, TestSimpleDate) {
- std::shared_ptr<ToDateHolder> to_date_holder;
- ASSERT_OK(ToDateHolder::Make("YYYY-MM-DD", 1, &to_date_holder));
+ EXPECT_OK_AND_ASSIGN(auto to_date_holder, ToDateHolder::Make("YYYY-MM-DD",
1));
auto& to_date = *to_date_holder;
bool out_valid;
@@ -119,10 +116,7 @@ TEST_F(TestToDateHolder, TestSimpleDate) {
}
TEST_F(TestToDateHolder, TestSimpleDateTimeError) {
- std::shared_ptr<ToDateHolder> to_date_holder;
-
- auto status = ToDateHolder::Make("YYYY-MM-DD HH:MI:SS", 0, &to_date_holder);
- EXPECT_EQ(status.ok(), true) << status.message();
+ EXPECT_OK_AND_ASSIGN(auto to_date_holder, ToDateHolder::Make("YYYY-MM-DD
HH:MI:SS", 0));
auto& to_date = *to_date_holder;
bool out_valid;
@@ -132,8 +126,7 @@ TEST_F(TestToDateHolder, TestSimpleDateTimeError) {
EXPECT_EQ(0, millis_since_epoch);
std::string expected_error =
"Error parsing value 1986-01-40 01:01:01 +0800 for given format";
- EXPECT_TRUE(execution_context_.get_error().find(expected_error) !=
std::string::npos)
- << status.message();
+ EXPECT_TRUE(execution_context_.get_error().find(expected_error) !=
std::string::npos);
// not valid should not return error
execution_context_.Reset();
@@ -143,15 +136,12 @@ 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);
- EXPECT_EQ(status.IsInvalid(), true) << status.message();
+ ASSERT_RAISES(Invalid, ToDateHolder::Make("YYYY-MM-DD HH:MI:SS tzo",
0).status());
}
TEST_F(TestToDateHolder, TestSimpleDateYearMonth) {
- std::shared_ptr<ToDateHolder> to_date_holder;
- ASSERT_OK(ToDateHolder::Make("YYYY-MM", 1, &to_date_holder));
+ EXPECT_OK_AND_ASSIGN(auto to_date_holder, ToDateHolder::Make("YYYY-MM", 1));
auto& to_date = *to_date_holder;
bool out_valid;
@@ -167,8 +157,7 @@ TEST_F(TestToDateHolder, TestSimpleDateYearMonth) {
}
TEST_F(TestToDateHolder, TestSimpleDateYear) {
- std::shared_ptr<ToDateHolder> to_date_holder;
- ASSERT_OK(ToDateHolder::Make("YYYY", 1, &to_date_holder));
+ EXPECT_OK_AND_ASSIGN(auto to_date_holder, ToDateHolder::Make("YYYY", 1));
auto& to_date = *to_date_holder;
bool out_valid;
