Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 31 additions & 7 deletions src/iceberg/expression/expression.cc
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,6 @@

#include <format>

#include "iceberg/result.h"

namespace iceberg {

// True implementation
Expand All @@ -31,15 +29,15 @@ 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() {
static const std::shared_ptr<False> instance = std::shared_ptr<False>(new False());
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)
Expand All @@ -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 {
Expand All @@ -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
48 changes: 41 additions & 7 deletions src/iceberg/expression/expression.h
Original file line number Diff line number Diff line change
Expand Up @@ -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 {

Expand Down Expand Up @@ -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");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why changing the return type? We need to stick to Result instead of exception.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It’s been a couple of days, let me recall. I believe the reason is that all expression implementations have a corresponding Negate form, so none of them would return incorrect results in this context.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything can be negated in theory, and this is also the case in practice.

}

/// \brief Returns whether this expression will accept the same values as another.
Expand All @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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;

Expand Down
114 changes: 102 additions & 12 deletions test/expression_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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
Loading