fix conflict

Author: wgtmac

src/iceberg/expression/expression.cc

@@ -55,8 +55,7 @@ Result<std::unique_ptr<And>> And::Make(std::shared_ptr<Expression> left,
 
 And::And(std::shared_ptr<Expression> left, std::shared_ptr<Expression> 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<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");
+  if (!left || !right) [[unlikely]] {
+    return InvalidExpression("Or cannot have null children");
   }
   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)) {
-  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<std::unique_ptr<Not>> Not::Make(std::shared_ptr<Expression> child) {
-  if (!child) {
+  if (!child) [[unlikely]] {
     return InvalidExpression("Not expression cannot have null child");
   }
   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");
+  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<Expression::Operation> 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();
 }

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<std::unique_ptr<And>> Make(std::shared_ptr<Expression> left,
                                            std::shared_ptr<Expression> 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<std::unique_ptr<Or>> Make(std::shared_ptr<Expression> left,
                                           std::shared_ptr<Expression> right);
 

src/iceberg/expression/expressions.cc

@@ -42,9 +42,8 @@ std::shared_ptr<Expression> Expressions::Not(std::shared_ptr<Expression> 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,46 +53,46 @@ std::shared_ptr<UnboundTransform> 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<UnboundTransform> 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<UnboundTransform> 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<UnboundTransform> 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<UnboundTransform> 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<UnboundTransform> 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<UnboundTransform> Expressions::Transform(
     std::string name, std::shared_ptr<::iceberg::Transform> 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<False> Expressions::AlwaysFalse() { return False::Instance(); }
 
 std::shared_ptr<NamedReference> 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<PrimitiveType> type) {

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>(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>(args)...);
     }

src/iceberg/expression/term.cc

@@ -44,14 +44,14 @@ Result<std::shared_ptr<B>> Unbound<B>::Bind(const Schema& schema) const {
 // NamedReference implementation
 Result<std::unique_ptr<NamedReference>> 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<NamedReference>(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<std::unique_ptr<BoundReference>> 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<BoundReference>(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<std::unique_ptr<UnboundTransform>> UnboundTransform::Make(
     std::shared_ptr<NamedReference> ref, std::shared_ptr<Transform> 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<UnboundTransform>(
       new UnboundTransform(std::move(ref), std::move(transform)));
@@ -117,8 +120,8 @@ Result<std::unique_ptr<UnboundTransform>> UnboundTransform::Make(
 UnboundTransform::UnboundTransform(std::shared_ptr<NamedReference> ref,
                                    std::shared_ptr<Transform> 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<std::unique_ptr<BoundTransform>> BoundTransform::Make(
     std::shared_ptr<BoundReference> ref, std::shared_ptr<Transform> transform,
     std::shared_ptr<TransformFunction> 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<BoundTransform>(new BoundTransform(
       std::move(ref), std::move(transform), std::move(transform_func)));
@@ -152,10 +156,8 @@ BoundTransform::BoundTransform(std::shared_ptr<BoundReference> 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;

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<std::unique_ptr<NamedReference>> 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<std::unique_ptr<BoundReference>> Make(SchemaField field);
 
   ~BoundReference() override;
@@ -206,8 +198,6 @@ class ICEBERG_EXPORT UnboundTransform : public UnboundTerm<class BoundTransform>
   ///
   /// \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<std::unique_ptr<UnboundTransform>> Make(
       std::shared_ptr<NamedReference> ref, std::shared_ptr<Transform> 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<std::unique_ptr<BoundTransform>> Make(
       std::shared_ptr<BoundReference> ref, std::shared_ptr<Transform> transform,
       std::shared_ptr<TransformFunction> transform_func);

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",

src/iceberg/schema_field.h

@@ -29,6 +29,7 @@
 #include <string_view>
 
 #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);
   }

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) \