Skip to content

Commit 2cb6cbd

Browse files
wgtmacwgtmac
committed
address feedback
1 parent dad14c8 commit 2cb6cbd

File tree

5 files changed

+59
-51
lines changed

5 files changed

+59
-51
lines changed

‎src/iceberg/expression/expressions.cc‎

Lines changed: 0 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -25,44 +25,6 @@
2525

2626
namespace iceberg {
2727

28-
// Logical operations
29-
30-
std::shared_ptr<Expression> Expressions::And(std::shared_ptr<Expression> left,
31-
std::shared_ptr<Expression> right) {
32-
if (left->op() == Expression::Operation::kFalse ||
33-
right->op() == Expression::Operation::kFalse) {
34-
return AlwaysFalse();
35-
}
36-
37-
if (left->op() == Expression::Operation::kTrue) {
38-
return right;
39-
}
40-
41-
if (right->op() == Expression::Operation::kTrue) {
42-
return left;
43-
}
44-
45-
return std::make_shared<::iceberg::And>(std::move(left), std::move(right));
46-
}
47-
48-
std::shared_ptr<Expression> Expressions::Or(std::shared_ptr<Expression> left,
49-
std::shared_ptr<Expression> right) {
50-
if (left->op() == Expression::Operation::kTrue ||
51-
right->op() == Expression::Operation::kTrue) {
52-
return AlwaysTrue();
53-
}
54-
55-
if (left->op() == Expression::Operation::kFalse) {
56-
return right;
57-
}
58-
59-
if (right->op() == Expression::Operation::kFalse) {
60-
return left;
61-
}
62-
63-
return std::make_shared<::iceberg::Or>(std::move(left), std::move(right));
64-
}
65-
6628
// Transform functions
6729

6830
std::shared_ptr<UnboundTransform> Expressions::Bucket(std::string name,

‎src/iceberg/expression/expressions.h‎

Lines changed: 47 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -40,12 +40,57 @@ class ICEBERG_EXPORT Expressions {
4040
// Logical operations
4141

4242
/// \brief Create an AND expression.
43+
template <typename... Args>
4344
static std::shared_ptr<Expression> And(std::shared_ptr<Expression> left,
44-
std::shared_ptr<Expression> right);
45+
std::shared_ptr<Expression> right,
46+
Args&&... args)
47+
requires std::conjunction_v<std::is_same<Args, std::shared_ptr<Expression>>...>
48+
{
49+
if constexpr (sizeof...(args) == 0) {
50+
if (left->op() == Expression::Operation::kFalse ||
51+
right->op() == Expression::Operation::kFalse) {
52+
return AlwaysFalse();
53+
}
54+
55+
if (left->op() == Expression::Operation::kTrue) {
56+
return right;
57+
}
58+
59+
if (right->op() == Expression::Operation::kTrue) {
60+
return left;
61+
}
62+
63+
return std::make_shared<::iceberg::And>(std::move(left), std::move(right));
64+
} else {
65+
return And(And(std::move(left), std::move(right)), std::forward<Args>(args)...);
66+
}
67+
}
4568

4669
/// \brief Create an OR expression.
70+
template <typename... Args>
4771
static std::shared_ptr<Expression> Or(std::shared_ptr<Expression> left,
48-
std::shared_ptr<Expression> right);
72+
std::shared_ptr<Expression> right, Args&&... args)
73+
requires std::conjunction_v<std::is_same<Args, std::shared_ptr<Expression>>...>
74+
{
75+
if constexpr (sizeof...(args) == 0) {
76+
if (left->op() == Expression::Operation::kTrue ||
77+
right->op() == Expression::Operation::kTrue) {
78+
return AlwaysTrue();
79+
}
80+
81+
if (left->op() == Expression::Operation::kFalse) {
82+
return right;
83+
}
84+
85+
if (right->op() == Expression::Operation::kFalse) {
86+
return left;
87+
}
88+
89+
return std::make_shared<::iceberg::Or>(std::move(left), std::move(right));
90+
} else {
91+
return Or(Or(std::move(left), std::move(right)), std::forward<Args>(args)...);
92+
}
93+
}
4994

5095
// Transform functions
5196

‎src/iceberg/expression/predicate.cc‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424

2525
#include "iceberg/exception.h"
2626
#include "iceberg/expression/expressions.h"
27+
#include "iceberg/expression/literal.h"
2728
#include "iceberg/result.h"
2829
#include "iceberg/type.h"
2930
#include "iceberg/util/checked_cast.h"
@@ -191,7 +192,9 @@ Result<std::shared_ptr<Expression>> UnboundPredicate<B>::BindLiteralOperation(
191192
values_.size());
192193
}
193194

