Skip to content

Commit f799dcb

Browse files
YinZheng-Sunyinzhengsun
YinZheng-Sun
and
yinzhengsun
authored
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 <yinzhengsun@tencent.com>
1 parent 7543e80 commit f799dcb

3 files changed

Lines changed: 98 additions & 38 deletions

File tree

‎src/iceberg/expression/binder.cc‎

Lines changed: 35 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,21 @@
2424

2525
namespace iceberg {
2626

27+
namespace {
28+
29+
Result<std::optional<bool>> CombineResults(const std::optional<bool>& left_result,
30+
const std::optional<bool>& right_result) {
31+
if (!left_result.has_value()) {
32+
return right_result;
33+
}
34+
if (right_result.has_value() && left_result != right_result) {
35+
return InvalidExpression("Found partially bound expression");
36+
}
37+
return left_result;
38+
}
39+
40+
} // namespace
41+
2742
Binder::Binder(const Schema& schema, bool case_sensitive)
2843
: schema_(schema), case_sensitive_(case_sensitive) {}
2944

@@ -82,40 +97,44 @@ Result<std::shared_ptr<Expression>> Binder::Aggregate(
8297
Result<bool> IsBoundVisitor::IsBound(const std::shared_ptr<Expression>& expr) {
8398
ICEBERG_PRECHECK(expr != nullptr, "Expression cannot be null");
8499
IsBoundVisitor visitor;
85-
return Visit<bool, IsBoundVisitor>(expr, visitor);
100+
ICEBERG_ASSIGN_OR_RAISE(auto is_bound, Visit<std::optional<bool>>(expr, visitor));
101+
return is_bound.value_or(false);
86102
}
87103

88-
Result<bool> IsBoundVisitor::AlwaysTrue() {
89-
return InvalidExpression("IsBoundVisitor does not support AlwaysTrue expression");
90-
}
104+
Result<std::optional<bool>> IsBoundVisitor::AlwaysTrue() { return std::nullopt; }
91105

92-
Result<bool> IsBoundVisitor::AlwaysFalse() {
93-
return InvalidExpression("IsBoundVisitor does not support AlwaysFalse expression");
94-
}
106+
Result<std::optional<bool>> IsBoundVisitor::AlwaysFalse() { return std::nullopt; }
95107

96-
Result<bool> IsBoundVisitor::Not(bool child_result) { return child_result; }
108+
Result<std::optional<bool>> IsBoundVisitor::Not(const std::optional<bool>& child_result) {
109+
return child_result;
110+
}
97111

98-
Result<bool> IsBoundVisitor::And(bool left_result, bool right_result) {
99-
return left_result && right_result;
112+
Result<std::optional<bool>> IsBoundVisitor::And(const std::optional<bool>& left_result,
113+
const std::optional<bool>& right_result) {
114+
return CombineResults(left_result, right_result);
100115
}
101116

102-
Result<bool> IsBoundVisitor::Or(bool left_result, bool right_result) {
103-
return left_result && right_result;
117+
Result<std::optional<bool>> IsBoundVisitor::Or(const std::optional<bool>& left_result,
118+
const std::optional<bool>& right_result) {
119+
return CombineResults(left_result, right_result);
104120
}
105121

106-
Result<bool> IsBoundVisitor::Predicate(const std::shared_ptr<BoundPredicate>& pred) {
122+
Result<std::optional<bool>> IsBoundVisitor::Predicate(
123+
const std::shared_ptr<BoundPredicate>& pred) {
107124
return true;
108125
}
109126

110-
Result<bool> IsBoundVisitor::Predicate(const std::shared_ptr<UnboundPredicate>& pred) {
127+
Result<std::optional<bool>> IsBoundVisitor::Predicate(
128+
const std::shared_ptr<UnboundPredicate>& pred) {
111129
return false;
112130
}
113131

114-
Result<bool> IsBoundVisitor::Aggregate(const std::shared_ptr<BoundAggregate>& aggregate) {
132+
Result<std::optional<bool>> IsBoundVisitor::Aggregate(
133+
const std::shared_ptr<BoundAggregate>& aggregate) {
115134
return true;
116135
}
117136

118-
Result<bool> IsBoundVisitor::Aggregate(
137+
Result<std::optional<bool>> IsBoundVisitor::Aggregate(
119138
const std::shared_ptr<UnboundAggregate>& aggregate) {
120139
return false;
121140
}

‎src/iceberg/expression/binder.h‎

Lines changed: 17 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
/// Bind an expression to a schema.
2424

2525
#include <functional>
26+
#include <optional>
2627
#include <unordered_set>
2728

2829
#include "iceberg/expression/expression_visitor.h"
@@ -61,19 +62,25 @@ class ICEBERG_EXPORT Binder : public ExpressionVisitor<std::shared_ptr<Expressio
6162
const bool case_sensitive_;
6263
};
6364

64-
class ICEBERG_EXPORT IsBoundVisitor : public ExpressionVisitor<bool> {
65+
class ICEBERG_EXPORT IsBoundVisitor : public ExpressionVisitor<std::optional<bool>> {
6566
public:
6667
static Result<bool> IsBound(const std::shared_ptr<Expression>& expr);
6768

68-
Result<bool> AlwaysTrue() override;
69-
Result<bool> AlwaysFalse() override;
70-
Result<bool> Not(bool child_result) override;
71-
Result<bool> And(bool left_result, bool right_result) override;
72-
Result<bool> Or(bool left_result, bool right_result) override;
73-
Result<bool> Predicate(const std::shared_ptr<BoundPredicate>& pred) override;
74-
Result<bool> Predicate(const std::shared_ptr<UnboundPredicate>& pred) override;
75-
Result<bool> Aggregate(const std::shared_ptr<BoundAggregate>& aggregate) override;
76-
Result<bool> Aggregate(const std::shared_ptr<UnboundAggregate>& aggregate) override;
69+
Result<std::optional<bool>> AlwaysTrue() override;
70+
Result<std::optional<bool>> AlwaysFalse() override;
71+
Result<std::optional<bool>> Not(const std::optional<bool>& child_result) override;
72+
Result<std::optional<bool>> And(const std::optional<bool>& left_result,
73+
const std::optional<bool>& right_result) override;
74+
Result<std::optional<bool>> Or(const std::optional<bool>& left_result,
75+
const std::optional<bool>& right_result) override;
76+
Result<std::optional<bool>> Predicate(
77+
const std::shared_ptr<BoundPredicate>& pred) override;
78+
Result<std::optional<bool>> Predicate(
79+
const std::shared_ptr<UnboundPredicate>& pred) override;
80+
Result<std::optional<bool>> Aggregate(
81+
const std::shared_ptr<BoundAggregate>& aggregate) override;
82+
Result<std::optional<bool>> Aggregate(
83+
const std::shared_ptr<UnboundAggregate>& aggregate) override;
7784
};
7885

7986
using FieldIdsSetRef = std::reference_wrapper<std::unordered_set<int32_t>>;

‎src/iceberg/test/expression_visitor_test.cc‎

Lines changed: 46 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -305,14 +305,15 @@ TEST_F(BinderTest, ErrorNestedUnboundField) {
305305
class IsBoundVisitorTest : public ExpressionVisitorTest {};
306306

307307
TEST_F(IsBoundVisitorTest, Constants) {
308-
// True and False should error out
309308
auto true_expr = Expressions::AlwaysTrue();
310309
auto result_true = IsBoundVisitor::IsBound(true_expr);
311-
EXPECT_THAT(result_true, IsError(ErrorKind::kInvalidExpression));
310+
ASSERT_THAT(result_true, IsOk());
311+
EXPECT_FALSE(*result_true);
312312

313313
auto false_expr = Expressions::AlwaysFalse();
314314
auto result_false = IsBoundVisitor::IsBound(false_expr);
315-
EXPECT_THAT(result_false, IsError(ErrorKind::kInvalidExpression));
315+
ASSERT_THAT(result_false, IsOk());
316+
EXPECT_FALSE(*result_false);
316317
}
317318

318319
TEST_F(IsBoundVisitorTest, UnboundPredicate) {
@@ -322,6 +323,39 @@ TEST_F(IsBoundVisitorTest, UnboundPredicate) {
322323
EXPECT_FALSE(is_bound);
323324
}
324325

326+
TEST_F(IsBoundVisitorTest, AndWithConstants) {
327+
auto unbound = Expressions::Equal("name", Literal::String("Alice"));
328+
ICEBERG_UNWRAP_OR_FAIL(auto bound, Bind(unbound));
329+
330+
// Use Make to keep constants in the expression tree instead of folding them.
331+
ICEBERG_UNWRAP_OR_FAIL(std::shared_ptr<Expression> unbound_and,
332+
And::Make(Expressions::AlwaysTrue(), unbound));
333+
ICEBERG_UNWRAP_OR_FAIL(auto unbound_result, IsBoundVisitor::IsBound(unbound_and));
334+
EXPECT_FALSE(unbound_result);
335+
EXPECT_THAT(Bind(unbound_and), IsOk());
336+
337+
ICEBERG_UNWRAP_OR_FAIL(std::shared_ptr<Expression> bound_and,
338+
And::Make(bound, Expressions::AlwaysTrue()));
339+
ICEBERG_UNWRAP_OR_FAIL(auto bound_result, IsBoundVisitor::IsBound(bound_and));
340+
EXPECT_TRUE(bound_result);
341+
}
342+
343+
TEST_F(IsBoundVisitorTest, OrWithConstants) {
344+
auto unbound = Expressions::Equal("name", Literal::String("Alice"));
345+
ICEBERG_UNWRAP_OR_FAIL(auto bound, Bind(unbound));
346+
347+
ICEBERG_UNWRAP_OR_FAIL(std::shared_ptr<Expression> unbound_or,
348+
Or::Make(Expressions::AlwaysFalse(), unbound));
349+
ICEBERG_UNWRAP_OR_FAIL(auto unbound_result, IsBoundVisitor::IsBound(unbound_or));
350+
EXPECT_FALSE(unbound_result);
351+
EXPECT_THAT(Bind(unbound_or), IsOk());
352+
353+
ICEBERG_UNWRAP_OR_FAIL(std::shared_ptr<Expression> bound_or,
354+
Or::Make(bound, Expressions::AlwaysFalse()));
355+
ICEBERG_UNWRAP_OR_FAIL(auto bound_result, IsBoundVisitor::IsBound(bound_or));
356+
EXPECT_TRUE(bound_result);
357+
}
358+
325359
TEST_F(IsBoundVisitorTest, BoundPredicate) {
326360
// Bound predicates should return true
327361
auto unbound_pred = Expressions::Equal("name", Literal::String("Alice"));
@@ -342,14 +376,14 @@ TEST_F(IsBoundVisitorTest, AndWithBoundChildren) {
342376
}
343377

344378
TEST_F(IsBoundVisitorTest, AndWithUnboundChild) {
345-
// AND with any unbound child should return false
379+
// Mixing bound and unbound predicates is invalid.
346380
auto bound_pred = Expressions::Equal("name", Literal::String("Alice"));
347381
ICEBERG_UNWRAP_OR_FAIL(auto pred1, Bind(bound_pred));
348382
auto pred2 = Expressions::Equal("age", Literal::Int(25)); // unbound
349383
auto mixed_and = Expressions::And(pred1, pred2);
350384

351-
ICEBERG_UNWRAP_OR_FAIL(auto is_bound, IsBoundVisitor::IsBound(mixed_and));
352-
EXPECT_FALSE(is_bound);
385+
EXPECT_THAT(IsBoundVisitor::IsBound(mixed_and),
386+
HasErrorMessage("Found partially bound expression"));
353387
}
354388

355389
TEST_F(IsBoundVisitorTest, OrWithBoundChildren) {
@@ -364,14 +398,14 @@ TEST_F(IsBoundVisitorTest, OrWithBoundChildren) {
364398
}
365399

366400
TEST_F(IsBoundVisitorTest, OrWithUnboundChild) {
367-
// OR with any unbound child should return false
401+
// Mixing bound and unbound predicates is invalid.
368402
auto pred1 = Expressions::IsNull("name"); // unbound
369403
auto bound_pred2 = Expressions::Equal("age", Literal::Int(25));
370404
ICEBERG_UNWRAP_OR_FAIL(auto pred2, Bind(bound_pred2));
371405
auto mixed_or = Expressions::Or(pred1, pred2);
372406

373-
ICEBERG_UNWRAP_OR_FAIL(auto is_bound, IsBoundVisitor::IsBound(mixed_or));
374-
EXPECT_FALSE(is_bound);
407+
EXPECT_THAT(IsBoundVisitor::IsBound(mixed_or),
408+
HasErrorMessage("Found partially bound expression"));
375409
}
376410

377411
TEST_F(IsBoundVisitorTest, NotWithBoundChild) {
@@ -405,15 +439,15 @@ TEST_F(IsBoundVisitorTest, ComplexExpression) {
405439
ICEBERG_UNWRAP_OR_FAIL(auto is_bound, IsBoundVisitor::IsBound(bound_complex));
406440
EXPECT_TRUE(is_bound);
407441

408-
// Complex expression: one unbound should return false
442+
// Mixed binding state is rejected even in a nested expression.
409443
auto unbound_pred = Expressions::Equal("name", Literal::String("Alice"));
410444
ICEBERG_UNWRAP_OR_FAIL(auto bound_pred2, Bind(pred2));
411445
ICEBERG_UNWRAP_OR_FAIL(auto bound_pred3, Bind(pred3));
412446
auto mixed_and = Expressions::And(unbound_pred, bound_pred2);
413447
auto mixed_complex = Expressions::Or(mixed_and, bound_pred3);
414448

415-
ICEBERG_UNWRAP_OR_FAIL(auto is_bound_mixed, IsBoundVisitor::IsBound(mixed_complex));
416-
EXPECT_FALSE(is_bound_mixed);
449+
EXPECT_THAT(IsBoundVisitor::IsBound(mixed_complex),
450+
HasErrorMessage("Found partially bound expression"));
417451
}
418452

419453
class RewriteNotTest : public ExpressionVisitorTest {};

0 commit comments

Comments
 (0)