wgtmac commented on code in PR #283:
URL: https://github.com/apache/iceberg-cpp/pull/283#discussion_r2484221819
##########
src/iceberg/expression/expression.cc:
##########
@@ -56,7 +70,9 @@ Result<std::shared_ptr<Expression>> And::Negate() const {
// De Morgan's law: not(A and B) = (not A) or (not B)
ICEBERG_ASSIGN_OR_RAISE(auto left_negated, left_->Negate());
ICEBERG_ASSIGN_OR_RAISE(auto right_negated, right_->Negate());
- return std::make_shared<Or>(std::move(left_negated),
std::move(right_negated));
+ ICEBERG_ASSIGN_OR_RAISE(auto or_expr,
+ Or::Make(std::move(left_negated),
std::move(right_negated)));
+ return std::shared_ptr<Expression>(std::move(or_expr));
Review Comment:
```suggestion
return Or::Make(std::move(left_negated), std::move(right_negated));
```
##########
src/iceberg/expression/expressions.h:
##########
@@ -27,14 +27,17 @@
#include <string>
#include <vector>
+#include "iceberg/exception.h"
#include "iceberg/expression/literal.h"
#include "iceberg/expression/predicate.h"
#include "iceberg/expression/term.h"
#include "iceberg/iceberg_export.h"
namespace iceberg {
-/// \brief Factory methods for creating expressions.
+/// \brief Factory methods for quickly creating expressions. Unlike most APIs
in this
+/// codebase that return Result<T>, these factory methods throw
ExpressionError exceptions
+/// on failures to provide a more convenient API for expression construction.
Review Comment:
```suggestion
/// \brief Fluent APIs to create expressions.
///
/// \throw `ExpressionError` for invalid expression.
```
##########
src/iceberg/expression/term.cc:
##########
@@ -55,15 +64,25 @@ Result<std::shared_ptr<BoundReference>>
NamedReference::Bind(const Schema& schem
return InvalidExpression("Cannot find field '{}' in struct: {}",
field_name_,
schema.ToString());
}
- return std::make_shared<BoundReference>(field_opt.value().get());
+ ICEBERG_ASSIGN_OR_RAISE(auto bound_ref,
BoundReference::Make(field_opt.value().get()));
+ return std::shared_ptr<BoundReference>(std::move(bound_ref));
}
std::string NamedReference::ToString() const {
return std::format("ref(name=\"{}\")", field_name_);
}
// BoundReference implementation
-BoundReference::BoundReference(SchemaField field) : field_(std::move(field)) {}
+Result<std::unique_ptr<BoundReference>> BoundReference::Make(SchemaField
field) {
+ if (!field.type()) {
+ return InvalidArgument("BoundReference field type cannot be null");
Review Comment:
```suggestion
return InvalidExpression("BoundReference field type cannot be null");
```
##########
src/iceberg/expression/term.cc:
##########
@@ -87,9 +106,24 @@ bool BoundReference::Equals(const BoundTerm& other) const {
}
// UnboundTransform implementation
+Result<std::unique_ptr<UnboundTransform>> UnboundTransform::Make(
+ std::shared_ptr<NamedReference> ref, std::shared_ptr<Transform> transform)
{
+ if (!ref) {
+ return InvalidArgument("UnboundTransform reference cannot be null");
Review Comment:
ditto
##########
src/iceberg/expression/expressions.cc:
##########
@@ -41,41 +41,72 @@ std::shared_ptr<Expression>
Expressions::Not(std::shared_ptr<Expression> child)
return not_expr.child();
}
- return std::make_shared<::iceberg::Not>(std::move(child));
+ auto result = ::iceberg::Not::Make(std::move(child));
Review Comment:
Both expressions.h and expressions.cc have a lot of duplicate code for
exception handling. Can we add a new macro `ICEBERG_ASSIGN_OR_THROW` similar to
`ICEBERG_ASSIGN_OR_RAISE`?
##########
src/iceberg/expression/expression.cc:
##########
@@ -45,8 +45,22 @@ const std::shared_ptr<False>& False::Instance() {
Result<std::shared_ptr<Expression>> False::Negate() const { return
True::Instance(); }
// And implementation
+Result<std::unique_ptr<And>> And::Make(std::shared_ptr<Expression> left,
+ std::shared_ptr<Expression> right) {
+ if (!left) {
+ return InvalidArgument("And expression left operand cannot be null");
Review Comment:
```suggestion
return InvalidExpression("And expression left operand cannot be null");
```
##########
src/iceberg/expression/expression.cc:
##########
@@ -80,7 +110,9 @@ Result<std::shared_ptr<Expression>> Or::Negate() const {
// De Morgan's law: not(A or B) = (not A) and (not B)
ICEBERG_ASSIGN_OR_RAISE(auto left_negated, left_->Negate());
ICEBERG_ASSIGN_OR_RAISE(auto right_negated, right_->Negate());
- return std::make_shared<And>(std::move(left_negated),
std::move(right_negated));
+ ICEBERG_ASSIGN_OR_RAISE(auto and_expr,
+ And::Make(std::move(left_negated),
std::move(right_negated)));
+ return std::shared_ptr<Expression>(std::move(and_expr));
Review Comment:
```suggestion
return And::Make(std::move(left_negated), std::move(right_negated));
```
##########
src/iceberg/expression/term.cc:
##########
@@ -42,8 +42,17 @@ Result<std::shared_ptr<B>> Unbound<B>::Bind(const Schema&
schema) const {
}
// NamedReference implementation
+Result<std::unique_ptr<NamedReference>> NamedReference::Make(std::string
field_name) {
+ if (field_name.empty()) {
+ return InvalidArgument("NamedReference field name cannot be empty");
Review Comment:
Same comments as those in expressions.cc
##########
src/iceberg/expression/expression.cc:
##########
@@ -93,7 +125,16 @@ bool Or::Equals(const Expression& expr) const {
}
// Not implementation
-Not::Not(std::shared_ptr<Expression> child) : child_(std::move(child)) {}
+Result<std::unique_ptr<Not>> Not::Make(std::shared_ptr<Expression> child) {
+ if (!child) {
+ return InvalidArgument("Not expression child cannot be null");
Review Comment:
```suggestion
return InvalidExpression("Not expression cannot have null child");
```
##########
src/iceberg/expression/expression.cc:
##########
@@ -45,8 +45,22 @@ const std::shared_ptr<False>& False::Instance() {
Result<std::shared_ptr<Expression>> False::Negate() const { return
True::Instance(); }
// And implementation
+Result<std::unique_ptr<And>> And::Make(std::shared_ptr<Expression> left,
+ std::shared_ptr<Expression> right) {
+ if (!left) {
+ return InvalidArgument("And expression left operand cannot be null");
Review Comment:
Same for other returning errors.
##########
src/iceberg/expression/term.cc:
##########
@@ -55,15 +64,25 @@ Result<std::shared_ptr<BoundReference>>
NamedReference::Bind(const Schema& schem
return InvalidExpression("Cannot find field '{}' in struct: {}",
field_name_,
schema.ToString());
}
- return std::make_shared<BoundReference>(field_opt.value().get());
+ ICEBERG_ASSIGN_OR_RAISE(auto bound_ref,
BoundReference::Make(field_opt.value().get()));
+ return std::shared_ptr<BoundReference>(std::move(bound_ref));
Review Comment:
```suggestion
return BoundReference::Make(field_opt.value().get());
```
##########
src/iceberg/expression/expression.cc:
##########
@@ -45,8 +45,22 @@ const std::shared_ptr<False>& False::Instance() {
Result<std::shared_ptr<Expression>> False::Negate() const { return
True::Instance(); }
// And implementation
+Result<std::unique_ptr<And>> And::Make(std::shared_ptr<Expression> left,
+ std::shared_ptr<Expression> right) {
+ if (!left) {
+ return InvalidArgument("And expression left operand cannot be null");
Review Comment:
nit: I think we can simplify it as below
```cpp
if (!left || !right) [[unlikely]] {
return InvalidExpression("And expression cannot have null children");
}
```
##########
src/iceberg/expression/term.cc:
##########
@@ -101,17 +135,40 @@ Result<std::shared_ptr<BoundTransform>>
UnboundTransform::Bind(
const Schema& schema, bool case_sensitive) const {
ICEBERG_ASSIGN_OR_RAISE(auto bound_ref, ref_->Bind(schema, case_sensitive));
ICEBERG_ASSIGN_OR_RAISE(auto transform_func,
transform_->Bind(bound_ref->type()));
- return std::make_shared<BoundTransform>(std::move(bound_ref), transform_,
- std::move(transform_func));
+ ICEBERG_ASSIGN_OR_RAISE(
+ auto bound_transform,
+ BoundTransform::Make(std::move(bound_ref), transform_,
std::move(transform_func)));
+ return std::shared_ptr<BoundTransform>(std::move(bound_transform));
Review Comment:
```suggestion
return BoundTransform::Make(std::move(bound_ref), transform_,
std::move(transform_func));
```
--
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]