194-
const auto& literal = values_[0];
195+
ICEBERG_ASSIGN_OR_RAISE(auto literal,
196+
values_[0].CastTo(internal::checked_pointer_cast<PrimitiveType>(
197+
bound_term->type())));
195198

196199
if (literal.IsNull()) {
197200
return InvalidExpression("Invalid value for conversion to type {}: {} ({})",
@@ -226,7 +229,7 @@ Result<std::shared_ptr<Expression>> UnboundPredicate<B>::BindLiteralOperation(
226229

227230
// TODO(gangwu): translate truncate(col) == value to startsWith(value)
228231
return std::make_shared<BoundLiteralPredicate>(BASE::op(), std::move(bound_term),
229-
literal);
232+
std::move(literal));
230233
}
231234

232235
template <typename B>
@@ -370,6 +373,8 @@ BoundSetPredicate::BoundSetPredicate(Expression::Operation op,
370373
std::span<const Literal> literals)
371374
: BoundPredicate(op, std::move(term)) {
372375
for (const auto& literal : literals) {
376+
ICEBERG_DCHECK((*literal.type() == *term_->type()),
377+
"Literal type does not match term type");
373378
value_set_.push_back(literal.value());
374379
}
375380
}

‎src/iceberg/expression/term.cc‎

Lines changed: 4 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -49,17 +49,13 @@ NamedReference::~NamedReference() = default;
4949

5050
Result<std::shared_ptr<BoundReference>> NamedReference::Bind(const Schema& schema,
5151
bool case_sensitive) const {
52-
if (!case_sensitive) {
53-
return NotImplemented("case-insensitive lookup is not implemented");
54-
}
55-
56-
auto field_opt = schema.GetFieldByName(field_name_);
52+
ICEBERG_ASSIGN_OR_RAISE(auto field_opt,
53+
schema.GetFieldByName(field_name_, case_sensitive));
5754
if (!field_opt.has_value()) {
5855
return InvalidExpression("Cannot find field '{}' in struct: {}", field_name_,
5956
schema.ToString());
6057
}
61-
62-
return std::make_shared<BoundReference>(field_opt->get());
58+
return std::make_shared<BoundReference>(field_opt.value().get());
6359
}
6460

6561
std::string NamedReference::ToString() const {
@@ -146,7 +142,7 @@ bool BoundTransform::Equals(const BoundTerm& other) const {
146142

147143
if (transform_->transform_type() == TransformType::kIdentity &&
148144
other.kind() == Term::Kind::kReference) {
149-
return ref_->Equals(other);
145+
return *ref_ == other;
150146
}
151147

152148
return false;

‎test/predicate_test.cc‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -408,7 +408,7 @@ TEST_F(PredicateTest, LiteralConversionEdgeCases) {
408408
ASSERT_THAT(bound_large_result, IsOk());
409409

410410
auto bound_large = bound_large_result.value();
411-
EXPECT_EQ(bound_large->op(), Expression::Operation::kLt);
411+
EXPECT_EQ(bound_large->op(), Expression::Operation::kTrue);
412412
}
413413

414414
TEST_F(PredicateTest, ComplexExpressionCombinations) {

0 commit comments

Comments
 (0)