diff --git a/src/iceberg/exception.h b/src/iceberg/exception.h index c5160bc43..bbb9c2283 100644 --- a/src/iceberg/exception.h +++ b/src/iceberg/exception.h @@ -38,6 +38,12 @@ class ICEBERG_EXPORT IcebergError : public std::runtime_error { explicit IcebergError(const std::string& what) : std::runtime_error(what) {} }; +/// \brief Exception thrown when expression construction fails. +class ICEBERG_EXPORT ExpressionError : public IcebergError { + public: + explicit ExpressionError(const std::string& what) : IcebergError(what) {} +}; + #define ICEBERG_CHECK(condition, ...) \ do { \ if (!(condition)) [[unlikely]] { \ diff --git a/src/iceberg/expression/expression.cc b/src/iceberg/expression/expression.cc index 070d5fe5f..69446a822 100644 --- a/src/iceberg/expression/expression.cc +++ b/src/iceberg/expression/expression.cc @@ -45,8 +45,18 @@ const std::shared_ptr& False::Instance() { Result> False::Negate() const { return True::Instance(); } // And implementation +Result> And::Make(std::shared_ptr left, + std::shared_ptr right) { + if (!left || !right) [[unlikely]] { + return InvalidExpression("And expression cannot have null children"); + } + return std::unique_ptr(new And(std::move(left), std::move(right))); +} + And::And(std::shared_ptr left, std::shared_ptr right) - : left_(std::move(left)), right_(std::move(right)) {} + : left_(std::move(left)), right_(std::move(right)) { + ICEBERG_DCHECK(left_ && right_, "And cannot have null children"); +} std::string And::ToString() const { return std::format("({} and {})", left_->ToString(), right_->ToString()); @@ -56,7 +66,7 @@ Result> And::Negate() const { // De Morgan's law: not(A and B) = (not A) or (not B) ICEBERG_ASSIGN_OR_RAISE(auto left_negated, left_->Negate()); ICEBERG_ASSIGN_OR_RAISE(auto right_negated, right_->Negate()); - return std::make_shared(std::move(left_negated), std::move(right_negated)); + return Or::Make(std::move(left_negated), std::move(right_negated)); } bool And::Equals(const Expression& expr) const { @@ -69,8 +79,18 @@ bool And::Equals(const Expression& expr) const { } // Or implementation +Result> Or::Make(std::shared_ptr left, + std::shared_ptr right) { + if (!left || !right) [[unlikely]] { + return InvalidExpression("Or cannot have null children"); + } + return std::unique_ptr(new Or(std::move(left), std::move(right))); +} + Or::Or(std::shared_ptr left, std::shared_ptr right) - : left_(std::move(left)), right_(std::move(right)) {} + : left_(std::move(left)), right_(std::move(right)) { + ICEBERG_DCHECK(left_ && right_, "Or cannot have null children"); +} std::string Or::ToString() const { return std::format("({} or {})", left_->ToString(), right_->ToString()); @@ -80,7 +100,7 @@ Result> Or::Negate() const { // De Morgan's law: not(A or B) = (not A) and (not B) ICEBERG_ASSIGN_OR_RAISE(auto left_negated, left_->Negate()); ICEBERG_ASSIGN_OR_RAISE(auto right_negated, right_->Negate()); - return std::make_shared(std::move(left_negated), std::move(right_negated)); + return And::Make(std::move(left_negated), std::move(right_negated)); } bool Or::Equals(const Expression& expr) const { @@ -93,7 +113,16 @@ bool Or::Equals(const Expression& expr) const { } // Not implementation -Not::Not(std::shared_ptr child) : child_(std::move(child)) {} +Result> Not::Make(std::shared_ptr child) { + if (!child) [[unlikely]] { + return InvalidExpression("Not expression cannot have null child"); + } + return std::unique_ptr(new Not(std::move(child))); +} + +Not::Not(std::shared_ptr child) : child_(std::move(child)) { + ICEBERG_DCHECK(child_ != nullptr, "Not expression cannot have null child"); +} std::string Not::ToString() const { return std::format("not({})", child_->ToString()); } @@ -199,7 +228,7 @@ Result Negate(Expression::Operation op) { case Expression::Operation::kMax: case Expression::Operation::kMin: case Expression::Operation::kCount: - return InvalidArgument("No negation for operation: {}", op); + return InvalidExpression("No negation for operation: {}", op); } std::unreachable(); } diff --git a/src/iceberg/expression/expression.h b/src/iceberg/expression/expression.h index 9a8052209..931e35c84 100644 --- a/src/iceberg/expression/expression.h +++ b/src/iceberg/expression/expression.h @@ -130,11 +130,12 @@ class ICEBERG_EXPORT False : public Expression { /// evaluate to true. class ICEBERG_EXPORT And : public Expression { public: - /// \brief Constructs an And expression from two sub-expressions. + /// \brief Creates an And expression from two sub-expressions. /// /// \param left The left operand of the AND expression /// \param right The right operand of the AND expression - And(std::shared_ptr left, std::shared_ptr right); + static Result> Make(std::shared_ptr left, + std::shared_ptr right); /// \brief Returns the left operand of the AND expression. /// @@ -155,6 +156,8 @@ class ICEBERG_EXPORT And : public Expression { bool Equals(const Expression& other) const override; private: + And(std::shared_ptr left, std::shared_ptr right); + std::shared_ptr left_; std::shared_ptr right_; }; @@ -165,11 +168,12 @@ class ICEBERG_EXPORT And : public Expression { /// evaluates to true. class ICEBERG_EXPORT Or : public Expression { public: - /// \brief Constructs an Or expression from two sub-expressions. + /// \brief Creates an Or expression from two sub-expressions. /// /// \param left The left operand of the OR expression /// \param right The right operand of the OR expression - Or(std::shared_ptr left, std::shared_ptr right); + static Result> Make(std::shared_ptr left, + std::shared_ptr right); /// \brief Returns the left operand of the OR expression. /// @@ -190,6 +194,8 @@ class ICEBERG_EXPORT Or : public Expression { bool Equals(const Expression& other) const override; private: + Or(std::shared_ptr left, std::shared_ptr right); + std::shared_ptr left_; std::shared_ptr right_; }; @@ -199,10 +205,11 @@ class ICEBERG_EXPORT Or : public Expression { /// This expression negates its child expression. class ICEBERG_EXPORT Not : public Expression { public: - /// \brief Constructs a Not expression from a child expression. + /// \brief Creates a Not expression from a child expression. /// /// \param child The expression to negate - explicit Not(std::shared_ptr child); + /// \return A Result containing a unique pointer to Not, or an error if child is nullptr + static Result> Make(std::shared_ptr child); /// \brief Returns the child expression. /// @@ -218,6 +225,8 @@ class ICEBERG_EXPORT Not : public Expression { bool Equals(const Expression& other) const override; private: + explicit Not(std::shared_ptr child); + std::shared_ptr child_; }; diff --git a/src/iceberg/expression/expressions.cc b/src/iceberg/expression/expressions.cc index 686a55de5..6839157f9 100644 --- a/src/iceberg/expression/expressions.cc +++ b/src/iceberg/expression/expressions.cc @@ -22,6 +22,7 @@ #include "iceberg/exception.h" #include "iceberg/transform.h" #include "iceberg/type.h" +#include "iceberg/util/macros.h" namespace iceberg { @@ -41,41 +42,57 @@ std::shared_ptr Expressions::Not(std::shared_ptr child) return not_expr.child(); } - return std::make_shared<::iceberg::Not>(std::move(child)); + ICEBERG_ASSIGN_OR_THROW(auto not_expr, iceberg::Not::Make(std::move(child))); + return not_expr; } // Transform functions std::shared_ptr Expressions::Bucket(std::string name, int32_t num_buckets) { - return std::make_shared(Ref(std::move(name)), - Transform::Bucket(num_buckets)); + ICEBERG_ASSIGN_OR_THROW( + auto transform, + UnboundTransform::Make(Ref(std::move(name)), Transform::Bucket(num_buckets))); + return transform; } std::shared_ptr Expressions::Year(std::string name) { - return std::make_shared(Ref(std::move(name)), Transform::Year()); + ICEBERG_ASSIGN_OR_THROW( + auto transform, UnboundTransform::Make(Ref(std::move(name)), Transform::Year())); + return transform; } std::shared_ptr Expressions::Month(std::string name) { - return std::make_shared(Ref(std::move(name)), Transform::Month()); + ICEBERG_ASSIGN_OR_THROW( + auto transform, UnboundTransform::Make(Ref(std::move(name)), Transform::Month())); + return transform; } std::shared_ptr Expressions::Day(std::string name) { - return std::make_shared(Ref(std::move(name)), Transform::Day()); + ICEBERG_ASSIGN_OR_THROW(auto transform, + UnboundTransform::Make(Ref(std::move(name)), Transform::Day())); + return transform; } std::shared_ptr Expressions::Hour(std::string name) { - return std::make_shared(Ref(std::move(name)), Transform::Hour()); + ICEBERG_ASSIGN_OR_THROW( + auto transform, UnboundTransform::Make(Ref(std::move(name)), Transform::Hour())); + return transform; } std::shared_ptr Expressions::Truncate(std::string name, int32_t width) { - return std::make_shared(Ref(std::move(name)), - Transform::Truncate(width)); + ICEBERG_ASSIGN_OR_THROW( + auto transform, + UnboundTransform::Make(Ref(std::move(name)), Transform::Truncate(width))); + return transform; } std::shared_ptr Expressions::Transform( std::string name, std::shared_ptr<::iceberg::Transform> transform) { - return std::make_shared(Ref(std::move(name)), std::move(transform)); + ICEBERG_ASSIGN_OR_THROW( + auto unbound_transform, + UnboundTransform::Make(Ref(std::move(name)), std::move(transform))); + return unbound_transform; } // Template implementations for unary predicates @@ -327,11 +344,12 @@ std::shared_ptr Expressions::AlwaysFalse() { return False::Instance(); } // Utilities std::shared_ptr Expressions::Ref(std::string name) { - return std::make_shared(std::move(name)); + ICEBERG_ASSIGN_OR_THROW(auto ref, NamedReference::Make(std::move(name))); + return ref; } Literal Expressions::Lit(Literal::Value value, std::shared_ptr type) { - throw IcebergError("Literal creation is not implemented"); + throw ExpressionError("Literal creation is not implemented"); } } // namespace iceberg diff --git a/src/iceberg/expression/expressions.h b/src/iceberg/expression/expressions.h index 66083da06..13895bd01 100644 --- a/src/iceberg/expression/expressions.h +++ b/src/iceberg/expression/expressions.h @@ -27,14 +27,18 @@ #include #include +#include "iceberg/exception.h" #include "iceberg/expression/literal.h" #include "iceberg/expression/predicate.h" #include "iceberg/expression/term.h" #include "iceberg/iceberg_export.h" +#include "iceberg/util/macros.h" namespace iceberg { -/// \brief Factory methods for creating expressions. +/// \brief Fluent APIs to create expressions. +/// +/// \throw `ExpressionError` for invalid expression. class ICEBERG_EXPORT Expressions { public: // Logical operations @@ -60,7 +64,9 @@ class ICEBERG_EXPORT Expressions { return left; } - return std::make_shared<::iceberg::And>(std::move(left), std::move(right)); + ICEBERG_ASSIGN_OR_THROW(auto and_expr, + iceberg::And::Make(std::move(left), std::move(right))); + return and_expr; } else { return And(And(std::move(left), std::move(right)), std::forward(args)...); } @@ -86,7 +92,9 @@ class ICEBERG_EXPORT Expressions { return left; } - return std::make_shared<::iceberg::Or>(std::move(left), std::move(right)); + ICEBERG_ASSIGN_OR_THROW(auto or_expr, + iceberg::Or::Make(std::move(left), std::move(right))); + return or_expr; } else { return Or(Or(std::move(left), std::move(right)), std::forward(args)...); } diff --git a/src/iceberg/expression/term.cc b/src/iceberg/expression/term.cc index 30bdf8ed6..ba6e55ea3 100644 --- a/src/iceberg/expression/term.cc +++ b/src/iceberg/expression/term.cc @@ -42,8 +42,17 @@ Result> Unbound::Bind(const Schema& schema) const { } // NamedReference implementation +Result> NamedReference::Make(std::string field_name) { + if (field_name.empty()) [[unlikely]] { + return InvalidExpression("NamedReference cannot have empty field name"); + } + return std::unique_ptr(new NamedReference(std::move(field_name))); +} + NamedReference::NamedReference(std::string field_name) - : field_name_(std::move(field_name)) {} + : field_name_(std::move(field_name)) { + ICEBERG_DCHECK(!field_name_.empty(), "NamedReference cannot have empty field name"); +} NamedReference::~NamedReference() = default; @@ -51,11 +60,11 @@ Result> NamedReference::Bind(const Schema& schem bool case_sensitive) const { ICEBERG_ASSIGN_OR_RAISE(auto field_opt, schema.GetFieldByName(field_name_, case_sensitive)); - if (!field_opt.has_value()) { + if (!field_opt.has_value()) [[unlikely]] { return InvalidExpression("Cannot find field '{}' in struct: {}", field_name_, schema.ToString()); } - return std::make_shared(field_opt.value().get()); + return BoundReference::Make(field_opt.value().get()); } std::string NamedReference::ToString() const { @@ -63,7 +72,18 @@ std::string NamedReference::ToString() const { } // BoundReference implementation -BoundReference::BoundReference(SchemaField field) : field_(std::move(field)) {} +Result> BoundReference::Make(SchemaField field) { + if (auto status = field.Validate(); !status.has_value()) [[unlikely]] { + return InvalidExpression("Cannot create BoundReference with invalid field: {}", + status.error().message); + } + return std::unique_ptr(new BoundReference(std::move(field))); +} + +BoundReference::BoundReference(SchemaField field) : field_(std::move(field)) { + ICEBERG_DCHECK(field_.Validate().has_value(), + "Cannot create BoundReference with invalid field"); +} BoundReference::~BoundReference() = default; @@ -87,9 +107,22 @@ bool BoundReference::Equals(const BoundTerm& other) const { } // UnboundTransform implementation +Result> UnboundTransform::Make( + std::shared_ptr ref, std::shared_ptr transform) { + if (!ref || !transform) [[unlikely]] { + return InvalidExpression( + "Cannot create UnboundTransform with null reference or transform"); + } + return std::unique_ptr( + new UnboundTransform(std::move(ref), std::move(transform))); +} + UnboundTransform::UnboundTransform(std::shared_ptr ref, std::shared_ptr transform) - : ref_(std::move(ref)), transform_(std::move(transform)) {} + : ref_(std::move(ref)), transform_(std::move(transform)) { + ICEBERG_DCHECK(!ref || !transform, + "Cannot create UnboundTransform with null reference or transform"); +} UnboundTransform::~UnboundTransform() = default; @@ -101,17 +134,31 @@ Result> UnboundTransform::Bind( const Schema& schema, bool case_sensitive) const { ICEBERG_ASSIGN_OR_RAISE(auto bound_ref, ref_->Bind(schema, case_sensitive)); ICEBERG_ASSIGN_OR_RAISE(auto transform_func, transform_->Bind(bound_ref->type())); - return std::make_shared(std::move(bound_ref), transform_, - std::move(transform_func)); + return BoundTransform::Make(std::move(bound_ref), transform_, + std::move(transform_func)); } // BoundTransform implementation +Result> BoundTransform::Make( + std::shared_ptr ref, std::shared_ptr transform, + std::shared_ptr transform_func) { + if (!ref || !transform || !transform_func) [[unlikely]] { + return InvalidExpression( + "Cannot create BoundTransform with null reference or transform"); + } + return std::unique_ptr(new BoundTransform( + std::move(ref), std::move(transform), std::move(transform_func))); +} + BoundTransform::BoundTransform(std::shared_ptr ref, std::shared_ptr transform, std::shared_ptr transform_func) : ref_(std::move(ref)), transform_(std::move(transform)), - transform_func_(std::move(transform_func)) {} + transform_func_(std::move(transform_func)) { + ICEBERG_DCHECK(ref_ && transform_ && transform_func_, + "Cannot create BoundTransform with null reference or transform"); +} BoundTransform::~BoundTransform() = default; diff --git a/src/iceberg/expression/term.h b/src/iceberg/expression/term.h index e0a883c13..6259b826e 100644 --- a/src/iceberg/expression/term.h +++ b/src/iceberg/expression/term.h @@ -32,11 +32,6 @@ namespace iceberg { -// TODO(gangwu): add a struct-like interface to wrap a row of data from ArrowArray or -// structs like ManifestFile and ManifestEntry to facilitate generailization of the -// evaluation of expressions on top of different data structures. -class StructLike; - /// \brief A term is an expression node that produces a typed value when evaluated. class ICEBERG_EXPORT Term : public util::Formattable { public: @@ -138,7 +133,7 @@ class ICEBERG_EXPORT NamedReference /// \brief Create a named reference to a field. /// /// \param field_name The name of the field to reference - explicit NamedReference(std::string field_name); + static Result> Make(std::string field_name); ~NamedReference() override; @@ -154,6 +149,8 @@ class ICEBERG_EXPORT NamedReference Kind kind() const override { return Kind::kReference; } private: + explicit NamedReference(std::string field_name); + std::string field_name_; }; @@ -166,7 +163,7 @@ class ICEBERG_EXPORT BoundReference /// \brief Create a bound reference. /// /// \param field The schema field - explicit BoundReference(SchemaField field); + static Result> Make(SchemaField field); ~BoundReference() override; @@ -189,6 +186,8 @@ class ICEBERG_EXPORT BoundReference Kind kind() const override { return Kind::kReference; } private: + explicit BoundReference(SchemaField field); + SchemaField field_; }; @@ -199,8 +198,8 @@ class ICEBERG_EXPORT UnboundTransform : public UnboundTerm /// /// \param ref The term to apply the transformation to /// \param transform The transformation function to apply - UnboundTransform(std::shared_ptr ref, - std::shared_ptr transform); + static Result> Make( + std::shared_ptr ref, std::shared_ptr transform); ~UnboundTransform() override; @@ -216,6 +215,9 @@ class ICEBERG_EXPORT UnboundTransform : public UnboundTerm Kind kind() const override { return Kind::kTransform; } private: + UnboundTransform(std::shared_ptr ref, + std::shared_ptr transform); + std::shared_ptr ref_; std::shared_ptr transform_; }; @@ -228,9 +230,9 @@ class ICEBERG_EXPORT BoundTransform : public BoundTerm { /// \param ref The bound term to apply the transformation to /// \param transform The transform to apply /// \param transform_func The bound transform function to apply - BoundTransform(std::shared_ptr ref, - std::shared_ptr transform, - std::shared_ptr transform_func); + static Result> Make( + std::shared_ptr ref, std::shared_ptr transform, + std::shared_ptr transform_func); ~BoundTransform() override; @@ -251,6 +253,10 @@ class ICEBERG_EXPORT BoundTransform : public BoundTerm { Kind kind() const override { return Kind::kTransform; } private: + BoundTransform(std::shared_ptr ref, + std::shared_ptr transform, + std::shared_ptr transform_func); + std::shared_ptr ref_; std::shared_ptr transform_; std::shared_ptr transform_func_; diff --git a/src/iceberg/schema_field.cc b/src/iceberg/schema_field.cc index 04dc35bc6..04b55a025 100644 --- a/src/iceberg/schema_field.cc +++ b/src/iceberg/schema_field.cc @@ -54,6 +54,16 @@ bool SchemaField::optional() const { return optional_; } std::string_view SchemaField::doc() const { return doc_; } +Status SchemaField::Validate() const { + if (name_.empty()) [[unlikely]] { + return InvalidSchema("SchemaField cannot have empty name"); + } + if (type_ == nullptr) [[unlikely]] { + return InvalidSchema("SchemaField cannot have null type"); + } + return {}; +} + std::string SchemaField::ToString() const { std::string result = std::format("{} ({}): {} ({}){}", name_, field_id_, *type_, optional_ ? "optional" : "required", diff --git a/src/iceberg/schema_field.h b/src/iceberg/schema_field.h index e947f2036..4190dba7d 100644 --- a/src/iceberg/schema_field.h +++ b/src/iceberg/schema_field.h @@ -29,6 +29,7 @@ #include #include "iceberg/iceberg_export.h" +#include "iceberg/result.h" #include "iceberg/type_fwd.h" #include "iceberg/util/formattable.h" @@ -72,6 +73,8 @@ class ICEBERG_EXPORT SchemaField : public iceberg::util::Formattable { [[nodiscard]] std::string ToString() const override; + Status Validate() const; + friend bool operator==(const SchemaField& lhs, const SchemaField& rhs) { return lhs.Equals(rhs); } diff --git a/src/iceberg/test/expression_test.cc b/src/iceberg/test/expression_test.cc index 326dadcb1..97e4272ff 100644 --- a/src/iceberg/test/expression_test.cc +++ b/src/iceberg/test/expression_test.cc @@ -55,12 +55,15 @@ TEST(ANDTest, Basic) { auto true_expr2 = True::Instance(); // Create an AND expression - auto and_expr = std::make_shared(true_expr1, true_expr2); + auto and_expr_result = And::Make(true_expr1, true_expr2); + ASSERT_THAT(and_expr_result, IsOk()); + auto and_expr = std::shared_ptr(std::move(and_expr_result.value())); EXPECT_EQ(and_expr->op(), Expression::Operation::kAnd); EXPECT_EQ(and_expr->ToString(), "(true and true)"); - EXPECT_EQ(and_expr->left()->op(), Expression::Operation::kTrue); - EXPECT_EQ(and_expr->right()->op(), Expression::Operation::kTrue); + auto& and_ref = static_cast(*and_expr); + EXPECT_EQ(and_ref.left()->op(), Expression::Operation::kTrue); + EXPECT_EQ(and_ref.right()->op(), Expression::Operation::kTrue); } TEST(ORTest, Basic) { @@ -69,12 +72,15 @@ TEST(ORTest, Basic) { auto false_expr = False::Instance(); // Create an OR expression - auto or_expr = std::make_shared(true_expr, false_expr); + auto or_expr_result = Or::Make(true_expr, false_expr); + ASSERT_THAT(or_expr_result, IsOk()); + auto or_expr = std::shared_ptr(std::move(or_expr_result.value())); EXPECT_EQ(or_expr->op(), Expression::Operation::kOr); EXPECT_EQ(or_expr->ToString(), "(true or false)"); - EXPECT_EQ(or_expr->left()->op(), Expression::Operation::kTrue); - EXPECT_EQ(or_expr->right()->op(), Expression::Operation::kFalse); + auto& or_ref = static_cast(*or_expr); + EXPECT_EQ(or_ref.left()->op(), Expression::Operation::kTrue); + EXPECT_EQ(or_ref.right()->op(), Expression::Operation::kFalse); } TEST(ORTest, Negation) { @@ -82,7 +88,9 @@ TEST(ORTest, Negation) { auto true_expr = True::Instance(); auto false_expr = False::Instance(); - auto or_expr = std::make_shared(true_expr, false_expr); + auto or_expr_result = Or::Make(true_expr, false_expr); + ASSERT_THAT(or_expr_result, IsOk()); + auto or_expr = std::shared_ptr(std::move(or_expr_result.value())); auto negated_or_result = or_expr->Negate(); ASSERT_THAT(negated_or_result, IsOk()); auto negated_or = negated_or_result.value(); @@ -97,20 +105,31 @@ TEST(ORTest, Equals) { auto false_expr = False::Instance(); // Test basic equality - auto or_expr1 = std::make_shared(true_expr, false_expr); - auto or_expr2 = std::make_shared(true_expr, false_expr); + auto or_expr1_result = Or::Make(true_expr, false_expr); + ASSERT_THAT(or_expr1_result, IsOk()); + auto or_expr1 = std::shared_ptr(std::move(or_expr1_result.value())); + + auto or_expr2_result = Or::Make(true_expr, false_expr); + ASSERT_THAT(or_expr2_result, IsOk()); + auto or_expr2 = std::shared_ptr(std::move(or_expr2_result.value())); EXPECT_TRUE(or_expr1->Equals(*or_expr2)); // Test commutativity: (A or B) equals (B or A) - auto or_expr3 = std::make_shared(false_expr, true_expr); + auto or_expr3_result = Or::Make(false_expr, true_expr); + ASSERT_THAT(or_expr3_result, IsOk()); + auto or_expr3 = std::shared_ptr(std::move(or_expr3_result.value())); EXPECT_TRUE(or_expr1->Equals(*or_expr3)); // Test inequality with different expressions - auto or_expr4 = std::make_shared(true_expr, true_expr); + auto or_expr4_result = Or::Make(true_expr, true_expr); + ASSERT_THAT(or_expr4_result, IsOk()); + auto or_expr4 = std::shared_ptr(std::move(or_expr4_result.value())); EXPECT_FALSE(or_expr1->Equals(*or_expr4)); // Test inequality with different operation types - auto and_expr = std::make_shared(true_expr, false_expr); + auto and_expr_result = And::Make(true_expr, false_expr); + ASSERT_THAT(and_expr_result, IsOk()); + auto and_expr = std::shared_ptr(std::move(and_expr_result.value())); EXPECT_FALSE(or_expr1->Equals(*and_expr)); } @@ -119,7 +138,9 @@ TEST(ANDTest, Negation) { auto true_expr = True::Instance(); auto false_expr = False::Instance(); - auto and_expr = std::make_shared(true_expr, false_expr); + auto and_expr_result = And::Make(true_expr, false_expr); + ASSERT_THAT(and_expr_result, IsOk()); + auto and_expr = std::shared_ptr(std::move(and_expr_result.value())); auto negated_and_result = and_expr->Negate(); ASSERT_THAT(negated_and_result, IsOk()); auto negated_and = negated_and_result.value(); @@ -134,20 +155,31 @@ TEST(ANDTest, Equals) { auto false_expr = False::Instance(); // Test basic equality - auto and_expr1 = std::make_shared(true_expr, false_expr); - auto and_expr2 = std::make_shared(true_expr, false_expr); + auto and_expr1_result = And::Make(true_expr, false_expr); + ASSERT_THAT(and_expr1_result, IsOk()); + auto and_expr1 = std::shared_ptr(std::move(and_expr1_result.value())); + + auto and_expr2_result = And::Make(true_expr, false_expr); + ASSERT_THAT(and_expr2_result, IsOk()); + auto and_expr2 = std::shared_ptr(std::move(and_expr2_result.value())); EXPECT_TRUE(and_expr1->Equals(*and_expr2)); // Test commutativity: (A and B) equals (B and A) - auto and_expr3 = std::make_shared(false_expr, true_expr); + auto and_expr3_result = And::Make(false_expr, true_expr); + ASSERT_THAT(and_expr3_result, IsOk()); + auto and_expr3 = std::shared_ptr(std::move(and_expr3_result.value())); EXPECT_TRUE(and_expr1->Equals(*and_expr3)); // Test inequality with different expressions - auto and_expr4 = std::make_shared(true_expr, true_expr); + auto and_expr4_result = And::Make(true_expr, true_expr); + ASSERT_THAT(and_expr4_result, IsOk()); + auto and_expr4 = std::shared_ptr(std::move(and_expr4_result.value())); EXPECT_FALSE(and_expr1->Equals(*and_expr4)); // Test inequality with different operation types - auto or_expr = std::make_shared(true_expr, false_expr); + auto or_expr_result = Or::Make(true_expr, false_expr); + ASSERT_THAT(or_expr_result, IsOk()); + auto or_expr = std::shared_ptr(std::move(or_expr_result.value())); EXPECT_FALSE(and_expr1->Equals(*or_expr)); } @@ -168,17 +200,22 @@ TEST(ExpressionTest, BaseClassNegateErrorOut) { TEST(NotTest, Basic) { auto true_expr = True::Instance(); - auto not_expr = std::make_shared(true_expr); + auto not_expr_result = Not::Make(true_expr); + ASSERT_THAT(not_expr_result, IsOk()); + auto not_expr = std::shared_ptr(std::move(not_expr_result.value())); EXPECT_EQ(not_expr->op(), Expression::Operation::kNot); EXPECT_EQ(not_expr->ToString(), "not(true)"); - EXPECT_EQ(not_expr->child()->op(), Expression::Operation::kTrue); + auto& not_ref = static_cast(*not_expr); + EXPECT_EQ(not_ref.child()->op(), Expression::Operation::kTrue); } TEST(NotTest, Negation) { // Test that not(not(x)) = x auto true_expr = True::Instance(); - auto not_expr = std::make_shared(true_expr); + auto not_expr_result = Not::Make(true_expr); + ASSERT_THAT(not_expr_result, IsOk()); + auto not_expr = std::shared_ptr(std::move(not_expr_result.value())); auto negated_result = not_expr->Negate(); ASSERT_THAT(negated_result, IsOk()); @@ -193,16 +230,25 @@ TEST(NotTest, Equals) { auto false_expr = False::Instance(); // Test basic equality - auto not_expr1 = std::make_shared(true_expr); - auto not_expr2 = std::make_shared(true_expr); + auto not_expr1_result = Not::Make(true_expr); + ASSERT_THAT(not_expr1_result, IsOk()); + auto not_expr1 = std::shared_ptr(std::move(not_expr1_result.value())); + + auto not_expr2_result = Not::Make(true_expr); + ASSERT_THAT(not_expr2_result, IsOk()); + auto not_expr2 = std::shared_ptr(std::move(not_expr2_result.value())); EXPECT_TRUE(not_expr1->Equals(*not_expr2)); // Test inequality with different child expressions - auto not_expr3 = std::make_shared(false_expr); + auto not_expr3_result = Not::Make(false_expr); + ASSERT_THAT(not_expr3_result, IsOk()); + auto not_expr3 = std::shared_ptr(std::move(not_expr3_result.value())); EXPECT_FALSE(not_expr1->Equals(*not_expr3)); // Test inequality with different operation types - auto and_expr = std::make_shared(true_expr, false_expr); + auto and_expr_result = And::Make(true_expr, false_expr); + ASSERT_THAT(and_expr_result, IsOk()); + auto and_expr = std::shared_ptr(std::move(and_expr_result.value())); EXPECT_FALSE(not_expr1->Equals(*and_expr)); } diff --git a/src/iceberg/test/predicate_test.cc b/src/iceberg/test/predicate_test.cc index 5ab790820..532e908b4 100644 --- a/src/iceberg/test/predicate_test.cc +++ b/src/iceberg/test/predicate_test.cc @@ -218,14 +218,14 @@ TEST_F(PredicateTest, ReferenceFactory) { } TEST_F(PredicateTest, NamedReferenceBasics) { - auto ref = std::make_shared("id"); + auto ref = Expressions::Ref("id"); EXPECT_EQ(ref->name(), "id"); EXPECT_EQ(ref->ToString(), "ref(name=\"id\")"); EXPECT_EQ(ref->reference(), ref); } TEST_F(PredicateTest, NamedReferenceBind) { - auto ref = std::make_shared("id"); + auto ref = Expressions::Ref("id"); auto bound_result = ref->Bind(*schema_, /*case_sensitive=*/true); ASSERT_THAT(bound_result, IsOk()); @@ -237,15 +237,15 @@ TEST_F(PredicateTest, NamedReferenceBind) { } TEST_F(PredicateTest, NamedReferenceBindNonExistentField) { - auto ref = std::make_shared("non_existent_field"); + auto ref = Expressions::Ref("non_existent_field"); auto bound_result = ref->Bind(*schema_, /*case_sensitive=*/true); EXPECT_THAT(bound_result, IsError(ErrorKind::kInvalidExpression)); } TEST_F(PredicateTest, BoundReferenceEquality) { - auto ref1 = std::make_shared("id"); - auto ref2 = std::make_shared("id"); - auto ref3 = std::make_shared("name"); + auto ref1 = Expressions::Ref("id"); + auto ref2 = Expressions::Ref("id"); + auto ref3 = Expressions::Ref("name"); auto bound1 = ref1->Bind(*schema_, true).value(); auto bound2 = ref2->Bind(*schema_, true).value(); diff --git a/src/iceberg/util/macros.h b/src/iceberg/util/macros.h index 733b07f1a..50ac13f27 100644 --- a/src/iceberg/util/macros.h +++ b/src/iceberg/util/macros.h @@ -44,9 +44,16 @@ #define ICEBERG_DCHECK(expr, message) assert((expr) && (message)) +#define ERROR_TO_EXCEPTION(error) \ + if (error.kind == iceberg::ErrorKind::kInvalidExpression) { \ + throw iceberg::ExpressionError(error.message); \ + } else { \ + throw iceberg::IcebergError(error.message); \ + } + #define ICEBERG_THROW_NOT_OK(result) \ if (auto&& result_name = result; !result_name) [[unlikely]] { \ - throw iceberg::IcebergError(result_name.error().message); \ + ERROR_TO_EXCEPTION(result_name.error()); \ } #define ICEBERG_ASSIGN_OR_THROW_IMPL(result_name, lhs, rexpr) \