IsBoundVisitor mixed bound/unbound predicate should error

Author: zhjwpku

src/iceberg/expression/binder.cc

@@ -19,8 +19,25 @@
 
 #include "iceberg/expression/binder.h"
 
+#include "iceberg/util/macros.h"
+
 namespace iceberg {
 
+namespace {
+
+Result<std::optional<bool>> CombineResults(const std::optional<bool>& is_left_bound,
+                                           const std::optional<bool>& is_right_bound) {
+  if (is_left_bound.has_value()) {
+    ICEBERG_CHECK(!is_right_bound.has_value() || is_left_bound == is_right_bound,
+                  "Found partially bound expression");
+    return is_left_bound;
+  } else {
+    return is_right_bound;
+  }
+}
+
+}  // anonymous namespace
+
 Binder::Binder(const Schema& schema, bool case_sensitive)
     : schema_(schema), case_sensitive_(case_sensitive) {}
 
@@ -79,36 +96,47 @@ Result<std::shared_ptr<Expression>> Binder::Aggregate(
 Result<bool> IsBoundVisitor::IsBound(const std::shared_ptr<Expression>& expr) {
   ICEBERG_DCHECK(expr != nullptr, "Expression cannot be null");
   IsBoundVisitor visitor;
-  return Visit<bool, IsBoundVisitor>(expr, visitor);
+  auto visit_result = Visit<std::optional<bool>, IsBoundVisitor>(expr, visitor);
+  ICEBERG_RETURN_UNEXPECTED(visit_result);
+  auto result = std::move(visit_result.value());
+  // If the result is null, return true
+  return result.value_or(true);
 }
 
-Result<bool> IsBoundVisitor::AlwaysTrue() { return true; }
+Result<std::optional<bool>> IsBoundVisitor::AlwaysTrue() { return std::nullopt; }
 
-Result<bool> IsBoundVisitor::AlwaysFalse() { return true; }
+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

@@ -22,6 +22,8 @@
 /// \file iceberg/expression/binder.h
 /// Bind an expression to a schema.
 
+#include <optional>
+
 #include "iceberg/expression/expression_visitor.h"
 
 namespace iceberg {
@@ -58,19 +60,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;
 };
 
 // TODO(gangwu): add the Java parity `ReferenceVisitor`

src/iceberg/test/expression_visitor_test.cc

@@ -332,14 +332,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) {
@@ -354,14 +354,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) {
@@ -395,15 +395,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 {};