(iceberg-cpp) branch main updated: feat: add `or` expression (#120)

[fo...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

fokko pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/iceberg-cpp.git


The following commit(s) were added to refs/heads/main by this push:
     new f2a79a1  feat: add `or` expression (#120)
f2a79a1 is described below

commit f2a79a14a4ef5d8200d0240d1122307be2a06e3f
Author: Ying Cai <yi...@gmail.com>
AuthorDate: Wed Jun 25 23:03:00 2025 +0800

    feat: add `or` expression (#120)
    
    - or expression
     - change negate return type form Result to Expression
     - add some tests
---
 src/iceberg/expression/expression.cc |  38 +++++++++---
 src/iceberg/expression/expression.h  |  48 ++++++++++++---
 test/expression_test.cc              | 114 +++++++++++++++++++++++++++++++----
 3 files changed, 174 insertions(+), 26 deletions(-)

diff --git a/src/iceberg/expression/expression.cc b/src/iceberg/expression/expression.cc
index 77f341e..c6fa940 100644
--- a/src/iceberg/expression/expression.cc
+++ b/src/iceberg/expression/expression.cc
@@ -21,8 +21,6 @@
 
 #include <format>
 
-#include "iceberg/result.h"
-
 namespace iceberg {
 
 // True implementation
@@ -31,7 +29,7 @@ const std::shared_ptr<True>& True::Instance() {
   return instance;
 }
 
-Result<std::shared_ptr<Expression>> True::Negate() const { return False::Instance(); }
+std::shared_ptr<Expression> True::Negate() const { return False::Instance(); }
 
 // False implementation
 const std::shared_ptr<False>& False::Instance() {
@@ -39,7 +37,7 @@ const std::shared_ptr<False>& False::Instance() {
   return instance;
 }
 
-Result<std::shared_ptr<Expression>> False::Negate() const { return True::Instance(); }
+std::shared_ptr<Expression> False::Negate() const { return True::Instance(); }
 
 // And implementation
 And::And(std::shared_ptr<Expression> left, std::shared_ptr<Expression> right)
@@ -49,9 +47,11 @@ std::string And::ToString() const {
   return std::format("({} and {})", left_->ToString(), right_->ToString());
 }
 
-Result<std::shared_ptr<Expression>> And::Negate() const {
-  // TODO(yingcai-cy): Implement Or expression
-  return InvalidExpression("And negation not yet implemented");
+std::shared_ptr<Expression> And::Negate() const {
+  // De Morgan's law: not(A and B) = (not A) or (not B)
+  auto left_negated = left_->Negate();
+  auto right_negated = right_->Negate();
+  return std::make_shared<Or>(left_negated, right_negated);
 }
 
 bool And::Equals(const Expression& expr) const {
@@ -63,4 +63,28 @@ bool And::Equals(const Expression& expr) const {
   return false;
 }
 
+// Or implementation
+Or::Or(std::shared_ptr<Expression> left, std::shared_ptr<Expression> right)
+    : left_(std::move(left)), right_(std::move(right)) {}
+
+std::string Or::ToString() const {
+  return std::format("({} or {})", left_->ToString(), right_->ToString());
+}
+
+std::shared_ptr<Expression> Or::Negate() const {
+  // De Morgan's law: not(A or B) = (not A) and (not B)
+  auto left_negated = left_->Negate();
+  auto right_negated = right_->Negate();
+  return std::make_shared<And>(left_negated, right_negated);
+}
+
+bool Or::Equals(const Expression& expr) const {
+  if (expr.op() == Operation::kOr) {
+    const auto& other = static_cast<const Or&>(expr);
+    return (left_->Equals(*other.left()) && right_->Equals(*other.right())) ||
+           (left_->Equals(*other.right()) && right_->Equals(*other.left()));
+  }
+  return false;
+}
+
 }  // namespace iceberg
diff --git a/src/iceberg/expression/expression.h b/src/iceberg/expression/expression.h
index 342122f..9ceae1c 100644
--- a/src/iceberg/expression/expression.h
+++ b/src/iceberg/expression/expression.h
@@ -25,9 +25,8 @@
 #include <memory>
 #include <string>
 
-#include "iceberg/expected.h"
+#include "iceberg/exception.h"
 #include "iceberg/iceberg_export.h"
-#include "iceberg/result.h"
 
 namespace iceberg {
 
@@ -67,8 +66,8 @@ class ICEBERG_EXPORT Expression {
   virtual Operation op() const = 0;
 
   /// \brief Returns the negation of this expression, equivalent to not(this).
-  virtual Result<std::shared_ptr<Expression>> Negate() const {
-    return InvalidExpression("Expression cannot be negated");
+  virtual std::shared_ptr<Expression> Negate() const {
+    throw IcebergError("Expression cannot be negated");
   }
 
   /// \brief Returns whether this expression will accept the same values as another.
@@ -94,7 +93,7 @@ class ICEBERG_EXPORT True : public Expression {
 
   std::string ToString() const override { return "true"; }
 
-  Result<std::shared_ptr<Expression>> Negate() const override;
+  std::shared_ptr<Expression> Negate() const override;
 
   bool Equals(const Expression& other) const override {
     return other.op() == Operation::kTrue;
@@ -114,7 +113,7 @@ class ICEBERG_EXPORT False : public Expression {
 
   std::string ToString() const override { return "false"; }
 
-  Result<std::shared_ptr<Expression>> Negate() const override;
+  std::shared_ptr<Expression> Negate() const override;
 
   bool Equals(const Expression& other) const override {
     return other.op() == Operation::kFalse;
@@ -150,7 +149,42 @@ class ICEBERG_EXPORT And : public Expression {
 
   std::string ToString() const override;
 
-  Result<std::shared_ptr<Expression>> Negate() const override;
+  std::shared_ptr<Expression> Negate() const override;
+
+  bool Equals(const Expression& other) const override;
+
+ private:
+  std::shared_ptr<Expression> left_;
+  std::shared_ptr<Expression> right_;
+};
+
+/// \brief An Expression that represents a logical OR operation between two expressions.
+///
+/// This expression evaluates to true if at least one of its child expressions
+/// evaluates to true.
+class ICEBERG_EXPORT Or : public Expression {
+ public:
+  /// \brief Constructs 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);
+
+  /// \brief Returns the left operand of the OR expression.
+  ///
+  /// \return The left operand of the OR expression
+  const std::shared_ptr<Expression>& left() const { return left_; }
+
+  /// \brief Returns the right operand of the OR expression.
+  ///
+  /// \return The right operand of the OR expression
+  const std::shared_ptr<Expression>& right() const { return right_; }
+
+  Operation op() const override { return Operation::kOr; }
+
+  std::string ToString() const override;
+
+  std::shared_ptr<Expression> Negate() const override;
 
   bool Equals(const Expression& other) const override;
 
diff --git a/test/expression_test.cc b/test/expression_test.cc
index f722d62..c14c7d9 100644
--- a/test/expression_test.cc
+++ b/test/expression_test.cc
@@ -30,25 +30,17 @@ TEST(TrueFalseTest, Basic) {
   auto false_instance = False::Instance();
   auto negated = false_instance->Negate();
 
-  EXPECT_TRUE(negated.has_value());
-
   // Check that negated expression is True
-  auto true_expr = negated.value();
-  EXPECT_EQ(true_expr->op(), Expression::Operation::kTrue);
-
-  EXPECT_EQ(true_expr->ToString(), "true");
+  EXPECT_EQ(negated->op(), Expression::Operation::kTrue);
+  EXPECT_EQ(negated->ToString(), "true");
 
   // Test negation of True returns false
   auto true_instance = True::Instance();
   negated = true_instance->Negate();
 
-  EXPECT_TRUE(negated.has_value());
-
   // Check that negated expression is False
-  auto false_expr = negated.value();
-  EXPECT_EQ(false_expr->op(), Expression::Operation::kFalse);
-
-  EXPECT_EQ(false_expr->ToString(), "false");
+  EXPECT_EQ(negated->op(), Expression::Operation::kFalse);
+  EXPECT_EQ(negated->ToString(), "false");
 }
 
 TEST(ANDTest, Basic) {
@@ -64,4 +56,102 @@ TEST(ANDTest, Basic) {
   EXPECT_EQ(and_expr->left()->op(), Expression::Operation::kTrue);
   EXPECT_EQ(and_expr->right()->op(), Expression::Operation::kTrue);
 }
+
+TEST(ORTest, Basic) {
+  // Create True and False expressions
+  auto true_expr = True::Instance();
+  auto false_expr = False::Instance();
+
+  // Create an OR expression
+  auto or_expr = std::make_shared<Or>(true_expr, false_expr);
+
+  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);
+}
+
+TEST(ORTest, Negation) {
+  // Test De Morgan's law: not(A or B) = (not A) and (not B)
+  auto true_expr = True::Instance();
+  auto false_expr = False::Instance();
+
+  auto or_expr = std::make_shared<Or>(true_expr, false_expr);
+  auto negated_or = or_expr->Negate();
+
+  // Should become AND expression
+  EXPECT_EQ(negated_or->op(), Expression::Operation::kAnd);
+  EXPECT_EQ(negated_or->ToString(), "(false and true)");
+}
+
+TEST(ORTest, Equals) {
+  auto true_expr = True::Instance();
+  auto false_expr = False::Instance();
+
+  // Test basic equality
+  auto or_expr1 = std::make_shared<Or>(true_expr, false_expr);
+  auto or_expr2 = std::make_shared<Or>(true_expr, false_expr);
+  EXPECT_TRUE(or_expr1->Equals(*or_expr2));
+
+  // Test commutativity: (A or B) equals (B or A)
+  auto or_expr3 = std::make_shared<Or>(false_expr, true_expr);
+  EXPECT_TRUE(or_expr1->Equals(*or_expr3));
+
+  // Test inequality with different expressions
+  auto or_expr4 = std::make_shared<Or>(true_expr, true_expr);
+  EXPECT_FALSE(or_expr1->Equals(*or_expr4));
+
+  // Test inequality with different operation types
+  auto and_expr = std::make_shared<And>(true_expr, false_expr);
+  EXPECT_FALSE(or_expr1->Equals(*and_expr));
+}
+
+TEST(ANDTest, Negation) {
+  // Test De Morgan's law: not(A and B) = (not A) or (not B)
+  auto true_expr = True::Instance();
+  auto false_expr = False::Instance();
+
+  auto and_expr = std::make_shared<And>(true_expr, false_expr);
+  auto negated_and = and_expr->Negate();
+
+  // Should become OR expression
+  EXPECT_EQ(negated_and->op(), Expression::Operation::kOr);
+  EXPECT_EQ(negated_and->ToString(), "(false or true)");
+}
+
+TEST(ANDTest, Equals) {
+  auto true_expr = True::Instance();
+  auto false_expr = False::Instance();
+
+  // Test basic equality
+  auto and_expr1 = std::make_shared<And>(true_expr, false_expr);
+  auto and_expr2 = std::make_shared<And>(true_expr, false_expr);
+  EXPECT_TRUE(and_expr1->Equals(*and_expr2));
+
+  // Test commutativity: (A and B) equals (B and A)
+  auto and_expr3 = std::make_shared<And>(false_expr, true_expr);
+  EXPECT_TRUE(and_expr1->Equals(*and_expr3));
+
+  // Test inequality with different expressions
+  auto and_expr4 = std::make_shared<And>(true_expr, true_expr);
+  EXPECT_FALSE(and_expr1->Equals(*and_expr4));
+
+  // Test inequality with different operation types
+  auto or_expr = std::make_shared<Or>(true_expr, false_expr);
+  EXPECT_FALSE(and_expr1->Equals(*or_expr));
+}
+
+TEST(ExpressionTest, BaseClassNegateThrowsException) {
+  // Create a mock expression that doesn't override Negate()
+  class MockExpression : public Expression {
+   public:
+    Operation op() const override { return Operation::kTrue; }
+    // Deliberately not overriding Negate() to test base class behavior
+  };
+
+  auto mock_expr = std::make_shared<MockExpression>();
+
+  // Should throw IcebergError when calling Negate() on base class
+  EXPECT_THROW(mock_expr->Negate(), IcebergError);
+}
 }  // namespace iceberg