refactor: use factory pattern and throw exception for expression

Author: HeartLinked

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]] {                         \

src/iceberg/expression/expression.cc

@@ -45,8 +45,22 @@ const std::shared_ptr<False>& False::Instance() {
 Result<std::shared_ptr<Expression>> False::Negate() const { return True::Instance(); }
 
 // And implementation
+Result<std::unique_ptr<And>> And::Make(std::shared_ptr<Expression> left,
+                                       std::shared_ptr<Expression> right) {
+  if (!left) {
+    return InvalidArgument("And expression left operand cannot be null");
+  }
+  if (!right) {
+    return InvalidArgument("And expression right operand cannot be null");
+  }
+  return std::unique_ptr<And>(new And(std::move(left), std::move(right)));
+}
+
 And::And(std::shared_ptr<Expression> left, std::shared_ptr<Expression> 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 +70,9 @@ Result<std::shared_ptr<Expression>> 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<Or>(std::move(left_negated), std::move(right_negated));
+  ICEBERG_ASSIGN_OR_RAISE(auto or_expr,
+                          Or::Make(std::move(left_negated), std::move(right_negated)));
+  return std::shared_ptr<Expression>(std::move(or_expr));
 }
 
 bool And::Equals(const Expression& expr) const {
@@ -69,8 +85,22 @@ bool And::Equals(const Expression& expr) const {
 }
 
 // Or implementation
+Result<std::unique_ptr<Or>> Or::Make(std::shared_ptr<Expression> left,
+                                     std::shared_ptr<Expression> 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<Or>(new Or(std::move(left), std::move(right)));
+}
+
 Or::Or(std::shared_ptr<Expression> left, std::shared_ptr<Expression> 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 +110,9 @@ Result<std::shared_ptr<Expression>> 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<And>(std::move(left_negated), std::move(right_negated));
+  ICEBERG_ASSIGN_OR_RAISE(auto and_expr,
+                          And::Make(std::move(left_negated), std::move(right_negated)));
+  return std::shared_ptr<Expression>(std::move(and_expr));
 }
 
 bool Or::Equals(const Expression& expr) const {
@@ -93,7 +125,16 @@ bool Or::Equals(const Expression& expr) const {
 }
 
 // Not implementation
-Not::Not(std::shared_ptr<Expression> child) : child_(std::move(child)) {}
+Result<std::unique_ptr<Not>> Not::Make(std::shared_ptr<Expression> child) {
+  if (!child) {
+    return InvalidArgument("Not expression child cannot be null");
+  }
+  return std::unique_ptr<Not>(new Not(std::move(child)));
+}
+
+Not::Not(std::shared_ptr<Expression> 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()); }
 

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<Expression> left, std::shared_ptr<Expression> right);
+  /// \return A Result containing a unique pointer to And, or an error if either parameter
+  /// is nullptr
+  static Result<std::unique_ptr<And>> Make(std::shared_ptr<Expression> left,
+                                           std::shared_ptr<Expression> 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<Expression> left, std::shared_ptr<Expression> right);
+
   std::shared_ptr<Expression> left_;
   std::shared_ptr<Expression> 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<Expression> left, std::shared_ptr<Expression> right);
+  /// \return A Result containing a unique pointer to Or, or an error if either parameter
+  /// is nullptr
+  static Result<std::unique_ptr<Or>> Make(std::shared_ptr<Expression> left,
+                                          std::shared_ptr<Expression> 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<Expression> left, std::shared_ptr<Expression> right);
+
   std::shared_ptr<Expression> left_;
   std::shared_ptr<Expression> 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<Expression> child);
+  /// \return A Result containing a unique pointer to Not, or an error if child is nullptr
+  static Result<std::unique_ptr<Not>> Make(std::shared_ptr<Expression> 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<Expression> child);
+
   std::shared_ptr<Expression> child_;
 };
 

src/iceberg/expression/expressions.cc

@@ -41,41 +41,72 @@ std::shared_ptr<Expression> Expressions::Not(std::shared_ptr<Expression> child)
     return not_expr.child();
   }
 
