From 7730c88a5a15e4b04f0d9455fd2ef01c9e9e8dfa Mon Sep 17 00:00:00 2001 From: Junwang Zhao Date: Sat, 10 Jan 2026 18:10:18 +0800 Subject: [PATCH 1/2] 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 {}; From a38bea7bca8423a7dba599c5554c273012423d32 Mon Sep 17 00:00:00 2001 From: Junwang Zhao Date: Wed, 14 Jan 2026 10:15:00 +0800 Subject: [PATCH 2/2] fix: review comments --- src/iceberg/expression/binder.cc | 14 ++++++----- src/iceberg/test/expression_visitor_test.cc | 28 ++++++++++----------- 2 files changed, 22 insertions(+), 20 deletions(-) diff --git a/src/iceberg/expression/binder.cc b/src/iceberg/expression/binder.cc index e908768f0..9474151d7 100644 --- a/src/iceberg/expression/binder.cc +++ b/src/iceberg/expression/binder.cc @@ -85,20 +85,22 @@ Result IsBoundVisitor::IsBound(const std::shared_ptr& expr) { return Visit(expr, visitor); } -Result IsBoundVisitor::AlwaysTrue() { return true; } +Result IsBoundVisitor::AlwaysTrue() { + return InvalidExpression("IsBoundVisitor does not support AlwaysTrue expression"); +} -Result IsBoundVisitor::AlwaysFalse() { return true; } +Result IsBoundVisitor::AlwaysFalse() { + return InvalidExpression("IsBoundVisitor does not support AlwaysFalse expression"); +} Result IsBoundVisitor::Not(bool child_result) { return child_result; } Result IsBoundVisitor::And(bool left_result, bool right_result) { - ICEBERG_PRECHECK(left_result == right_result, "Found partially bound expression"); - return left_result; + return left_result && right_result; } Result IsBoundVisitor::Or(bool left_result, bool right_result) { - ICEBERG_PRECHECK(left_result == right_result, "Found partially bound expression"); - return left_result; + return left_result && right_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 017ff9dfe..97d70d7a8 100644 --- a/src/iceberg/test/expression_visitor_test.cc +++ b/src/iceberg/test/expression_visitor_test.cc @@ -296,14 +296,14 @@ TEST_F(BinderTest, ErrorNestedUnboundField) { class IsBoundVisitorTest : public ExpressionVisitorTest {}; TEST_F(IsBoundVisitorTest, Constants) { - // True and False are always considered bound + // True and False should error out auto true_expr = Expressions::AlwaysTrue(); - ICEBERG_UNWRAP_OR_FAIL(auto is_bound_true, IsBoundVisitor::IsBound(true_expr)); - EXPECT_TRUE(is_bound_true); + auto result_true = IsBoundVisitor::IsBound(true_expr); + EXPECT_THAT(result_true, IsError(ErrorKind::kInvalidExpression)); auto false_expr = Expressions::AlwaysFalse(); - ICEBERG_UNWRAP_OR_FAIL(auto is_bound_false, IsBoundVisitor::IsBound(false_expr)); - EXPECT_TRUE(is_bound_false); + auto result_false = IsBoundVisitor::IsBound(false_expr); + EXPECT_THAT(result_false, IsError(ErrorKind::kInvalidExpression)); } TEST_F(IsBoundVisitorTest, UnboundPredicate) { @@ -333,14 +333,14 @@ TEST_F(IsBoundVisitorTest, AndWithBoundChildren) { } TEST_F(IsBoundVisitorTest, AndWithUnboundChild) { - // AND with mixed bound/unbound children should error + // AND with any unbound child should return false 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); - auto result = IsBoundVisitor::IsBound(mixed_and); - EXPECT_THAT(result, HasErrorMessage("Found partially bound expression")); + ICEBERG_UNWRAP_OR_FAIL(auto is_bound, IsBoundVisitor::IsBound(mixed_and)); + EXPECT_FALSE(is_bound); } TEST_F(IsBoundVisitorTest, OrWithBoundChildren) { @@ -355,14 +355,14 @@ TEST_F(IsBoundVisitorTest, OrWithBoundChildren) { } TEST_F(IsBoundVisitorTest, OrWithUnboundChild) { - // OR with mixed bound/unbound children should error + // OR with any unbound child should return false 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); - auto result = IsBoundVisitor::IsBound(mixed_or); - EXPECT_THAT(result, HasErrorMessage("Found partially bound expression")); + ICEBERG_UNWRAP_OR_FAIL(auto is_bound, IsBoundVisitor::IsBound(mixed_or)); + EXPECT_FALSE(is_bound); } 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: mixed bound/unbound children should error + // Complex expression: one unbound should return false 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); - auto result_mixed = IsBoundVisitor::IsBound(mixed_complex); - EXPECT_THAT(result_mixed, HasErrorMessage("Found partially bound expression")); + ICEBERG_UNWRAP_OR_FAIL(auto is_bound_mixed, IsBoundVisitor::IsBound(mixed_complex)); + EXPECT_FALSE(is_bound_mixed); } class RewriteNotTest : public ExpressionVisitorTest {};