From 7730c88a5a15e4b04f0d9455fd2ef01c9e9e8dfa Mon Sep 17 00:00:00 2001 From: Junwang Zhao Date: Sat, 10 Jan 2026 18:10:18 +0800 Subject: [PATCH] IsBoundVisitor mixed bound/unbound predicate should error --- src/iceberg/expression/binder.cc | 6 ++++-- src/iceberg/test/expression_visitor_test.cc | 18 +++++++++--------- 2 files changed, 13 insertions(+), 11 deletions(-) diff --git a/src/iceberg/expression/binder.cc b/src/iceberg/expression/binder.cc index 650dc730d..e908768f0 100644 --- a/src/iceberg/expression/binder.cc +++ b/src/iceberg/expression/binder.cc @@ -92,11 +92,13 @@ Result IsBoundVisitor::AlwaysFalse() { return true; } Result IsBoundVisitor::Not(bool child_result) { return child_result; } Result IsBoundVisitor::And(bool left_result, bool right_result) { - return left_result && right_result; + ICEBERG_PRECHECK(left_result == right_result, "Found partially bound expression"); + return left_result; } Result IsBoundVisitor::Or(bool left_result, bool right_result) { - return left_result && right_result; + ICEBERG_PRECHECK(left_result == right_result, "Found partially bound expression"); + return left_result; } Result IsBoundVisitor::Predicate(const std::shared_ptr& pred) { diff --git a/src/iceberg/test/expression_visitor_test.cc b/src/iceberg/test/expression_visitor_test.cc index 697c0096a..017ff9dfe 100644 --- a/src/iceberg/test/expression_visitor_test.cc +++ b/src/iceberg/test/expression_visitor_test.cc @@ -333,14 +333,14 @@ TEST_F(IsBoundVisitorTest, AndWithBoundChildren) { } TEST_F(IsBoundVisitorTest, AndWithUnboundChild) { - // AND with any unbound child should return false + // AND with mixed bound/unbound children should error 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); + auto result = IsBoundVisitor::IsBound(mixed_and); + EXPECT_THAT(result, HasErrorMessage("Found partially bound expression")); } TEST_F(IsBoundVisitorTest, OrWithBoundChildren) { @@ -355,14 +355,14 @@ TEST_F(IsBoundVisitorTest, OrWithBoundChildren) { } TEST_F(IsBoundVisitorTest, OrWithUnboundChild) { - // OR with any unbound child should return false + // OR with mixed bound/unbound children should error 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); + auto result = IsBoundVisitor::IsBound(mixed_or); + EXPECT_THAT(result, HasErrorMessage("Found partially bound expression")); } TEST_F(IsBoundVisitorTest, NotWithBoundChild) { @@ -396,15 +396,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 + // Complex expression: mixed bound/unbound children should error 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); + auto result_mixed = IsBoundVisitor::IsBound(mixed_complex); + EXPECT_THAT(result_mixed, HasErrorMessage("Found partially bound expression")); } class RewriteNotTest : public ExpressionVisitorTest {};