diff --git a/src/iceberg/expression/binder.cc b/src/iceberg/expression/binder.cc index 9474151d7..ecc1f1f2a 100644 --- a/src/iceberg/expression/binder.cc +++ b/src/iceberg/expression/binder.cc @@ -24,6 +24,21 @@ namespace iceberg { +namespace { + +Result> CombineResults(const std::optional& left_result, + const std::optional& right_result) { + if (!left_result.has_value()) { + return right_result; + } + if (right_result.has_value() && left_result != right_result) { + return InvalidExpression("Found partially bound expression"); + } + return left_result; +} + +} // namespace + Binder::Binder(const Schema& schema, bool case_sensitive) : schema_(schema), case_sensitive_(case_sensitive) {} @@ -82,40 +97,44 @@ Result> Binder::Aggregate( Result IsBoundVisitor::IsBound(const std::shared_ptr& expr) { ICEBERG_PRECHECK(expr != nullptr, "Expression cannot be null"); IsBoundVisitor visitor; - return Visit(expr, visitor); + ICEBERG_ASSIGN_OR_RAISE(auto is_bound, Visit>(expr, visitor)); + return is_bound.value_or(false); } -Result IsBoundVisitor::AlwaysTrue() { - return InvalidExpression("IsBoundVisitor does not support AlwaysTrue expression"); -} +Result> IsBoundVisitor::AlwaysTrue() { return std::nullopt; } -Result IsBoundVisitor::AlwaysFalse() { - return InvalidExpression("IsBoundVisitor does not support AlwaysFalse expression"); -} +Result> IsBoundVisitor::AlwaysFalse() { return std::nullopt; } -Result IsBoundVisitor::Not(bool child_result) { return child_result; } +Result> IsBoundVisitor::Not(const std::optional& child_result) { + return child_result; +} -Result IsBoundVisitor::And(bool left_result, bool right_result) { - return left_result && right_result; +Result> IsBoundVisitor::And(const std::optional& left_result, + const std::optional& right_result) { + return CombineResults(left_result, right_result); } -Result IsBoundVisitor::Or(bool left_result, bool right_result) { - return left_result && right_result; +Result> IsBoundVisitor::Or(const std::optional& left_result, + const std::optional& right_result) { + return CombineResults(left_result, right_result); } -Result IsBoundVisitor::Predicate(const std::shared_ptr& pred) { +Result> IsBoundVisitor::Predicate( + const std::shared_ptr& pred) { return true; } -Result IsBoundVisitor::Predicate(const std::shared_ptr& pred) { +Result> IsBoundVisitor::Predicate( + const std::shared_ptr& pred) { return false; } -Result IsBoundVisitor::Aggregate(const std::shared_ptr& aggregate) { +Result> IsBoundVisitor::Aggregate( + const std::shared_ptr& aggregate) { return true; } -Result IsBoundVisitor::Aggregate( +Result> IsBoundVisitor::Aggregate( const std::shared_ptr& aggregate) { return false; } diff --git a/src/iceberg/expression/binder.h b/src/iceberg/expression/binder.h index 276ab0760..9aac5b752 100644 --- a/src/iceberg/expression/binder.h +++ b/src/iceberg/expression/binder.h @@ -23,6 +23,7 @@ /// Bind an expression to a schema. #include +#include #include #include "iceberg/expression/expression_visitor.h" @@ -61,19 +62,25 @@ class ICEBERG_EXPORT Binder : public ExpressionVisitor { +class ICEBERG_EXPORT IsBoundVisitor : public ExpressionVisitor> { public: static Result IsBound(const std::shared_ptr& expr); - Result AlwaysTrue() override; - Result AlwaysFalse() override; - Result Not(bool child_result) override; - Result And(bool left_result, bool right_result) override; - Result Or(bool left_result, bool right_result) override; - Result Predicate(const std::shared_ptr& pred) override; - Result Predicate(const std::shared_ptr& pred) override; - Result Aggregate(const std::shared_ptr& aggregate) override; - Result Aggregate(const std::shared_ptr& aggregate) override; + Result> AlwaysTrue() override; + Result> AlwaysFalse() override; + Result> Not(const std::optional& child_result) override; + Result> And(const std::optional& left_result, + const std::optional& right_result) override; + Result> Or(const std::optional& left_result, + const std::optional& right_result) override; + Result> Predicate( + const std::shared_ptr& pred) override; + Result> Predicate( + const std::shared_ptr& pred) override; + Result> Aggregate( + const std::shared_ptr& aggregate) override; + Result> Aggregate( + const std::shared_ptr& aggregate) override; }; using FieldIdsSetRef = std::reference_wrapper>; diff --git a/src/iceberg/test/expression_visitor_test.cc b/src/iceberg/test/expression_visitor_test.cc index a3b2c4cad..e58902dca 100644 --- a/src/iceberg/test/expression_visitor_test.cc +++ b/src/iceberg/test/expression_visitor_test.cc @@ -305,14 +305,15 @@ TEST_F(BinderTest, ErrorNestedUnboundField) { class IsBoundVisitorTest : public ExpressionVisitorTest {}; TEST_F(IsBoundVisitorTest, Constants) { - // True and False should error out auto true_expr = Expressions::AlwaysTrue(); auto result_true = IsBoundVisitor::IsBound(true_expr); - EXPECT_THAT(result_true, IsError(ErrorKind::kInvalidExpression)); + ASSERT_THAT(result_true, IsOk()); + EXPECT_FALSE(*result_true); auto false_expr = Expressions::AlwaysFalse(); auto result_false = IsBoundVisitor::IsBound(false_expr); - EXPECT_THAT(result_false, IsError(ErrorKind::kInvalidExpression)); + ASSERT_THAT(result_false, IsOk()); + EXPECT_FALSE(*result_false); } TEST_F(IsBoundVisitorTest, UnboundPredicate) { @@ -322,6 +323,39 @@ TEST_F(IsBoundVisitorTest, UnboundPredicate) { EXPECT_FALSE(is_bound); } +TEST_F(IsBoundVisitorTest, AndWithConstants) { + auto unbound = Expressions::Equal("name", Literal::String("Alice")); + ICEBERG_UNWRAP_OR_FAIL(auto bound, Bind(unbound)); + + // Use Make to keep constants in the expression tree instead of folding them. + ICEBERG_UNWRAP_OR_FAIL(std::shared_ptr unbound_and, + And::Make(Expressions::AlwaysTrue(), unbound)); + ICEBERG_UNWRAP_OR_FAIL(auto unbound_result, IsBoundVisitor::IsBound(unbound_and)); + EXPECT_FALSE(unbound_result); + EXPECT_THAT(Bind(unbound_and), IsOk()); + + ICEBERG_UNWRAP_OR_FAIL(std::shared_ptr bound_and, + And::Make(bound, Expressions::AlwaysTrue())); + ICEBERG_UNWRAP_OR_FAIL(auto bound_result, IsBoundVisitor::IsBound(bound_and)); + EXPECT_TRUE(bound_result); +} + +TEST_F(IsBoundVisitorTest, OrWithConstants) { + auto unbound = Expressions::Equal("name", Literal::String("Alice")); + ICEBERG_UNWRAP_OR_FAIL(auto bound, Bind(unbound)); + + ICEBERG_UNWRAP_OR_FAIL(std::shared_ptr unbound_or, + Or::Make(Expressions::AlwaysFalse(), unbound)); + ICEBERG_UNWRAP_OR_FAIL(auto unbound_result, IsBoundVisitor::IsBound(unbound_or)); + EXPECT_FALSE(unbound_result); + EXPECT_THAT(Bind(unbound_or), IsOk()); + + ICEBERG_UNWRAP_OR_FAIL(std::shared_ptr bound_or, + Or::Make(bound, Expressions::AlwaysFalse())); + ICEBERG_UNWRAP_OR_FAIL(auto bound_result, IsBoundVisitor::IsBound(bound_or)); + EXPECT_TRUE(bound_result); +} + TEST_F(IsBoundVisitorTest, BoundPredicate) { // Bound predicates should return true auto unbound_pred = Expressions::Equal("name", Literal::String("Alice")); @@ -342,14 +376,14 @@ TEST_F(IsBoundVisitorTest, AndWithBoundChildren) { } TEST_F(IsBoundVisitorTest, AndWithUnboundChild) { - // AND with any unbound child should return false + // Mixing bound and unbound predicates is invalid. auto bound_pred = Expressions::Equal("name", Literal::String("Alice")); ICEBERG_UNWRAP_OR_FAIL(auto pred1, Bind(bound_pred)); auto pred2 = Expressions::Equal("age", Literal::Int(25)); // unbound auto mixed_and = Expressions::And(pred1, pred2); - ICEBERG_UNWRAP_OR_FAIL(auto is_bound, IsBoundVisitor::IsBound(mixed_and)); - EXPECT_FALSE(is_bound); + EXPECT_THAT(IsBoundVisitor::IsBound(mixed_and), + HasErrorMessage("Found partially bound expression")); } TEST_F(IsBoundVisitorTest, OrWithBoundChildren) { @@ -364,14 +398,14 @@ TEST_F(IsBoundVisitorTest, OrWithBoundChildren) { } TEST_F(IsBoundVisitorTest, OrWithUnboundChild) { - // OR with any unbound child should return false + // Mixing bound and unbound predicates is invalid. auto pred1 = Expressions::IsNull("name"); // unbound auto bound_pred2 = Expressions::Equal("age", Literal::Int(25)); ICEBERG_UNWRAP_OR_FAIL(auto pred2, Bind(bound_pred2)); auto mixed_or = Expressions::Or(pred1, pred2); - ICEBERG_UNWRAP_OR_FAIL(auto is_bound, IsBoundVisitor::IsBound(mixed_or)); - EXPECT_FALSE(is_bound); + EXPECT_THAT(IsBoundVisitor::IsBound(mixed_or), + HasErrorMessage("Found partially bound expression")); } TEST_F(IsBoundVisitorTest, NotWithBoundChild) { @@ -405,15 +439,15 @@ TEST_F(IsBoundVisitorTest, ComplexExpression) { ICEBERG_UNWRAP_OR_FAIL(auto is_bound, IsBoundVisitor::IsBound(bound_complex)); EXPECT_TRUE(is_bound); - // Complex expression: one unbound should return false + // Mixed binding state is rejected even in a nested expression. auto unbound_pred = Expressions::Equal("name", Literal::String("Alice")); ICEBERG_UNWRAP_OR_FAIL(auto bound_pred2, Bind(pred2)); ICEBERG_UNWRAP_OR_FAIL(auto bound_pred3, Bind(pred3)); auto mixed_and = Expressions::And(unbound_pred, bound_pred2); auto mixed_complex = Expressions::Or(mixed_and, bound_pred3); - ICEBERG_UNWRAP_OR_FAIL(auto is_bound_mixed, IsBoundVisitor::IsBound(mixed_complex)); - EXPECT_FALSE(is_bound_mixed); + EXPECT_THAT(IsBoundVisitor::IsBound(mixed_complex), + HasErrorMessage("Found partially bound expression")); } class RewriteNotTest : public ExpressionVisitorTest {};