wgtmac commented on code in PR #301:
URL: https://github.com/apache/iceberg-cpp/pull/301#discussion_r2509186364
##########
src/iceberg/expression/predicate.cc:
##########
@@ -516,7 +627,9 @@ Result<bool> BoundSetPredicate::Test(const Literal& value)
const {
Result<std::shared_ptr<Expression>> BoundSetPredicate::Negate() const {
ICEBERG_ASSIGN_OR_RAISE(auto negated_op, ::iceberg::Negate(op()));
- return std::make_shared<BoundSetPredicate>(negated_op, term_, value_set_);
+ ICEBERG_ASSIGN_OR_RAISE(auto negated_pred,
+ BoundSetPredicate::Make(negated_op, term_,
value_set_));
+ return std::shared_ptr<Expression>(std::move(negated_pred));
Review Comment:
```suggestion
return BoundSetPredicate::Make(negated_op, term_, value_set_);
```
##########
src/iceberg/expression/predicate.cc:
##########
@@ -401,7 +487,9 @@ Result<bool> BoundLiteralPredicate::Test(const Literal&
value) const {
Result<std::shared_ptr<Expression>> BoundLiteralPredicate::Negate() const {
ICEBERG_ASSIGN_OR_RAISE(auto negated_op, ::iceberg::Negate(op()));
- return std::make_shared<BoundLiteralPredicate>(negated_op, term_, literal_);
+ ICEBERG_ASSIGN_OR_RAISE(auto negated_pred,
+ BoundLiteralPredicate::Make(negated_op, term_,
literal_));
+ return std::shared_ptr<Expression>(std::move(negated_pred));
Review Comment:
```suggestion
return BoundLiteralPredicate::Make(negated_op, term_, literal_);
```
##########
src/iceberg/expression/predicate.cc:
##########
@@ -337,7 +410,9 @@ Result<bool> BoundUnaryPredicate::Test(const Literal&
literal) const {
Result<std::shared_ptr<Expression>> BoundUnaryPredicate::Negate() const {
ICEBERG_ASSIGN_OR_RAISE(auto negated_op, ::iceberg::Negate(op()));
- return std::make_shared<BoundUnaryPredicate>(negated_op, term_);
+ ICEBERG_ASSIGN_OR_RAISE(auto negated_pred,
+ BoundUnaryPredicate::Make(negated_op, term_));
+ return std::shared_ptr<Expression>(std::move(negated_pred));
Review Comment:
```suggestion
return BoundUnaryPredicate::Make(negated_op, term_);
```
##########
src/iceberg/expression/predicate.cc:
##########
@@ -117,7 +156,9 @@ std::string UnboundPredicate<B>::ToString() const {
template <typename B>
Result<std::shared_ptr<Expression>> UnboundPredicate<B>::Negate() const {
ICEBERG_ASSIGN_OR_RAISE(auto negated_op, ::iceberg::Negate(BASE::op()));
- return std::make_shared<UnboundPredicate>(negated_op, BASE::term(), values_);
+ ICEBERG_ASSIGN_OR_RAISE(auto negated_pred,
+ UnboundPredicate::Make(negated_op, BASE::term(),
values_));
+ return std::shared_ptr<Expression>(std::move(negated_pred));
Review Comment:
```suggestion
return UnboundPredicate::Make(negated_op, BASE::term(), values_);
```
##########
src/iceberg/expression/predicate.cc:
##########
@@ -285,25 +337,35 @@ Result<std::shared_ptr<Expression>>
UnboundPredicate<B>::BindInOperation(
if (converted_literals.size() == 1) {
const auto& single_literal = converted_literals[0];
switch (BASE::op()) {
- case Expression::Operation::kIn:
- return std::make_shared<BoundLiteralPredicate>(
- Expression::Operation::kEq, std::move(bound_term), single_literal);
- case Expression::Operation::kNotIn:
- return std::make_shared<BoundLiteralPredicate>(
- Expression::Operation::kNotEq, std::move(bound_term),
single_literal);
+ case Expression::Operation::kIn: {
+ ICEBERG_ASSIGN_OR_RAISE(auto pred, BoundLiteralPredicate::Make(
+ Expression::Operation::kEq,
+ std::move(bound_term),
single_literal));
+ return std::shared_ptr<Expression>(std::move(pred));
+ }
+ case Expression::Operation::kNotIn: {
+ ICEBERG_ASSIGN_OR_RAISE(auto pred, BoundLiteralPredicate::Make(
+ Expression::Operation::kNotEq,
+ std::move(bound_term),
single_literal));
+ return std::shared_ptr<Expression>(std::move(pred));
+ }
default:
return InvalidExpression("Operation must be IN or NOT_IN");
}
}
// Multiple literals - create a set predicate
- return std::make_shared<BoundSetPredicate>(
- BASE::op(), std::move(bound_term), std::span<const
Literal>(converted_literals));
+ ICEBERG_ASSIGN_OR_RAISE(
+ auto pred, BoundSetPredicate::Make(BASE::op(), std::move(bound_term),
+ std::span<const
Literal>(converted_literals)));
+ return std::shared_ptr<Expression>(std::move(pred));
Review Comment:
```suggestion
return BoundSetPredicate::Make(
BASE::op(), std::move(bound_term), std::span<const
Literal>(converted_literals));
```
##########
src/iceberg/expression/predicate.cc:
##########
@@ -401,7 +487,9 @@ Result<bool> BoundLiteralPredicate::Test(const Literal&
value) const {
Result<std::shared_ptr<Expression>> BoundLiteralPredicate::Negate() const {
ICEBERG_ASSIGN_OR_RAISE(auto negated_op, ::iceberg::Negate(op()));
- return std::make_shared<BoundLiteralPredicate>(negated_op, term_, literal_);
+ ICEBERG_ASSIGN_OR_RAISE(auto negated_pred,
Review Comment:
ditto
##########
src/iceberg/expression/predicate.cc:
##########
@@ -247,8 +297,10 @@ Result<std::shared_ptr<Expression>>
UnboundPredicate<B>::BindLiteralOperation(
}
// TODO(gangwu): translate truncate(col) == value to startsWith(value)
- return std::make_shared<BoundLiteralPredicate>(BASE::op(),
std::move(bound_term),
- std::move(literal));
+ ICEBERG_ASSIGN_OR_RAISE(
Review Comment:
ditto
##########
src/iceberg/expression/predicate.cc:
##########
@@ -169,26 +210,35 @@ template <typename B>
Result<std::shared_ptr<Expression>> UnboundPredicate<B>::BindUnaryOperation(
std::shared_ptr<B> bound_term) const {
switch (BASE::op()) {
- case Expression::Operation::kIsNull:
+ case Expression::Operation::kIsNull: {
if (!bound_term->MayProduceNull()) {
return Expressions::AlwaysFalse();
}
// TODO(gangwu): deal with UnknownType
- return
std::make_shared<BoundUnaryPredicate>(Expression::Operation::kIsNull,
- std::move(bound_term));
- case Expression::Operation::kNotNull:
+ ICEBERG_ASSIGN_OR_RAISE(auto pred,
+
BoundUnaryPredicate::Make(Expression::Operation::kIsNull,
+
std::move(bound_term)));
+ return std::shared_ptr<Expression>(std::move(pred));
+ }
+ case Expression::Operation::kNotNull: {
if (!bound_term->MayProduceNull()) {
return Expressions::AlwaysTrue();
}
- return
std::make_shared<BoundUnaryPredicate>(Expression::Operation::kNotNull,
- std::move(bound_term));
+ ICEBERG_ASSIGN_OR_RAISE(auto pred,
+
BoundUnaryPredicate::Make(Expression::Operation::kNotNull,
+
std::move(bound_term)));
+ return std::shared_ptr<Expression>(std::move(pred));
+ }
case Expression::Operation::kIsNan:
- case Expression::Operation::kNotNan:
+ case Expression::Operation::kNotNan: {
if (!IsFloatingType(bound_term->type()->type_id())) {
return InvalidExpression("{} cannot be used with a non-floating-point
column",
BASE::op());
}
- return std::make_shared<BoundUnaryPredicate>(BASE::op(),
std::move(bound_term));
+ ICEBERG_ASSIGN_OR_RAISE(
Review Comment:
ditto
##########
src/iceberg/expression/predicate.cc:
##########
@@ -169,26 +210,35 @@ template <typename B>
Result<std::shared_ptr<Expression>> UnboundPredicate<B>::BindUnaryOperation(
std::shared_ptr<B> bound_term) const {
switch (BASE::op()) {
- case Expression::Operation::kIsNull:
+ case Expression::Operation::kIsNull: {
if (!bound_term->MayProduceNull()) {
return Expressions::AlwaysFalse();
}
// TODO(gangwu): deal with UnknownType
- return
std::make_shared<BoundUnaryPredicate>(Expression::Operation::kIsNull,
- std::move(bound_term));
- case Expression::Operation::kNotNull:
+ ICEBERG_ASSIGN_OR_RAISE(auto pred,
Review Comment:
ditto
##########
src/iceberg/expression/predicate.cc:
##########
@@ -169,26 +210,35 @@ template <typename B>
Result<std::shared_ptr<Expression>> UnboundPredicate<B>::BindUnaryOperation(
std::shared_ptr<B> bound_term) const {
switch (BASE::op()) {
- case Expression::Operation::kIsNull:
+ case Expression::Operation::kIsNull: {
if (!bound_term->MayProduceNull()) {
return Expressions::AlwaysFalse();
}
// TODO(gangwu): deal with UnknownType
- return
std::make_shared<BoundUnaryPredicate>(Expression::Operation::kIsNull,
- std::move(bound_term));
- case Expression::Operation::kNotNull:
+ ICEBERG_ASSIGN_OR_RAISE(auto pred,
+
BoundUnaryPredicate::Make(Expression::Operation::kIsNull,
+
std::move(bound_term)));
+ return std::shared_ptr<Expression>(std::move(pred));
+ }
+ case Expression::Operation::kNotNull: {
if (!bound_term->MayProduceNull()) {
return Expressions::AlwaysTrue();
}
- return
std::make_shared<BoundUnaryPredicate>(Expression::Operation::kNotNull,
- std::move(bound_term));
+ ICEBERG_ASSIGN_OR_RAISE(auto pred,
Review Comment:
ditto
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
