From 919b10ad2bea1719930e1b2a3656f8fad4304cb3 Mon Sep 17 00:00:00 2001 From: Li Feiyang Date: Mon, 3 Nov 2025 17:58:55 +0800 Subject: [PATCH 1/2] refactor: use factory pattern and throw exception for expression --- src/iceberg/exception.h | 6 ++ src/iceberg/expression/expression.cc | 44 ++++++++++-- src/iceberg/expression/expression.h | 25 +++++-- src/iceberg/expression/expressions.cc | 43 ++++++++---- src/iceberg/expression/expressions.h | 14 +++- src/iceberg/expression/term.cc | 61 ++++++++++++++--- src/iceberg/expression/term.h | 32 +++++++-- src/iceberg/test/expression_test.cc | 96 ++++++++++++++++++++------- src/iceberg/test/predicate_test.cc | 12 ++-- src/iceberg/util/macros.h | 11 +++ 10 files changed, 272 insertions(+), 72 deletions(-) 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..f2d8c5b9e 100644 --- a/src/iceberg/expression/expression.cc +++ b/src/iceberg/expression/expression.cc @@ -45,8 +45,19 @@ 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_ != nullptr, "And left operand cannot be null"); + ICEBERG_DCHECK(right_ != nullptr, "And right operand cannot be null"); +} std::string And::ToString() const { return std::format("({} and {})", left_->ToString(), right_->ToString()); @@ -56,7 +67,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 +80,22 @@ bool And::Equals(const Expression& expr) const { } // Or implementation +Result> Or::Make(std::shared_ptr left, + std::shared_ptr right) { + if (!left) { + return InvalidArgument("Or expression left operand cannot be null"); + } + if (!right) { + return InvalidArgument("Or expression right operand cannot be null"); + } + 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_ != nullptr, "Or left operand cannot be null"); + ICEBERG_DCHECK(right_ != nullptr, "Or right operand cannot be null"); +} std::string Or::ToString() const { return std::format("({} or {})", left_->ToString(), right_->ToString()); @@ -80,7 +105,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 +118,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) { + 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 child expression cannot be null"); +} std::string Not::ToString() const { return std::format("not({})", child_->ToString()); } diff --git a/src/iceberg/expression/expression.h b/src/iceberg/expression/expression.h index 9a8052209..cc3813f9a 100644 --- a/src/iceberg/expression/expression.h +++ b/src/iceberg/expression/expression.h @@ -130,11 +130,14 @@ 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); + /// \return A Result containing a unique pointer to And, or an error if either parameter + /// is nullptr + static Result> Make(std::shared_ptr left, + std::shared_ptr right); /// \brief Returns the left operand of the AND expression. /// @@ -155,6 +158,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 +170,14 @@ 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); + /// \return A Result containing a unique pointer to Or, or an error if either parameter + /// is nullptr + static Result> Make(std::shared_ptr left, + std::shared_ptr right); /// \brief Returns the left operand of the OR expression. /// @@ -190,6 +198,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 +209,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 +229,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..e90e2e303 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,58 @@ std::shared_ptr Expressions::Not(std::shared_ptr child) return not_expr.child(); } - return std::make_shared<::iceberg::Not>(std::move(child)); + auto result = ::iceberg::Not::Make(std::move(child)); + ICEBERG_ASSIGN_OR_THROW(auto not_expr, std::move(result)); + return {std::move(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 {std::move(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 {std::move(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 {std::move(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 {std::move(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 {std::move(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 {std::move(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 {std::move(unbound_transform)}; } // Template implementations for unary predicates @@ -327,11 +345,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 {std::move(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..ea69cac8d 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)); + auto result = ::iceberg::And::Make(std::move(left), std::move(right)); + ICEBERG_ASSIGN_OR_THROW(auto and_expr, std::move(result)); + return {std::move(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)); + auto result = ::iceberg::Or::Make(std::move(left), std::move(right)); + ICEBERG_ASSIGN_OR_THROW(auto or_expr, std::move(result)); + return {std::move(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..6521720f5 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 field name cannot be empty"); + } + 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 field name cannot be empty"); +} 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,16 @@ std::string NamedReference::ToString() const { } // BoundReference implementation -BoundReference::BoundReference(SchemaField field) : field_(std::move(field)) {} +Result> BoundReference::Make(SchemaField field) { + if (!field.type()) [[unlikely]] { + return InvalidExpression("BoundReference field type cannot be null"); + } + return std::unique_ptr(new BoundReference(std::move(field))); +} + +BoundReference::BoundReference(SchemaField field) : field_(std::move(field)) { + ICEBERG_DCHECK(field_.type() != nullptr, "BoundReference field type cannot be null"); +} BoundReference::~BoundReference() = default; @@ -87,9 +105,21 @@ 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("UnboundTransform cannot have null children"); + } + 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_ != nullptr, "UnboundTransform reference cannot be null"); + ICEBERG_DCHECK(transform_ != nullptr, "UnboundTransform transform cannot be null"); +} UnboundTransform::~UnboundTransform() = default; @@ -101,17 +131,32 @@ 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("BoundTransform cannot have null children"); + } + 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_ != nullptr, "BoundTransform reference cannot be null"); + ICEBERG_DCHECK(transform_ != nullptr, "BoundTransform transform cannot be null"); + ICEBERG_DCHECK(transform_func_ != nullptr, + "BoundTransform transform function cannot be null"); +} BoundTransform::~BoundTransform() = default; diff --git a/src/iceberg/expression/term.h b/src/iceberg/expression/term.h index e0a883c13..18d5ab13d 100644 --- a/src/iceberg/expression/term.h +++ b/src/iceberg/expression/term.h @@ -138,7 +138,9 @@ 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); + /// \return A Result containing a unique pointer to NamedReference, or an error if + /// field_name is empty + static Result> Make(std::string field_name); ~NamedReference() override; @@ -154,6 +156,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 +170,8 @@ class ICEBERG_EXPORT BoundReference /// \brief Create a bound reference. /// /// \param field The schema field - explicit BoundReference(SchemaField field); + /// \return A unique pointer to BoundReference (field cannot be null in practice) + static Result> Make(SchemaField field); ~BoundReference() override; @@ -189,6 +194,8 @@ class ICEBERG_EXPORT BoundReference Kind kind() const override { return Kind::kReference; } private: + explicit BoundReference(SchemaField field); + SchemaField field_; }; @@ -199,8 +206,10 @@ 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); + /// \return A Result containing a unique pointer to UnboundTransform, or an error if + /// parameters are null + static Result> Make( + std::shared_ptr ref, std::shared_ptr transform); ~UnboundTransform() override; @@ -216,6 +225,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 +240,11 @@ 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); + /// \return A Result containing a unique pointer to BoundTransform, or an error if + /// parameters are null + static Result> Make( + std::shared_ptr ref, std::shared_ptr transform, + std::shared_ptr transform_func); ~BoundTransform() override; @@ -251,6 +265,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/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..4d8a3514d 100644 --- a/src/iceberg/util/macros.h +++ b/src/iceberg/util/macros.h @@ -42,6 +42,17 @@ ICEBERG_ASSIGN_OR_RAISE_IMPL(ICEBERG_ASSIGN_OR_RAISE_NAME(result_, __COUNTER__), lhs, \ rexpr) +#define ICEBERG_ASSIGN_OR_THROW_IMPL(result_name, lhs, rexpr) \ + auto&& result_name = (rexpr); \ + if (!result_name) [[unlikely]] { \ + throw iceberg::ExpressionError(result_name.error().message); \ + } \ + lhs = std::move(result_name.value()); + +#define ICEBERG_ASSIGN_OR_THROW(lhs, rexpr) \ + ICEBERG_ASSIGN_OR_THROW_IMPL(ICEBERG_ASSIGN_OR_RAISE_NAME(result_, __COUNTER__), lhs, \ + rexpr) + #define ICEBERG_DCHECK(expr, message) assert((expr) && (message)) #define ICEBERG_THROW_NOT_OK(result) \ From a22dc7f59b962306c292f39db1ad1294a87bef33 Mon Sep 17 00:00:00 2001 From: Gang Wu Date: Tue, 4 Nov 2025 11:03:46 +0800 Subject: [PATCH 2/2] fix conflict --- src/iceberg/expression/expression.cc | 19 +++++++----------- src/iceberg/expression/expression.h | 4 ---- src/iceberg/expression/expressions.cc | 21 ++++++++++---------- src/iceberg/expression/expressions.h | 12 ++++++------ src/iceberg/expression/term.cc | 28 ++++++++++++++------------- src/iceberg/expression/term.h | 12 ------------ src/iceberg/schema_field.cc | 10 ++++++++++ src/iceberg/schema_field.h | 3 +++ src/iceberg/util/macros.h | 20 ++++++++----------- 9 files changed, 59 insertions(+), 70 deletions(-) diff --git a/src/iceberg/expression/expression.cc b/src/iceberg/expression/expression.cc index f2d8c5b9e..69446a822 100644 --- a/src/iceberg/expression/expression.cc +++ b/src/iceberg/expression/expression.cc @@ -55,8 +55,7 @@ Result> And::Make(std::shared_ptr left, And::And(std::shared_ptr left, std::shared_ptr right) : left_(std::move(left)), right_(std::move(right)) { - ICEBERG_DCHECK(left_ != nullptr, "And left operand cannot be null"); - ICEBERG_DCHECK(right_ != nullptr, "And right operand cannot be null"); + ICEBERG_DCHECK(left_ && right_, "And cannot have null children"); } std::string And::ToString() const { @@ -82,19 +81,15 @@ bool And::Equals(const Expression& expr) const { // Or implementation Result> Or::Make(std::shared_ptr left, std::shared_ptr right) { - if (!left) { - return InvalidArgument("Or expression left operand cannot be null"); - } - if (!right) { - return InvalidArgument("Or expression right operand cannot be null"); + 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)) { - ICEBERG_DCHECK(left_ != nullptr, "Or left operand cannot be null"); - ICEBERG_DCHECK(right_ != nullptr, "Or right operand cannot be null"); + ICEBERG_DCHECK(left_ && right_, "Or cannot have null children"); } std::string Or::ToString() const { @@ -119,14 +114,14 @@ bool Or::Equals(const Expression& expr) const { // Not implementation Result> Not::Make(std::shared_ptr child) { - if (!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 child expression cannot be null"); + ICEBERG_DCHECK(child_ != nullptr, "Not expression cannot have null child"); } std::string Not::ToString() const { return std::format("not({})", child_->ToString()); } @@ -233,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 cc3813f9a..931e35c84 100644 --- a/src/iceberg/expression/expression.h +++ b/src/iceberg/expression/expression.h @@ -134,8 +134,6 @@ class ICEBERG_EXPORT And : public Expression { /// /// \param left The left operand of the AND expression /// \param right The right operand of the AND expression - /// \return A Result containing a unique pointer to And, or an error if either parameter - /// is nullptr static Result> Make(std::shared_ptr left, std::shared_ptr right); @@ -174,8 +172,6 @@ class ICEBERG_EXPORT Or : public Expression { /// /// \param left The left operand of the OR expression /// \param right The right operand of the OR expression - /// \return A Result containing a unique pointer to Or, or an error if either parameter - /// is nullptr static Result> Make(std::shared_ptr left, std::shared_ptr right); diff --git a/src/iceberg/expression/expressions.cc b/src/iceberg/expression/expressions.cc index e90e2e303..6839157f9 100644 --- a/src/iceberg/expression/expressions.cc +++ b/src/iceberg/expression/expressions.cc @@ -42,9 +42,8 @@ std::shared_ptr Expressions::Not(std::shared_ptr child) return not_expr.child(); } - auto result = ::iceberg::Not::Make(std::move(child)); - ICEBERG_ASSIGN_OR_THROW(auto not_expr, std::move(result)); - return {std::move(not_expr)}; + ICEBERG_ASSIGN_OR_THROW(auto not_expr, iceberg::Not::Make(std::move(child))); + return not_expr; } // Transform functions @@ -54,38 +53,38 @@ std::shared_ptr Expressions::Bucket(std::string name, ICEBERG_ASSIGN_OR_THROW( auto transform, UnboundTransform::Make(Ref(std::move(name)), Transform::Bucket(num_buckets))); - return {std::move(transform)}; + return transform; } std::shared_ptr Expressions::Year(std::string name) { ICEBERG_ASSIGN_OR_THROW( auto transform, UnboundTransform::Make(Ref(std::move(name)), Transform::Year())); - return {std::move(transform)}; + return transform; } std::shared_ptr Expressions::Month(std::string name) { ICEBERG_ASSIGN_OR_THROW( auto transform, UnboundTransform::Make(Ref(std::move(name)), Transform::Month())); - return {std::move(transform)}; + return transform; } std::shared_ptr Expressions::Day(std::string name) { ICEBERG_ASSIGN_OR_THROW(auto transform, UnboundTransform::Make(Ref(std::move(name)), Transform::Day())); - return {std::move(transform)}; + return transform; } std::shared_ptr Expressions::Hour(std::string name) { ICEBERG_ASSIGN_OR_THROW( auto transform, UnboundTransform::Make(Ref(std::move(name)), Transform::Hour())); - return {std::move(transform)}; + return transform; } std::shared_ptr Expressions::Truncate(std::string name, int32_t width) { ICEBERG_ASSIGN_OR_THROW( auto transform, UnboundTransform::Make(Ref(std::move(name)), Transform::Truncate(width))); - return {std::move(transform)}; + return transform; } std::shared_ptr Expressions::Transform( @@ -93,7 +92,7 @@ std::shared_ptr Expressions::Transform( ICEBERG_ASSIGN_OR_THROW( auto unbound_transform, UnboundTransform::Make(Ref(std::move(name)), std::move(transform))); - return {std::move(unbound_transform)}; + return unbound_transform; } // Template implementations for unary predicates @@ -346,7 +345,7 @@ std::shared_ptr Expressions::AlwaysFalse() { return False::Instance(); } std::shared_ptr Expressions::Ref(std::string name) { ICEBERG_ASSIGN_OR_THROW(auto ref, NamedReference::Make(std::move(name))); - return {std::move(ref)}; + return ref; } Literal Expressions::Lit(Literal::Value value, std::shared_ptr type) { diff --git a/src/iceberg/expression/expressions.h b/src/iceberg/expression/expressions.h index ea69cac8d..13895bd01 100644 --- a/src/iceberg/expression/expressions.h +++ b/src/iceberg/expression/expressions.h @@ -64,9 +64,9 @@ class ICEBERG_EXPORT Expressions { return left; } - auto result = ::iceberg::And::Make(std::move(left), std::move(right)); - ICEBERG_ASSIGN_OR_THROW(auto and_expr, std::move(result)); - return {std::move(and_expr)}; + 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)...); } @@ -92,9 +92,9 @@ class ICEBERG_EXPORT Expressions { return left; } - auto result = ::iceberg::Or::Make(std::move(left), std::move(right)); - ICEBERG_ASSIGN_OR_THROW(auto or_expr, std::move(result)); - return {std::move(or_expr)}; + 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 6521720f5..ba6e55ea3 100644 --- a/src/iceberg/expression/term.cc +++ b/src/iceberg/expression/term.cc @@ -44,14 +44,14 @@ Result> Unbound::Bind(const Schema& schema) const { // NamedReference implementation Result> NamedReference::Make(std::string field_name) { if (field_name.empty()) [[unlikely]] { - return InvalidExpression("NamedReference field name cannot be empty"); + 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)) { - ICEBERG_DCHECK(!field_name_.empty(), "NamedReference field name cannot be empty"); + ICEBERG_DCHECK(!field_name_.empty(), "NamedReference cannot have empty field name"); } NamedReference::~NamedReference() = default; @@ -73,14 +73,16 @@ std::string NamedReference::ToString() const { // BoundReference implementation Result> BoundReference::Make(SchemaField field) { - if (!field.type()) [[unlikely]] { - return InvalidExpression("BoundReference field type cannot be null"); + 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_.type() != nullptr, "BoundReference field type cannot be null"); + ICEBERG_DCHECK(field_.Validate().has_value(), + "Cannot create BoundReference with invalid field"); } BoundReference::~BoundReference() = default; @@ -108,7 +110,8 @@ bool BoundReference::Equals(const BoundTerm& other) const { Result> UnboundTransform::Make( std::shared_ptr ref, std::shared_ptr transform) { if (!ref || !transform) [[unlikely]] { - return InvalidExpression("UnboundTransform cannot have null children"); + return InvalidExpression( + "Cannot create UnboundTransform with null reference or transform"); } return std::unique_ptr( new UnboundTransform(std::move(ref), std::move(transform))); @@ -117,8 +120,8 @@ Result> UnboundTransform::Make( UnboundTransform::UnboundTransform(std::shared_ptr ref, std::shared_ptr transform) : ref_(std::move(ref)), transform_(std::move(transform)) { - ICEBERG_DCHECK(ref_ != nullptr, "UnboundTransform reference cannot be null"); - ICEBERG_DCHECK(transform_ != nullptr, "UnboundTransform transform cannot be null"); + ICEBERG_DCHECK(!ref || !transform, + "Cannot create UnboundTransform with null reference or transform"); } UnboundTransform::~UnboundTransform() = default; @@ -140,7 +143,8 @@ Result> BoundTransform::Make( std::shared_ptr ref, std::shared_ptr transform, std::shared_ptr transform_func) { if (!ref || !transform || !transform_func) [[unlikely]] { - return InvalidExpression("BoundTransform cannot have null children"); + 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))); @@ -152,10 +156,8 @@ BoundTransform::BoundTransform(std::shared_ptr ref, : ref_(std::move(ref)), transform_(std::move(transform)), transform_func_(std::move(transform_func)) { - ICEBERG_DCHECK(ref_ != nullptr, "BoundTransform reference cannot be null"); - ICEBERG_DCHECK(transform_ != nullptr, "BoundTransform transform cannot be null"); - ICEBERG_DCHECK(transform_func_ != nullptr, - "BoundTransform transform function cannot be null"); + 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 18d5ab13d..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,8 +133,6 @@ class ICEBERG_EXPORT NamedReference /// \brief Create a named reference to a field. /// /// \param field_name The name of the field to reference - /// \return A Result containing a unique pointer to NamedReference, or an error if - /// field_name is empty static Result> Make(std::string field_name); ~NamedReference() override; @@ -170,7 +163,6 @@ class ICEBERG_EXPORT BoundReference /// \brief Create a bound reference. /// /// \param field The schema field - /// \return A unique pointer to BoundReference (field cannot be null in practice) static Result> Make(SchemaField field); ~BoundReference() override; @@ -206,8 +198,6 @@ class ICEBERG_EXPORT UnboundTransform : public UnboundTerm /// /// \param ref The term to apply the transformation to /// \param transform The transformation function to apply - /// \return A Result containing a unique pointer to UnboundTransform, or an error if - /// parameters are null static Result> Make( std::shared_ptr ref, std::shared_ptr transform); @@ -240,8 +230,6 @@ 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 - /// \return A Result containing a unique pointer to BoundTransform, or an error if - /// parameters are null static Result> Make( 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/util/macros.h b/src/iceberg/util/macros.h index 4d8a3514d..50ac13f27 100644 --- a/src/iceberg/util/macros.h +++ b/src/iceberg/util/macros.h @@ -42,22 +42,18 @@ ICEBERG_ASSIGN_OR_RAISE_IMPL(ICEBERG_ASSIGN_OR_RAISE_NAME(result_, __COUNTER__), lhs, \ rexpr) -#define ICEBERG_ASSIGN_OR_THROW_IMPL(result_name, lhs, rexpr) \ - auto&& result_name = (rexpr); \ - if (!result_name) [[unlikely]] { \ - throw iceberg::ExpressionError(result_name.error().message); \ - } \ - lhs = std::move(result_name.value()); - -#define ICEBERG_ASSIGN_OR_THROW(lhs, rexpr) \ - ICEBERG_ASSIGN_OR_THROW_IMPL(ICEBERG_ASSIGN_OR_RAISE_NAME(result_, __COUNTER__), lhs, \ - rexpr) - #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) \