fix(expression): treat constants as neutral in IsBoundVisitor

src/iceberg/expression/binder.cc

@@ -24,6 +24,21 @@
 
 namespace iceberg {
 
+namespace {
+
+Result<std::optional<bool>> CombineResults(const std::optional<bool>& left_result,
+                                           const std::optional<bool>& 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<std::shared_ptr<Expression>> Binder::Aggregate(
 Result<bool> IsBoundVisitor::IsBound(const std::shared_ptr<Expression>& expr) {
   ICEBERG_PRECHECK(expr != nullptr, "Expression cannot be null");
   IsBoundVisitor visitor;
-  return Visit<bool, IsBoundVisitor>(expr, visitor);
+  ICEBERG_ASSIGN_OR_RAISE(auto is_bound, Visit<std::optional<bool>>(expr, visitor));
+  return is_bound.value_or(false);
 }
 
-Result<bool> IsBoundVisitor::AlwaysTrue() {
-  return InvalidExpression("IsBoundVisitor does not support AlwaysTrue expression");
-}
+Result<std::optional<bool>> IsBoundVisitor::AlwaysTrue() { return std::nullopt; }
 
-Result<bool> IsBoundVisitor::AlwaysFalse() {
-  return InvalidExpression("IsBoundVisitor does not support AlwaysFalse expression");
-}
+Result<std::optional<bool>> IsBoundVisitor::AlwaysFalse() { return std::nullopt; }
 
-Result<bool> IsBoundVisitor::Not(bool child_result) { return child_result; }
+Result<std::optional<bool>> IsBoundVisitor::Not(const std::optional<bool>& child_result) {
+  return child_result;
+}
 
-Result<bool> IsBoundVisitor::And(bool left_result, bool right_result) {
-  return left_result && right_result;
+Result<std::optional<bool>> IsBoundVisitor::And(const std::optional<bool>& left_result,
+                                                const std::optional<bool>& right_result) {
+  return CombineResults(left_result, right_result);
 }
 
-Result<bool> IsBoundVisitor::Or(bool left_result, bool right_result) {
-  return left_result && right_result;
+Result<std::optional<bool>> IsBoundVisitor::Or(const std::optional<bool>& left_result,
+                                               const std::optional<bool>& right_result) {
+  return CombineResults(left_result, right_result);
 }
 
-Result<bool> IsBoundVisitor::Predicate(const std::shared_ptr<BoundPredicate>& pred) {
+Result<std::optional<bool>> IsBoundVisitor::Predicate(
+    const std::shared_ptr<BoundPredicate>& pred) {
   return true;
 }
 
-Result<bool> IsBoundVisitor::Predicate(const std::shared_ptr<UnboundPredicate>& pred) {
+Result<std::optional<bool>> IsBoundVisitor::Predicate(
+    const std::shared_ptr<UnboundPredicate>& pred) {
   return false;
 }
 
-Result<bool> IsBoundVisitor::Aggregate(const std::shared_ptr<BoundAggregate>& aggregate) {
+Result<std::optional<bool>> IsBoundVisitor::Aggregate(
+    const std::shared_ptr<BoundAggregate>& aggregate) {
   return true;
 }
 
-Result<bool> IsBoundVisitor::Aggregate(
+Result<std::optional<bool>> IsBoundVisitor::Aggregate(
     const std::shared_ptr<UnboundAggregate>& aggregate) {
   return false;
 }

src/iceberg/expression/binder.h

@@ -23,6 +23,7 @@
 /// Bind an expression to a schema.
 
 #include <functional>
+#include <optional>
 #include <unordered_set>
 
 #include "iceberg/expression/expression_visitor.h"
@@ -61,19 +62,25 @@ class ICEBERG_EXPORT Binder : public ExpressionVisitor<std::shared_ptr<Expressio
   const bool case_sensitive_;
 };
 
-class ICEBERG_EXPORT IsBoundVisitor : public ExpressionVisitor<bool> {
+class ICEBERG_EXPORT IsBoundVisitor : public ExpressionVisitor<std::optional<bool>> {
  public:
   static Result<bool> IsBound(const std::shared_ptr<Expression>& expr);
 
-  Result<bool> AlwaysTrue() override;
-  Result<bool> AlwaysFalse() override;
-  Result<bool> Not(bool child_result) override;
-  Result<bool> And(bool left_result, bool right_result) override;
-  Result<bool> Or(bool left_result, bool right_result) override;
-  Result<bool> Predicate(const std::shared_ptr<BoundPredicate>& pred) override;
-  Result<bool> Predicate(const std::shared_ptr<UnboundPredicate>& pred) override;
-  Result<bool> Aggregate(const std::shared_ptr<BoundAggregate>& aggregate) override;
-  Result<bool> Aggregate(const std::shared_ptr<UnboundAggregate>& aggregate) override;
+  Result<std::optional<bool>> AlwaysTrue() override;
+  Result<std::optional<bool>> AlwaysFalse() override;
+  Result<std::optional<bool>> Not(const std::optional<bool>& child_result) override;
+  Result<std::optional<bool>> And(const std::optional<bool>& left_result,
+                                  const std::optional<bool>& right_result) override;
+  Result<std::optional<bool>> Or(const std::optional<bool>& left_result,
+                                 const std::optional<bool>& right_result) override;
+  Result<std::optional<bool>> Predicate(
+      const std::shared_ptr<BoundPredicate>& pred) override;
+  Result<std::optional<bool>> Predicate(
+      const std::shared_ptr<UnboundPredicate>& pred) override;
+  Result<std::optional<bool>> Aggregate(
+      const std::shared_ptr<BoundAggregate>& aggregate) override;
+  Result<std::optional<bool>> Aggregate(
+      const std::shared_ptr<UnboundAggregate>& aggregate) override;
 };
 
 using FieldIdsSetRef = std::reference_wrapper<std::unordered_set<int32_t>>;

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<Expression> 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<Expression> 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<Expression> 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<Expression> 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 {};