Skip to content

Commit a0301bd

Browse files
HeartLinkedHeartLinked
committed
fix review
1 parent e977898 commit a0301bd

File tree

5 files changed

+65
-92
lines changed

5 files changed

+65
-92
lines changed

‎src/iceberg/expression/expression.cc‎

Lines changed: 5 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -47,11 +47,8 @@ Result<std::shared_ptr<Expression>> False::Negate() const { return True::Instanc
4747
// And implementation
4848
Result<std::unique_ptr<And>> And::Make(std::shared_ptr<Expression> left,
4949
std::shared_ptr<Expression> right) {
50-
if (!left) {
51-
return InvalidArgument("And expression left operand cannot be null");
52-
}
53-
if (!right) {
54-
return InvalidArgument("And expression right operand cannot be null");
50+
if (!left || !right) [[unlikely]] {
51+
return InvalidExpression("And expression cannot have null children");
5552
}
5653
return std::unique_ptr<And>(new And(std::move(left), std::move(right)));
5754
}
@@ -70,9 +67,7 @@ Result<std::shared_ptr<Expression>> And::Negate() const {
7067
// De Morgan's law: not(A and B) = (not A) or (not B)
7168
ICEBERG_ASSIGN_OR_RAISE(auto left_negated, left_->Negate());
7269
ICEBERG_ASSIGN_OR_RAISE(auto right_negated, right_->Negate());
73-
ICEBERG_ASSIGN_OR_RAISE(auto or_expr,
74-
Or::Make(std::move(left_negated), std::move(right_negated)));
75-
return std::shared_ptr<Expression>(std::move(or_expr));
70+
return Or::Make(std::move(left_negated), std::move(right_negated));
7671
}
7772

7873
bool And::Equals(const Expression& expr) const {
@@ -110,9 +105,7 @@ Result<std::shared_ptr<Expression>> Or::Negate() const {
110105
// De Morgan's law: not(A or B) = (not A) and (not B)
111106
ICEBERG_ASSIGN_OR_RAISE(auto left_negated, left_->Negate());
112107
ICEBERG_ASSIGN_OR_RAISE(auto right_negated, right_->Negate());
113-
ICEBERG_ASSIGN_OR_RAISE(auto and_expr,
114-
And::Make(std::move(left_negated), std::move(right_negated)));
115-
return std::shared_ptr<Expression>(std::move(and_expr));
108+
return And::Make(std::move(left_negated), std::move(right_negated));
116109
}
117110

118111
bool Or::Equals(const Expression& expr) const {
@@ -127,7 +120,7 @@ bool Or::Equals(const Expression& expr) const {
127120
// Not implementation
128121
Result<std::unique_ptr<Not>> Not::Make(std::shared_ptr<Expression> child) {
129122
if (!child) {
130-
return InvalidArgument("Not expression child cannot be null");
123+
return InvalidExpression("Not expression cannot have null child");
131124
}
132125
return std::unique_ptr<Not>(new Not(std::move(child)));
133126
}

‎src/iceberg/expression/expressions.cc‎

Lines changed: 29 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222
#include "iceberg/exception.h"
2323
#include "iceberg/transform.h"
2424
#include "iceberg/type.h"
25+
#include "iceberg/util/macros.h"
2526

2627
namespace iceberg {
2728

@@ -42,71 +43,57 @@ std::shared_ptr<Expression> Expressions::Not(std::shared_ptr<Expression> child)
4243
}
4344

4445
auto result = ::iceberg::Not::Make(std::move(child));
45-
if (!result) {
46-
throw ExpressionError(result.error().message);
47-
}
48-
return {std::move(result.value())};
46+
ICEBERG_ASSIGN_OR_THROW(auto not_expr, std::move(result));
47+
return {std::move(not_expr)};
4948
}
5049

5150
// Transform functions
5251

5352
std::shared_ptr<UnboundTransform> Expressions::Bucket(std::string name,
5453
int32_t num_buckets) {
55-
auto result =
56-
UnboundTransform::Make(Ref(std::move(name)), Transform::Bucket(num_buckets));
57-
if (!result) {
58-
throw ExpressionError(result.error().message);
59-
}
60-
return {std::move(result.value())};
54+
ICEBERG_ASSIGN_OR_THROW(
55+
auto transform,
56+
UnboundTransform::Make(Ref(std::move(name)), Transform::Bucket(num_buckets)));
57+
return {std::move(transform)};
6158
}
6259

6360
std::shared_ptr<UnboundTransform> Expressions::Year(std::string name) {
64-
auto result = UnboundTransform::Make(Ref(std::move(name)), Transform::Year());
65-
if (!result) {
66-
throw ExpressionError(result.error().message);
67-
}
68-
return {std::move(result.value())};
61+
ICEBERG_ASSIGN_OR_THROW(
62+
auto transform, UnboundTransform::Make(Ref(std::move(name)), Transform::Year()));
63+
return {std::move(transform)};
6964
}
7065

7166
std::shared_ptr<UnboundTransform> Expressions::Month(std::string name) {
72-
auto result = UnboundTransform::Make(Ref(std::move(name)), Transform::Month());
73-
if (!result) {
74-
throw ExpressionError(result.error().message);
75-
}
76-
return {std::move(result.value())};
67+
ICEBERG_ASSIGN_OR_THROW(
68+
auto transform, UnboundTransform::Make(Ref(std::move(name)), Transform::Month()));
69+
return {std::move(transform)};
7770
}
7871

7972
std::shared_ptr<UnboundTransform> Expressions::Day(std::string name) {
80-
auto result = UnboundTransform::Make(Ref(std::move(name)), Transform::Day());
81-
if (!result) {
82-
throw ExpressionError(result.error().message);
83-
}
84-
return {std::move(result.value())};
73+
ICEBERG_ASSIGN_OR_THROW(auto transform,
74+
UnboundTransform::Make(Ref(std::move(name)), Transform::Day()));
75+
return {std::move(transform)};
8576
}
8677

8778
std::shared_ptr<UnboundTransform> Expressions::Hour(std::string name) {
88-
auto result = UnboundTransform::Make(Ref(std::move(name)), Transform::Hour());
89-
if (!result) {
90-
throw ExpressionError(result.error().message);
91-
}
92-
return {std::move(result.value())};
79+
ICEBERG_ASSIGN_OR_THROW(
80+
auto transform, UnboundTransform::Make(Ref(std::move(name)), Transform::Hour()));
81+
return {std::move(transform)};
9382
}
9483

9584
std::shared_ptr<UnboundTransform> Expressions::Truncate(std::string name, int32_t width) {
96-
auto result = UnboundTransform::Make(Ref(std::move(name)), Transform::Truncate(width));
97-
if (!result) {
98-
throw ExpressionError(result.error().message);
99-
}
100-
return {std::move(result.value())};
85+
ICEBERG_ASSIGN_OR_THROW(
86+
auto transform,
87+
UnboundTransform::Make(Ref(std::move(name)), Transform::Truncate(width)));
88+
return {std::move(transform)};
10189
}
10290

10391
std::shared_ptr<UnboundTransform> Expressions::Transform(
10492
std::string name, std::shared_ptr<::iceberg::Transform> transform) {
105-
auto result = UnboundTransform::Make(Ref(std::move(name)), std::move(transform));
106-
if (!result) {
107-
throw ExpressionError(result.error().message);
108-
}
109-
return {std::move(result.value())};
93+
ICEBERG_ASSIGN_OR_THROW(
94+
auto unbound_transform,
95+
UnboundTransform::Make(Ref(std::move(name)), std::move(transform)));
96+
return {std::move(unbound_transform)};
11097
}
11198

11299
// Template implementations for unary predicates
@@ -358,11 +345,8 @@ std::shared_ptr<False> Expressions::AlwaysFalse() { return False::Instance(); }
358345
// Utilities
359346

360347
std::shared_ptr<NamedReference> Expressions::Ref(std::string name) {
361-
auto result = NamedReference::Make(std::move(name));
362-
if (!result) {
363-
throw ExpressionError(result.error().message);
364-
}
365-
return {std::move(result.value())};
348+
ICEBERG_ASSIGN_OR_THROW(auto ref, NamedReference::Make(std::move(name)));
349+
return {std::move(ref)};
366350
}
367351

368352
Literal Expressions::Lit(Literal::Value value, std::shared_ptr<PrimitiveType> type) {

‎src/iceberg/expression/expressions.h‎

Lines changed: 8 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -32,12 +32,13 @@
3232
#include "iceberg/expression/predicate.h"
3333
#include "iceberg/expression/term.h"
3434
#include "iceberg/iceberg_export.h"
35+
#include "iceberg/util/macros.h"
3536

3637
namespace iceberg {
3738

38-
/// \brief Factory methods for quickly creating expressions. Unlike most APIs in this
39-
/// codebase that return Result<T>, these factory methods throw ExpressionError exceptions
40-
/// on failures to provide a more convenient API for expression construction.
39+
/// \brief Fluent APIs to create expressions.
40+
///
41+
/// \throw `ExpressionError` for invalid expression.
4142
class ICEBERG_EXPORT Expressions {
4243
public:
4344
// Logical operations
@@ -64,10 +65,8 @@ class ICEBERG_EXPORT Expressions {
6465
}
6566

6667
auto result = ::iceberg::And::Make(std::move(left), std::move(right));
67-
if (!result) {
68-
throw ExpressionError(result.error().message);
69-
}
70-
return {std::move(result.value())};
68+
ICEBERG_ASSIGN_OR_THROW(auto and_expr, std::move(result));
69+
return {std::move(and_expr)};
7170
} else {
7271
return And(And(std::move(left), std::move(right)), std::forward<Args>(args)...);
7372
}
@@ -94,10 +93,8 @@ class ICEBERG_EXPORT Expressions {
9493
}
9594

9695
auto result = ::iceberg::Or::Make(std::move(left), std::move(right));
97-
if (!result) {
98-
throw ExpressionError(result.error().message);
99-
}
100-
return {std::move(result.value())};
96+
ICEBERG_ASSIGN_OR_THROW(auto or_expr, std::move(result));
97+
return {std::move(or_expr)};
10198
} else {
10299
return Or(Or(std::move(left), std::move(right)), std::forward<Args>(args)...);
103100
}

‎src/iceberg/expression/term.cc‎

Lines changed: 12 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -43,8 +43,8 @@ Result<std::shared_ptr<B>> Unbound<B>::Bind(const Schema& schema) const {
4343

4444
// NamedReference implementation
4545
Result<std::unique_ptr<NamedReference>> NamedReference::Make(std::string field_name) {
46-
if (field_name.empty()) {
47-
return InvalidArgument("NamedReference field name cannot be empty");
46+
if (field_name.empty()) [[unlikely]] {
47+
return InvalidExpression("NamedReference field name cannot be empty");
4848
}
4949
return std::unique_ptr<NamedReference>(new NamedReference(std::move(field_name)));
5050
}
@@ -60,12 +60,11 @@ Result<std::shared_ptr<BoundReference>> NamedReference::Bind(const Schema& schem
6060
bool case_sensitive) const {
6161
ICEBERG_ASSIGN_OR_RAISE(auto field_opt,
6262
schema.GetFieldByName(field_name_, case_sensitive));
63-
if (!field_opt.has_value()) {
63+
if (!field_opt.has_value()) [[unlikely]] {
6464
return InvalidExpression("Cannot find field '{}' in struct: {}", field_name_,
6565
schema.ToString());
6666
}
67-
ICEBERG_ASSIGN_OR_RAISE(auto bound_ref, BoundReference::Make(field_opt.value().get()));
68-
return std::shared_ptr<BoundReference>(std::move(bound_ref));
67+
return BoundReference::Make(field_opt.value().get());
6968
}
7069

7170
std::string NamedReference::ToString() const {
@@ -74,8 +73,8 @@ std::string NamedReference::ToString() const {
7473

7574
// BoundReference implementation
7675
Result<std::unique_ptr<BoundReference>> BoundReference::Make(SchemaField field) {
77-
if (!field.type()) {
78-
return InvalidArgument("BoundReference field type cannot be null");
76+
if (!field.type()) [[unlikely]] {
77+
return InvalidExpression("BoundReference field type cannot be null");
7978
}
8079
return std::unique_ptr<BoundReference>(new BoundReference(std::move(field)));
8180
}
@@ -108,11 +107,8 @@ bool BoundReference::Equals(const BoundTerm& other) const {
108107
// UnboundTransform implementation
109108
Result<std::unique_ptr<UnboundTransform>> UnboundTransform::Make(
110109
std::shared_ptr<NamedReference> ref, std::shared_ptr<Transform> transform) {
111-
if (!ref) {
112-
return InvalidArgument("UnboundTransform reference cannot be null");
113-
}
114-
if (!transform) {
115-
return InvalidArgument("UnboundTransform transform cannot be null");
110+
if (!ref || !transform) [[unlikely]] {
111+
return InvalidExpression("UnboundTransform cannot have null children");
116112
}
117113
return std::unique_ptr<UnboundTransform>(
118114
new UnboundTransform(std::move(ref), std::move(transform)));
@@ -135,24 +131,16 @@ Result<std::shared_ptr<BoundTransform>> UnboundTransform::Bind(
135131
const Schema& schema, bool case_sensitive) const {
136132
ICEBERG_ASSIGN_OR_RAISE(auto bound_ref, ref_->Bind(schema, case_sensitive));
137133
ICEBERG_ASSIGN_OR_RAISE(auto transform_func, transform_->Bind(bound_ref->type()));
138-
ICEBERG_ASSIGN_OR_RAISE(
139-
auto bound_transform,
140-
BoundTransform::Make(std::move(bound_ref), transform_, std::move(transform_func)));
141-
return std::shared_ptr<BoundTransform>(std::move(bound_transform));
134+
return BoundTransform::Make(std::move(bound_ref), transform_,
135+
std::move(transform_func));
142136
}
143137

144138
// BoundTransform implementation
145139
Result<std::unique_ptr<BoundTransform>> BoundTransform::Make(
146140
std::shared_ptr<BoundReference> ref, std::shared_ptr<Transform> transform,
147141
std::shared_ptr<TransformFunction> transform_func) {
148-
if (!ref) {
149-
return InvalidArgument("BoundTransform reference cannot be null");
150-
}
151-
if (!transform) {
152-
return InvalidArgument("BoundTransform transform cannot be null");
153-
}
154-
if (!transform_func) {
155-
return InvalidArgument("BoundTransform transform function cannot be null");
142+
if (!ref || !transform || !transform_func) [[unlikely]] {
143+
return InvalidExpression("BoundTransform cannot have null children");
156144
}
157145
return std::unique_ptr<BoundTransform>(new BoundTransform(
158146
std::move(ref), std::move(transform), std::move(transform_func)));

‎src/iceberg/util/macros.h‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,4 +39,15 @@
3939
ICEBERG_ASSIGN_OR_RAISE_IMPL(ICEBERG_ASSIGN_OR_RAISE_NAME(result_, __COUNTER__), lhs, \
4040
rexpr)
4141

42+
#define ICEBERG_ASSIGN_OR_THROW_IMPL(result_name, lhs, rexpr) \
43+
auto&& result_name = (rexpr); \
44+
if (!result_name) [[unlikely]] { \
45+
throw iceberg::ExpressionError(result_name.error().message); \
46+
} \
47+
lhs = std::move(result_name.value());
48+
49+
#define ICEBERG_ASSIGN_OR_THROW(lhs, rexpr) \
50+
ICEBERG_ASSIGN_OR_THROW_IMPL(ICEBERG_ASSIGN_OR_RAISE_NAME(result_, __COUNTER__), lhs, \
51+
rexpr)
52+
4253
#define ICEBERG_DCHECK(expr, message) assert((expr) && (message))

0 commit comments

Comments
 (0)