-  return std::make_shared<::iceberg::Not>(std::move(child));
+  auto result = ::iceberg::Not::Make(std::move(child));
+  if (!result) {
+    throw ExpressionError(result.error().message);
+  }
+  return {std::move(result.value())};
 }
 
 // Transform functions
 
 std::shared_ptr<UnboundTransform> Expressions::Bucket(std::string name,
                                                       int32_t num_buckets) {
-  return std::make_shared<UnboundTransform>(Ref(std::move(name)),
-                                            Transform::Bucket(num_buckets));
+  auto result =
+      UnboundTransform::Make(Ref(std::move(name)), Transform::Bucket(num_buckets));
+  if (!result) {
+    throw ExpressionError(result.error().message);
+  }
+  return {std::move(result.value())};
 }
 
 std::shared_ptr<UnboundTransform> Expressions::Year(std::string name) {
-  return std::make_shared<UnboundTransform>(Ref(std::move(name)), Transform::Year());
+  auto result = UnboundTransform::Make(Ref(std::move(name)), Transform::Year());
+  if (!result) {
+    throw ExpressionError(result.error().message);
+  }
+  return {std::move(result.value())};
 }
 
 std::shared_ptr<UnboundTransform> Expressions::Month(std::string name) {
-  return std::make_shared<UnboundTransform>(Ref(std::move(name)), Transform::Month());
+  auto result = UnboundTransform::Make(Ref(std::move(name)), Transform::Month());
+  if (!result) {
+    throw ExpressionError(result.error().message);
+  }
+  return {std::move(result.value())};
 }
 
 std::shared_ptr<UnboundTransform> Expressions::Day(std::string name) {
-  return std::make_shared<UnboundTransform>(Ref(std::move(name)), Transform::Day());
+  auto result = UnboundTransform::Make(Ref(std::move(name)), Transform::Day());
+  if (!result) {
+    throw ExpressionError(result.error().message);
+  }
+  return {std::move(result.value())};
 }
 
 std::shared_ptr<UnboundTransform> Expressions::Hour(std::string name) {
-  return std::make_shared<UnboundTransform>(Ref(std::move(name)), Transform::Hour());
+  auto result = UnboundTransform::Make(Ref(std::move(name)), Transform::Hour());
+  if (!result) {
+    throw ExpressionError(result.error().message);
+  }
+  return {std::move(result.value())};
 }
 
 std::shared_ptr<UnboundTransform> Expressions::Truncate(std::string name, int32_t width) {
-  return std::make_shared<UnboundTransform>(Ref(std::move(name)),
-                                            Transform::Truncate(width));
+  auto result = UnboundTransform::Make(Ref(std::move(name)), Transform::Truncate(width));
+  if (!result) {
+    throw ExpressionError(result.error().message);
+  }
+  return {std::move(result.value())};
 }
 
 std::shared_ptr<UnboundTransform> Expressions::Transform(
     std::string name, std::shared_ptr<::iceberg::Transform> transform) {
-  return std::make_shared<UnboundTransform>(Ref(std::move(name)), std::move(transform));
+  auto result = UnboundTransform::Make(Ref(std::move(name)), std::move(transform));
+  if (!result) {
+    throw ExpressionError(result.error().message);
+  }
+  return {std::move(result.value())};
 }
 
 // Template implementations for unary predicates
@@ -327,11 +358,15 @@ std::shared_ptr<False> Expressions::AlwaysFalse() { return False::Instance(); }
 // Utilities
 
 std::shared_ptr<NamedReference> Expressions::Ref(std::string name) {
-  return std::make_shared<NamedReference>(std::move(name));
+  auto result = NamedReference::Make(std::move(name));
+  if (!result) {
+    throw ExpressionError(result.error().message);
+  }
+  return {std::move(result.value())};
 }
 
 Literal Expressions::Lit(Literal::Value value, std::shared_ptr<PrimitiveType> type) {
-  throw IcebergError("Literal creation is not implemented");
+  throw ExpressionError("Literal creation is not implemented");
 }
 
 }  // namespace iceberg

src/iceberg/expression/expressions.h

@@ -27,14 +27,17 @@
 #include <string>
 #include <vector>
 
+#include "iceberg/exception.h"
 #include "iceberg/expression/literal.h"
 #include "iceberg/expression/predicate.h"
 #include "iceberg/expression/term.h"
 #include "iceberg/iceberg_export.h"
 
 namespace iceberg {
 
-/// \brief Factory methods for creating expressions.
+/// \brief Factory methods for quickly creating expressions. Unlike most APIs in this
+/// codebase that return Result<T>, these factory methods throw ExpressionError exceptions
+/// on failures to provide a more convenient API for expression construction.
 class ICEBERG_EXPORT Expressions {
  public:
   // Logical operations
@@ -60,7 +63,11 @@ 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));
+      if (!result) {
+        throw ExpressionError(result.error().message);
+      }
+      return {std::move(result.value())};
     } else {
       return And(And(std::move(left), std::move(right)), std::forward<Args>(args)...);
     }
@@ -86,7 +93,11 @@ 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));
+      if (!result) {
+        throw ExpressionError(result.error().message);
+      }
+      return {std::move(result.value())};
     } else {
       return Or(Or(std::move(left), std::move(right)), std::forward<Args>(args)...);
     }