diff --git a/cpp/src/gandiva/function_holder_maker_registry.cc b/cpp/src/gandiva/function_holder_maker_registry.cc index bb93402475a..37ca1fbf6c3 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 static arrow::Result HolderMaker(const FunctionNode& node) { - std::shared_ptr derived_instance; - ARROW_RETURN_NOT_OK(HolderType::Make(node, &derived_instance)); - return derived_instance; + return HolderType::Make(node); } arrow::Result FunctionHolderMakerRegistry::Make( diff --git a/cpp/src/gandiva/interval_holder.cc b/cpp/src/gandiva/interval_holder.cc index d63a11a10d3..70f77926352 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* holder) { +Result> IntervalDaysHolder::Make( + const FunctionNode& node) { const std::string function_name("castINTERVALDAY"); - return IntervalHolder::Make(node, holder, function_name); + return IntervalHolder::Make(node, function_name); } -Status IntervalDaysHolder::Make(int32_t suppress_errors, - std::shared_ptr* holder) { - return IntervalHolder::Make(suppress_errors, holder); +Result> IntervalDaysHolder::Make( + int32_t suppress_errors) { + return IntervalHolder::Make(suppress_errors); } -Status IntervalYearsHolder::Make(const FunctionNode& node, - std::shared_ptr* holder) { +Result> IntervalYearsHolder::Make( + const FunctionNode& node) { const std::string function_name("castINTERVALYEAR"); - return IntervalHolder::Make(node, holder, function_name); + return IntervalHolder::Make(node, function_name); } -Status IntervalYearsHolder::Make(int32_t suppress_errors, - std::shared_ptr* holder) { - return IntervalHolder::Make(suppress_errors, holder); +Result> IntervalYearsHolder::Make( + int32_t suppress_errors) { + return IntervalHolder::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 38d8e9f86a9..0a6a9880254 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* holder, - const std::string& function_name) { + static Result> 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(literal_suppress_errors->holder()); } - return Make(suppress_errors, holder); + return Make(suppress_errors); } - static Status Make(int32_t suppress_errors, std::shared_ptr* holder) { - auto lholder = std::make_shared(suppress_errors); - - *holder = lholder; - return Status::OK(); + static Result> Make(int32_t suppress_errors) { + return std::make_shared(suppress_errors); } explicit IntervalHolder(int32_t supress_errors) : suppress_errors_(supress_errors) {} @@ -94,11 +91,9 @@ class GANDIVA_EXPORT IntervalDaysHolder : public IntervalHolder* holder); + static Result> Make(const FunctionNode& node); - static Status Make(int32_t suppress_errors, - std::shared_ptr* holder); + static Result> 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* holder); + static Result> Make(const FunctionNode& node); - static Status Make(int32_t suppress_errors, - std::shared_ptr* holder); + static Result> 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 fbfd6335f45..59a10cace8c 100644 --- a/cpp/src/gandiva/interval_holder_test.cc +++ b/cpp/src/gandiva/interval_holder_test.cc @@ -20,8 +20,8 @@ #include #include -#include +#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 interval_days_holder; - std::shared_ptr 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 interval_days_holder; - std::shared_ptr 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 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 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 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 3d395741d70..8f80c5826d9 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* holder) { +Result> 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(new RandomGeneratorHolder()); - return Status::OK(); + return std::shared_ptr(new RandomGeneratorHolder()); } auto literal = dynamic_cast(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(new RandomGeneratorHolder( + return std::shared_ptr(new RandomGeneratorHolder( literal->is_null() ? 0 : std::get(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 65b6607e878..ffab725aa7f 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* holder); + static Result> 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 4b16c1b7d0d..77b2750f2e9 100644 --- a/cpp/src/gandiva/random_generator_holder_test.cc +++ b/cpp/src/gandiva/random_generator_holder_test.cc @@ -21,38 +21,35 @@ #include +#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(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 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 rand_gen_holder_1; - std::shared_ptr 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 rand_gen_holder_1; - std::shared_ptr rand_gen_holder_2; - std::shared_ptr 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 rand_gen_holder_1; - std::shared_ptr 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 30d68cbc87a..03a4af90d89 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 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* holder) { +Result> 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* h if (node.descriptor()->name() == "ilike") { regex_op.set_case_sensitive(false); // set case-insensitive for ilike function. - return Make(std::get(literal->holder()), holder, regex_op); + return Make(std::get(literal->holder()), regex_op); } if (node.children().size() == 2) { - return Make(std::get(literal->holder()), holder); + return Make(std::get(literal->holder())); } else { auto escape_char = dynamic_cast(node.children().at(2).get()); ARROW_RETURN_IF( @@ -118,12 +118,11 @@ Status LikeHolder::Make(const FunctionNode& node, std::shared_ptr* h Status::Invalid( "'like' function requires a string literal as the third parameter")); return Make(std::get(literal->holder()), - std::get(escape_char->holder()), holder); + std::get(escape_char->holder())); } } -Status LikeHolder::Make(const std::string& sql_pattern, - std::shared_ptr* holder) { +Result> 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* holder) { +Result> 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* holder, RE2::Options regex_op) { +Result> 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 lholder; - lholder = std::shared_ptr(new LikeHolder(pcre_pattern, regex_op)); + auto lholder = std::shared_ptr(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* holder) { +Result> 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(literal->holder()), holder); + return Make(std::get(literal->holder())); } -Status ReplaceHolder::Make(const std::string& sql_pattern, - std::shared_ptr* holder) { +Result> ReplaceHolder::Make( + const std::string& sql_pattern) { auto lholder = std::shared_ptr(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* holder) { +Result> 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(literal->holder()), holder); + return ExtractHolder::Make(std::get(literal->holder())); } -Status ExtractHolder::Make(const std::string& sql_pattern, - std::shared_ptr* holder) { +Result> ExtractHolder::Make( + const std::string& sql_pattern) { auto lholder = std::shared_ptr(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 ecf4095f3d4..36d942510bb 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* holder); + static Result> Make(const FunctionNode& node); - static Status Make(const std::string& sql_pattern, std::shared_ptr* holder); + static Result> Make(const std::string& sql_pattern); - static Status Make(const std::string& sql_pattern, const std::string& escape_char, - std::shared_ptr* holder); + static Result> Make(const std::string& sql_pattern, + const std::string& escape_char); - static Status Make(const std::string& sql_pattern, std::shared_ptr* holder, - RE2::Options regex_op); + static Result> 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* holder); + static Result> Make(const FunctionNode& node); - static Status Make(const std::string& sql_pattern, - std::shared_ptr* holder); + static Result> 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* holder); + static Result> Make(const FunctionNode& node); - static Status Make(const std::string& sql_pattern, - std::shared_ptr* holder); + static Result> 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 930f3a7ade7..534be5987a2 100644 --- a/cpp/src/gandiva/regex_functions_holder_test.cc +++ b/cpp/src/gandiva/regex_functions_holder_test.cc @@ -19,7 +19,8 @@ #include #include #include -#include + +#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(arrow::field("in", arrow::utf8())); auto pattern_node = std::make_shared(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(arrow::utf8(), LiteralHolder(pattern), false); auto escape_char_node = std::make_shared( 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 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 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 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 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 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 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 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 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 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 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(arrow::field("in", arrow::utf8())); auto pattern_node = std::make_shared(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 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 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 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 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 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 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 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 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 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 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 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 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 extract_holder; - // Create function with incorrect number of params auto field = std::make_shared(arrow::field("in", arrow::utf8())); auto pattern_node = std::make_shared( @@ -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 27a16d17799..0778c39b4c2 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* holder) { +Result> 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(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* holder) { +Result> ToDateHolder::Make(const std::string& sql_pattern, + int32_t suppress_errors) { std::shared_ptr transformed_pattern; ARROW_RETURN_NOT_OK(DateUtils::ToInternalFormat(sql_pattern, &transformed_pattern)); - auto lholder = std::shared_ptr( - new ToDateHolder(*(transformed_pattern.get()), suppress_errors)); - *holder = lholder; - return Status::OK(); + return std::shared_ptr( + 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 1211b6a3043..ac13f7a31b3 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* holder); + static Result> Make(const FunctionNode& node); - static Status Make(const std::string& sql_pattern, int32_t suppress_errors, - std::shared_ptr* holder); + static Result> 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 99036817d8e..1e0f2e1578f 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 -#include #include "arrow/testing/gtest_util.h" @@ -45,8 +44,7 @@ class TestToDateHolder : public ::testing::Test { }; TEST_F(TestToDateHolder, TestSimpleDateTime) { - std::shared_ptr 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 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 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 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 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 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;