Skip to content

Commit

Permalink
GH-38920: [C++][Gandiva] Refactor function holder to return arrow Res…
Browse files Browse the repository at this point in the history 
…ult (#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 #38632 (comment) 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 <niyue.com@gmail.com>
Signed-off-by: Antoine Pitrou <antoine@python.org>
  • Loading branch information
@niyue
niyue committed Nov 29, 2023
1 parent c441862 commit 6f497ec
Show file tree
Hide file tree
Showing 13 changed files with 151 additions and 297 deletions.
4 changes: 1 addition & 3 deletions cpp/src/gandiva/function_holder_maker_registry.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
24 changes: 12 additions & 12 deletions cpp/src/gandiva/interval_holder.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
25 changes: 9 additions & 16 deletions cpp/src/gandiva/interval_holder.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"));

Expand All @@ -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) {}
Expand All @@ -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,
Expand Down Expand Up @@ -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,
Expand Down
44 changes: 10 additions & 34 deletions cpp/src/gandiva/interval_holder_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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");
Expand All @@ -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");
Expand Down Expand Up @@ -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;
Expand Down
10 changes: 4 additions & 6 deletions cpp/src/gandiva/random_generator_holder.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand All @@ -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
3 changes: 1 addition & 2 deletions cpp/src/gandiva/random_generator_holder.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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_); }

Expand Down
39 changes: 13 additions & 26 deletions cpp/src/gandiva/random_generator_holder_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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;
Expand Down

0 comments on commit 6f497ec

Please sign in to comment.