This is an automated email from the ASF dual-hosted git repository.
wgtmac pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/iceberg-cpp.git
The following commit(s) were added to refs/heads/main by this push:
new f799dcbc fix(expression): treat constants as neutral in IsBoundVisitor
(#1004)
f799dcbc is described below
commit f799dcbc41a19f589123a4f5966075503347a49a
Author: YinZheng-Sun <[email protected]>
AuthorDate: Tue Oct 13 10:36:43 2026 +0800
fix(expression): treat constants as neutral in IsBoundVisitor (#1004)
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.
Co-authored-by: yinzhengsun <[email protected]>
---
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 9474151d..ecc1f1f2 100644
--- a/src/iceberg/expression/binder.cc
+++ b/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;
}
diff --git a/src/iceberg/expression/binder.h b/src/iceberg/expression/binder.h
index 276ab076..9aac5b75 100644
--- a/src/iceberg/expression/binder.h
+++ b/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>>;
diff --git a/src/iceberg/test/expression_visitor_test.cc
b/src/iceberg/test/expression_visitor_test.cc
index a3b2c4ca..e58902dc 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<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 {};
