From d6fa4cf9c9e88954e57c17550f0eba6f7513075e Mon Sep 17 00:00:00 2001 From: yinzhengsun Date: Mon, 12 Oct 2026 17:11:24 +0800 Subject: [PATCH] fix(expression): treat constants as neutral in IsBoundVisitor AlwaysTrue/AlwaysFalse no longer make IsBoundVisitor fail. Constants contain no references and are now neutral when combining binding states, so And(True, unbound) is unbound and And(True, bound) is bound. Mixing bound and unbound predicates is rejected with an error. This follows Java's IsBoundVisitor. --- src/iceberg/expression/binder.cc | 51 ++++++++++++------ src/iceberg/expression/binder.h | 27 ++++++---- src/iceberg/test/expression_visitor_test.cc | 58 ++++++++++++++++----- 3 files changed, 98 insertions(+), 38 deletions(-) diff --git a/src/iceberg/expression/binder.cc b/src/iceberg/expression/binder.cc index 9474151d71..ecc1f1f2a9 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 276ab07604..9aac5b7529 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 a3b2c4cad1..e58902dcac 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 {};