This is an automated email from the ASF dual-hosted git repository.
gangwu pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/iceberg-cpp.git
The following commit(s) were added to refs/heads/main by this push:
new 018df25 refactor: use fluent api in the expressions factory and throw
`ExpressionError` (#283)
018df25 is described below
commit 018df25c1e85b19b9102193dd9880eff4e295c2b
Author: Li Feiyang <[email protected]>
AuthorDate: Tue Nov 4 14:08:42 2025 +0800
refactor: use fluent api in the expressions factory and throw
`ExpressionError` (#283)
---
src/iceberg/exception.h | 6 +++
src/iceberg/expression/expression.cc | 41 ++++++++++++---
src/iceberg/expression/expression.h | 21 +++++---
src/iceberg/expression/expressions.cc | 42 ++++++++++-----
src/iceberg/expression/expressions.h | 14 +++--
src/iceberg/expression/term.cc | 63 ++++++++++++++++++++---
src/iceberg/expression/term.h | 30 ++++++-----
src/iceberg/schema_field.cc | 10 ++++
src/iceberg/schema_field.h | 3 ++
src/iceberg/test/expression_test.cc | 96 ++++++++++++++++++++++++++---------
src/iceberg/test/predicate_test.cc | 12 ++---
src/iceberg/util/macros.h | 9 +++-
12 files changed, 268 insertions(+), 79 deletions(-)
diff --git a/src/iceberg/exception.h b/src/iceberg/exception.h
index c5160bc..bbb9c22 100644
--- a/src/iceberg/exception.h
+++ b/src/iceberg/exception.h
@@ -38,6 +38,12 @@ class ICEBERG_EXPORT IcebergError : public
std::runtime_error {
explicit IcebergError(const std::string& what) : std::runtime_error(what) {}
};
+/// \brief Exception thrown when expression construction fails.
+class ICEBERG_EXPORT ExpressionError : public IcebergError {
+ public:
+ explicit ExpressionError(const std::string& what) : IcebergError(what) {}
+};
+
#define ICEBERG_CHECK(condition, ...) \
do { \
if (!(condition)) [[unlikely]] { \
diff --git a/src/iceberg/expression/expression.cc
b/src/iceberg/expression/expression.cc
index 070d5fe..69446a8 100644
--- a/src/iceberg/expression/expression.cc
+++ b/src/iceberg/expression/expression.cc
@@ -45,8 +45,18 @@ 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 || !right) [[unlikely]] {
+ return InvalidExpression("And expression cannot have null children");
+ }
+ return std::unique_ptr<And>(new And(std::move(left), std::move(right)));
+}
+
And::And(std::shared_ptr<Expression> left, std::shared_ptr<Expression> right)
- : left_(std::move(left)), right_(std::move(right)) {}
+ : left_(std::move(left)), right_(std::move(right)) {
+ ICEBERG_DCHECK(left_ && right_, "And cannot have null children");
+}
std::string And::ToString() const {
return std::format("({} and {})", left_->ToString(), right_->ToString());
@@ -56,7 +66,7 @@ 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));
+ return Or::Make(std::move(left_negated), std::move(right_negated));
}
bool And::Equals(const Expression& expr) const {
@@ -69,8 +79,18 @@ bool And::Equals(const Expression& expr) const {
}
// Or implementation
+Result<std::unique_ptr<Or>> Or::Make(std::shared_ptr<Expression> left,
+ std::shared_ptr<Expression> right) {
+ if (!left || !right) [[unlikely]] {
+ return InvalidExpression("Or cannot have null children");
+ }
+ return std::unique_ptr<Or>(new Or(std::move(left), std::move(right)));
+}
+
Or::Or(std::shared_ptr<Expression> left, std::shared_ptr<Expression> right)
- : left_(std::move(left)), right_(std::move(right)) {}
+ : left_(std::move(left)), right_(std::move(right)) {
+ ICEBERG_DCHECK(left_ && right_, "Or cannot have null children");
+}
std::string Or::ToString() const {
return std::format("({} or {})", left_->ToString(), right_->ToString());
@@ -80,7 +100,7 @@ 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));
+ return And::Make(std::move(left_negated), std::move(right_negated));
}
bool Or::Equals(const Expression& expr) const {
@@ -93,7 +113,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) [[unlikely]] {
+ return InvalidExpression("Not expression cannot have null child");
+ }
+ return std::unique_ptr<Not>(new Not(std::move(child)));
+}
+
+Not::Not(std::shared_ptr<Expression> child) : child_(std::move(child)) {
+ ICEBERG_DCHECK(child_ != nullptr, "Not expression cannot have null child");
+}
std::string Not::ToString() const { return std::format("not({})",
child_->ToString()); }
@@ -199,7 +228,7 @@ Result<Expression::Operation> Negate(Expression::Operation
op) {
case Expression::Operation::kMax:
case Expression::Operation::kMin:
case Expression::Operation::kCount:
- return InvalidArgument("No negation for operation: {}", op);
+ return InvalidExpression("No negation for operation: {}", op);
}
std::unreachable();
}
diff --git a/src/iceberg/expression/expression.h
b/src/iceberg/expression/expression.h
index 9a80522..931e35c 100644
--- a/src/iceberg/expression/expression.h
+++ b/src/iceberg/expression/expression.h
@@ -130,11 +130,12 @@ class ICEBERG_EXPORT False : public Expression {
/// evaluate to true.
class ICEBERG_EXPORT And : public Expression {
public:
- /// \brief Constructs an And expression from two sub-expressions.
+ /// \brief Creates an And expression from two sub-expressions.
///
/// \param left The left operand of the AND expression
/// \param right The right operand of the AND expression
- And(std::shared_ptr<Expression> left, std::shared_ptr<Expression> right);
+ static Result<std::unique_ptr<And>> Make(std::shared_ptr<Expression> left,
+ std::shared_ptr<Expression> right);
/// \brief Returns the left operand of the AND expression.
///
@@ -155,6 +156,8 @@ class ICEBERG_EXPORT And : public Expression {
bool Equals(const Expression& other) const override;
private:
+ And(std::shared_ptr<Expression> left, std::shared_ptr<Expression> right);
+
std::shared_ptr<Expression> left_;
std::shared_ptr<Expression> right_;
};
@@ -165,11 +168,12 @@ class ICEBERG_EXPORT And : public Expression {
/// evaluates to true.
class ICEBERG_EXPORT Or : public Expression {
public:
- /// \brief Constructs an Or expression from two sub-expressions.
+ /// \brief Creates an Or expression from two sub-expressions.
///
/// \param left The left operand of the OR expression
/// \param right The right operand of the OR expression
- Or(std::shared_ptr<Expression> left, std::shared_ptr<Expression> right);
+ static Result<std::unique_ptr<Or>> Make(std::shared_ptr<Expression> left,
+ std::shared_ptr<Expression> right);
/// \brief Returns the left operand of the OR expression.
///
@@ -190,6 +194,8 @@ class ICEBERG_EXPORT Or : public Expression {
bool Equals(const Expression& other) const override;
private:
+ Or(std::shared_ptr<Expression> left, std::shared_ptr<Expression> right);
+
std::shared_ptr<Expression> left_;
std::shared_ptr<Expression> right_;
};
@@ -199,10 +205,11 @@ class ICEBERG_EXPORT Or : public Expression {
/// This expression negates its child expression.
class ICEBERG_EXPORT Not : public Expression {
public:
- /// \brief Constructs a Not expression from a child expression.
+ /// \brief Creates a Not expression from a child expression.
///
/// \param child The expression to negate
- explicit Not(std::shared_ptr<Expression> child);
+ /// \return A Result containing a unique pointer to Not, or an error if
child is nullptr
+ static Result<std::unique_ptr<Not>> Make(std::shared_ptr<Expression> child);
/// \brief Returns the child expression.
///
@@ -218,6 +225,8 @@ class ICEBERG_EXPORT Not : public Expression {
bool Equals(const Expression& other) const override;
private:
+ explicit Not(std::shared_ptr<Expression> child);
+
std::shared_ptr<Expression> child_;
};
diff --git a/src/iceberg/expression/expressions.cc
b/src/iceberg/expression/expressions.cc
index 686a55d..6839157 100644
--- a/src/iceberg/expression/expressions.cc
+++ b/src/iceberg/expression/expressions.cc
@@ -22,6 +22,7 @@
#include "iceberg/exception.h"
#include "iceberg/transform.h"
#include "iceberg/type.h"
+#include "iceberg/util/macros.h"
namespace iceberg {
@@ -41,41 +42,57 @@ std::shared_ptr<Expression>
Expressions::Not(std::shared_ptr<Expression> child)
return not_expr.child();
}
- return std::make_shared<::iceberg::Not>(std::move(child));
+ ICEBERG_ASSIGN_OR_THROW(auto not_expr, iceberg::Not::Make(std::move(child)));
+ return not_expr;
}
// Transform functions
std::shared_ptr<UnboundTransform> Expressions::Bucket(std::string name,
int32_t num_buckets) {
- return std::make_shared<UnboundTransform>(Ref(std::move(name)),
- Transform::Bucket(num_buckets));
+ ICEBERG_ASSIGN_OR_THROW(
+ auto transform,
+ UnboundTransform::Make(Ref(std::move(name)),
Transform::Bucket(num_buckets)));
+ return transform;
}
std::shared_ptr<UnboundTransform> Expressions::Year(std::string name) {
- return std::make_shared<UnboundTransform>(Ref(std::move(name)),
Transform::Year());
+ ICEBERG_ASSIGN_OR_THROW(
+ auto transform, UnboundTransform::Make(Ref(std::move(name)),
Transform::Year()));
+ return transform;
}
std::shared_ptr<UnboundTransform> Expressions::Month(std::string name) {
- return std::make_shared<UnboundTransform>(Ref(std::move(name)),
Transform::Month());
+ ICEBERG_ASSIGN_OR_THROW(
+ auto transform, UnboundTransform::Make(Ref(std::move(name)),
Transform::Month()));
+ return transform;
}
std::shared_ptr<UnboundTransform> Expressions::Day(std::string name) {
- return std::make_shared<UnboundTransform>(Ref(std::move(name)),
Transform::Day());
+ ICEBERG_ASSIGN_OR_THROW(auto transform,
+ UnboundTransform::Make(Ref(std::move(name)),
Transform::Day()));
+ return transform;
}
std::shared_ptr<UnboundTransform> Expressions::Hour(std::string name) {
- return std::make_shared<UnboundTransform>(Ref(std::move(name)),
Transform::Hour());
+ ICEBERG_ASSIGN_OR_THROW(
+ auto transform, UnboundTransform::Make(Ref(std::move(name)),
Transform::Hour()));
+ return transform;
}
std::shared_ptr<UnboundTransform> Expressions::Truncate(std::string name,
int32_t width) {
- return std::make_shared<UnboundTransform>(Ref(std::move(name)),
- Transform::Truncate(width));
+ ICEBERG_ASSIGN_OR_THROW(
+ auto transform,
+ UnboundTransform::Make(Ref(std::move(name)),
Transform::Truncate(width)));
+ return transform;
}
std::shared_ptr<UnboundTransform> Expressions::Transform(
std::string name, std::shared_ptr<::iceberg::Transform> transform) {
- return std::make_shared<UnboundTransform>(Ref(std::move(name)),
std::move(transform));
+ ICEBERG_ASSIGN_OR_THROW(
+ auto unbound_transform,
+ UnboundTransform::Make(Ref(std::move(name)), std::move(transform)));
+ return unbound_transform;
}
// Template implementations for unary predicates
@@ -327,11 +344,12 @@ std::shared_ptr<False> Expressions::AlwaysFalse() {
return False::Instance(); }
// Utilities
std::shared_ptr<NamedReference> Expressions::Ref(std::string name) {
- return std::make_shared<NamedReference>(std::move(name));
+ ICEBERG_ASSIGN_OR_THROW(auto ref, NamedReference::Make(std::move(name)));
+ return ref;
}
Literal Expressions::Lit(Literal::Value value, std::shared_ptr<PrimitiveType>
type) {
- throw IcebergError("Literal creation is not implemented");
+ throw ExpressionError("Literal creation is not implemented");
}
} // namespace iceberg
diff --git a/src/iceberg/expression/expressions.h
b/src/iceberg/expression/expressions.h
index 66083da..13895bd 100644
--- a/src/iceberg/expression/expressions.h
+++ b/src/iceberg/expression/expressions.h
@@ -27,14 +27,18 @@
#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"
+#include "iceberg/util/macros.h"
namespace iceberg {
-/// \brief Factory methods for creating expressions.
+/// \brief Fluent APIs to create expressions.
+///
+/// \throw `ExpressionError` for invalid expression.
class ICEBERG_EXPORT Expressions {
public:
// Logical operations
@@ -60,7 +64,9 @@ class ICEBERG_EXPORT Expressions {
return left;
}
- return std::make_shared<::iceberg::And>(std::move(left),
std::move(right));
+ ICEBERG_ASSIGN_OR_THROW(auto and_expr,
+ iceberg::And::Make(std::move(left),
std::move(right)));
+ return and_expr;
} else {
return And(And(std::move(left), std::move(right)),
std::forward<Args>(args)...);
}
@@ -86,7 +92,9 @@ class ICEBERG_EXPORT Expressions {
return left;
}
- return std::make_shared<::iceberg::Or>(std::move(left),
std::move(right));
+ ICEBERG_ASSIGN_OR_THROW(auto or_expr,
+ iceberg::Or::Make(std::move(left),
std::move(right)));
+ return or_expr;
} else {
return Or(Or(std::move(left), std::move(right)),
std::forward<Args>(args)...);
}
diff --git a/src/iceberg/expression/term.cc b/src/iceberg/expression/term.cc
index 30bdf8e..ba6e55e 100644
--- a/src/iceberg/expression/term.cc
+++ b/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()) [[unlikely]] {
+ return InvalidExpression("NamedReference cannot have empty field name");
+ }
+ return std::unique_ptr<NamedReference>(new
NamedReference(std::move(field_name)));
+}
+
NamedReference::NamedReference(std::string field_name)
- : field_name_(std::move(field_name)) {}
+ : field_name_(std::move(field_name)) {
+ ICEBERG_DCHECK(!field_name_.empty(), "NamedReference cannot have empty field
name");
+}
NamedReference::~NamedReference() = default;
@@ -51,11 +60,11 @@ Result<std::shared_ptr<BoundReference>>
NamedReference::Bind(const Schema& schem
bool
case_sensitive) const {
ICEBERG_ASSIGN_OR_RAISE(auto field_opt,
schema.GetFieldByName(field_name_, case_sensitive));
- if (!field_opt.has_value()) {
+ if (!field_opt.has_value()) [[unlikely]] {
return InvalidExpression("Cannot find field '{}' in struct: {}",
field_name_,
schema.ToString());
}
- return std::make_shared<BoundReference>(field_opt.value().get());
+ return BoundReference::Make(field_opt.value().get());
}
std::string NamedReference::ToString() const {
@@ -63,7 +72,18 @@ std::string NamedReference::ToString() const {
}
// BoundReference implementation
-BoundReference::BoundReference(SchemaField field) : field_(std::move(field)) {}
+Result<std::unique_ptr<BoundReference>> BoundReference::Make(SchemaField
field) {
+ if (auto status = field.Validate(); !status.has_value()) [[unlikely]] {
+ return InvalidExpression("Cannot create BoundReference with invalid field:
{}",
+ status.error().message);
+ }
+ return std::unique_ptr<BoundReference>(new BoundReference(std::move(field)));
+}
+
+BoundReference::BoundReference(SchemaField field) : field_(std::move(field)) {
+ ICEBERG_DCHECK(field_.Validate().has_value(),
+ "Cannot create BoundReference with invalid field");
+}
BoundReference::~BoundReference() = default;
@@ -87,9 +107,22 @@ 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 || !transform) [[unlikely]] {
+ return InvalidExpression(
+ "Cannot create UnboundTransform with null reference or transform");
+ }
+ return std::unique_ptr<UnboundTransform>(
+ new UnboundTransform(std::move(ref), std::move(transform)));
+}
+
UnboundTransform::UnboundTransform(std::shared_ptr<NamedReference> ref,
std::shared_ptr<Transform> transform)
- : ref_(std::move(ref)), transform_(std::move(transform)) {}
+ : ref_(std::move(ref)), transform_(std::move(transform)) {
+ ICEBERG_DCHECK(!ref || !transform,
+ "Cannot create UnboundTransform with null reference or
transform");
+}
UnboundTransform::~UnboundTransform() = default;
@@ -101,17 +134,31 @@ 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));
+ return BoundTransform::Make(std::move(bound_ref), transform_,
+ std::move(transform_func));
}
// BoundTransform implementation
+Result<std::unique_ptr<BoundTransform>> BoundTransform::Make(
+ std::shared_ptr<BoundReference> ref, std::shared_ptr<Transform> transform,
+ std::shared_ptr<TransformFunction> transform_func) {
+ if (!ref || !transform || !transform_func) [[unlikely]] {
+ return InvalidExpression(
+ "Cannot create BoundTransform with null reference or transform");
+ }
+ return std::unique_ptr<BoundTransform>(new BoundTransform(
+ std::move(ref), std::move(transform), std::move(transform_func)));
+}
+
BoundTransform::BoundTransform(std::shared_ptr<BoundReference> ref,
std::shared_ptr<Transform> transform,
std::shared_ptr<TransformFunction>
transform_func)
: ref_(std::move(ref)),
transform_(std::move(transform)),
- transform_func_(std::move(transform_func)) {}
+ transform_func_(std::move(transform_func)) {
+ ICEBERG_DCHECK(ref_ && transform_ && transform_func_,
+ "Cannot create BoundTransform with null reference or
transform");
+}
BoundTransform::~BoundTransform() = default;
diff --git a/src/iceberg/expression/term.h b/src/iceberg/expression/term.h
index e0a883c..6259b82 100644
--- a/src/iceberg/expression/term.h
+++ b/src/iceberg/expression/term.h
@@ -32,11 +32,6 @@
namespace iceberg {
-// TODO(gangwu): add a struct-like interface to wrap a row of data from
ArrowArray or
-// structs like ManifestFile and ManifestEntry to facilitate generailization
of the
-// evaluation of expressions on top of different data structures.
-class StructLike;
-
/// \brief A term is an expression node that produces a typed value when
evaluated.
class ICEBERG_EXPORT Term : public util::Formattable {
public:
@@ -138,7 +133,7 @@ class ICEBERG_EXPORT NamedReference
/// \brief Create a named reference to a field.
///
/// \param field_name The name of the field to reference
- explicit NamedReference(std::string field_name);
+ static Result<std::unique_ptr<NamedReference>> Make(std::string field_name);
~NamedReference() override;
@@ -154,6 +149,8 @@ class ICEBERG_EXPORT NamedReference
Kind kind() const override { return Kind::kReference; }
private:
+ explicit NamedReference(std::string field_name);
+
std::string field_name_;
};
@@ -166,7 +163,7 @@ class ICEBERG_EXPORT BoundReference
/// \brief Create a bound reference.
///
/// \param field The schema field
- explicit BoundReference(SchemaField field);
+ static Result<std::unique_ptr<BoundReference>> Make(SchemaField field);
~BoundReference() override;
@@ -189,6 +186,8 @@ class ICEBERG_EXPORT BoundReference
Kind kind() const override { return Kind::kReference; }
private:
+ explicit BoundReference(SchemaField field);
+
SchemaField field_;
};
@@ -199,8 +198,8 @@ class ICEBERG_EXPORT UnboundTransform : public
UnboundTerm<class BoundTransform>
///
/// \param ref The term to apply the transformation to
/// \param transform The transformation function to apply
- UnboundTransform(std::shared_ptr<NamedReference> ref,
- std::shared_ptr<Transform> transform);
+ static Result<std::unique_ptr<UnboundTransform>> Make(
+ std::shared_ptr<NamedReference> ref, std::shared_ptr<Transform>
transform);
~UnboundTransform() override;
@@ -216,6 +215,9 @@ class ICEBERG_EXPORT UnboundTransform : public
UnboundTerm<class BoundTransform>
Kind kind() const override { return Kind::kTransform; }
private:
+ UnboundTransform(std::shared_ptr<NamedReference> ref,
+ std::shared_ptr<Transform> transform);
+
std::shared_ptr<NamedReference> ref_;
std::shared_ptr<Transform> transform_;
};
@@ -228,9 +230,9 @@ class ICEBERG_EXPORT BoundTransform : public BoundTerm {
/// \param ref The bound term to apply the transformation to
/// \param transform The transform to apply
/// \param transform_func The bound transform function to apply
- BoundTransform(std::shared_ptr<BoundReference> ref,
- std::shared_ptr<Transform> transform,
- std::shared_ptr<TransformFunction> transform_func);
+ static Result<std::unique_ptr<BoundTransform>> Make(
+ std::shared_ptr<BoundReference> ref, std::shared_ptr<Transform>
transform,
+ std::shared_ptr<TransformFunction> transform_func);
~BoundTransform() override;
@@ -251,6 +253,10 @@ class ICEBERG_EXPORT BoundTransform : public BoundTerm {
Kind kind() const override { return Kind::kTransform; }
private:
+ BoundTransform(std::shared_ptr<BoundReference> ref,
+ std::shared_ptr<Transform> transform,
+ std::shared_ptr<TransformFunction> transform_func);
+
std::shared_ptr<BoundReference> ref_;
std::shared_ptr<Transform> transform_;
std::shared_ptr<TransformFunction> transform_func_;
diff --git a/src/iceberg/schema_field.cc b/src/iceberg/schema_field.cc
index 04dc35b..04b55a0 100644
--- a/src/iceberg/schema_field.cc
+++ b/src/iceberg/schema_field.cc
@@ -54,6 +54,16 @@ bool SchemaField::optional() const { return optional_; }
std::string_view SchemaField::doc() const { return doc_; }
+Status SchemaField::Validate() const {
+ if (name_.empty()) [[unlikely]] {
+ return InvalidSchema("SchemaField cannot have empty name");
+ }
+ if (type_ == nullptr) [[unlikely]] {
+ return InvalidSchema("SchemaField cannot have null type");
+ }
+ return {};
+}
+
std::string SchemaField::ToString() const {
std::string result = std::format("{} ({}): {} ({}){}", name_, field_id_,
*type_,
optional_ ? "optional" : "required",
diff --git a/src/iceberg/schema_field.h b/src/iceberg/schema_field.h
index e947f20..4190dba 100644
--- a/src/iceberg/schema_field.h
+++ b/src/iceberg/schema_field.h
@@ -29,6 +29,7 @@
#include <string_view>
#include "iceberg/iceberg_export.h"
+#include "iceberg/result.h"
#include "iceberg/type_fwd.h"
#include "iceberg/util/formattable.h"
@@ -72,6 +73,8 @@ class ICEBERG_EXPORT SchemaField : public
iceberg::util::Formattable {
[[nodiscard]] std::string ToString() const override;
+ Status Validate() const;
+
friend bool operator==(const SchemaField& lhs, const SchemaField& rhs) {
return lhs.Equals(rhs);
}
diff --git a/src/iceberg/test/expression_test.cc
b/src/iceberg/test/expression_test.cc
index 326dadc..97e4272 100644
--- a/src/iceberg/test/expression_test.cc
+++ b/src/iceberg/test/expression_test.cc
@@ -55,12 +55,15 @@ TEST(ANDTest, Basic) {
auto true_expr2 = True::Instance();
// Create an AND expression
- auto and_expr = std::make_shared<And>(true_expr1, true_expr2);
+ auto and_expr_result = And::Make(true_expr1, true_expr2);
+ ASSERT_THAT(and_expr_result, IsOk());
+ auto and_expr =
std::shared_ptr<Expression>(std::move(and_expr_result.value()));
EXPECT_EQ(and_expr->op(), Expression::Operation::kAnd);
EXPECT_EQ(and_expr->ToString(), "(true and true)");
- EXPECT_EQ(and_expr->left()->op(), Expression::Operation::kTrue);
- EXPECT_EQ(and_expr->right()->op(), Expression::Operation::kTrue);
+ auto& and_ref = static_cast<const And&>(*and_expr);
+ EXPECT_EQ(and_ref.left()->op(), Expression::Operation::kTrue);
+ EXPECT_EQ(and_ref.right()->op(), Expression::Operation::kTrue);
}
TEST(ORTest, Basic) {
@@ -69,12 +72,15 @@ TEST(ORTest, Basic) {
auto false_expr = False::Instance();
// Create an OR expression
- auto or_expr = std::make_shared<Or>(true_expr, false_expr);
+ auto or_expr_result = Or::Make(true_expr, false_expr);
+ ASSERT_THAT(or_expr_result, IsOk());
+ auto or_expr =
std::shared_ptr<Expression>(std::move(or_expr_result.value()));
EXPECT_EQ(or_expr->op(), Expression::Operation::kOr);
EXPECT_EQ(or_expr->ToString(), "(true or false)");
- EXPECT_EQ(or_expr->left()->op(), Expression::Operation::kTrue);
- EXPECT_EQ(or_expr->right()->op(), Expression::Operation::kFalse);
+ auto& or_ref = static_cast<const Or&>(*or_expr);
+ EXPECT_EQ(or_ref.left()->op(), Expression::Operation::kTrue);
+ EXPECT_EQ(or_ref.right()->op(), Expression::Operation::kFalse);
}
TEST(ORTest, Negation) {
@@ -82,7 +88,9 @@ TEST(ORTest, Negation) {
auto true_expr = True::Instance();
auto false_expr = False::Instance();
- auto or_expr = std::make_shared<Or>(true_expr, false_expr);
+ auto or_expr_result = Or::Make(true_expr, false_expr);
+ ASSERT_THAT(or_expr_result, IsOk());
+ auto or_expr =
std::shared_ptr<Expression>(std::move(or_expr_result.value()));
auto negated_or_result = or_expr->Negate();
ASSERT_THAT(negated_or_result, IsOk());
auto negated_or = negated_or_result.value();
@@ -97,20 +105,31 @@ TEST(ORTest, Equals) {
auto false_expr = False::Instance();
// Test basic equality
- auto or_expr1 = std::make_shared<Or>(true_expr, false_expr);
- auto or_expr2 = std::make_shared<Or>(true_expr, false_expr);
+ auto or_expr1_result = Or::Make(true_expr, false_expr);
+ ASSERT_THAT(or_expr1_result, IsOk());
+ auto or_expr1 =
std::shared_ptr<Expression>(std::move(or_expr1_result.value()));
+
+ auto or_expr2_result = Or::Make(true_expr, false_expr);
+ ASSERT_THAT(or_expr2_result, IsOk());
+ auto or_expr2 =
std::shared_ptr<Expression>(std::move(or_expr2_result.value()));
EXPECT_TRUE(or_expr1->Equals(*or_expr2));
// Test commutativity: (A or B) equals (B or A)
- auto or_expr3 = std::make_shared<Or>(false_expr, true_expr);
+ auto or_expr3_result = Or::Make(false_expr, true_expr);
+ ASSERT_THAT(or_expr3_result, IsOk());
+ auto or_expr3 =
std::shared_ptr<Expression>(std::move(or_expr3_result.value()));
EXPECT_TRUE(or_expr1->Equals(*or_expr3));
// Test inequality with different expressions
- auto or_expr4 = std::make_shared<Or>(true_expr, true_expr);
+ auto or_expr4_result = Or::Make(true_expr, true_expr);
+ ASSERT_THAT(or_expr4_result, IsOk());
+ auto or_expr4 =
std::shared_ptr<Expression>(std::move(or_expr4_result.value()));
EXPECT_FALSE(or_expr1->Equals(*or_expr4));
// Test inequality with different operation types
- auto and_expr = std::make_shared<And>(true_expr, false_expr);
+ auto and_expr_result = And::Make(true_expr, false_expr);
+ ASSERT_THAT(and_expr_result, IsOk());
+ auto and_expr =
std::shared_ptr<Expression>(std::move(and_expr_result.value()));
EXPECT_FALSE(or_expr1->Equals(*and_expr));
}
@@ -119,7 +138,9 @@ TEST(ANDTest, Negation) {
auto true_expr = True::Instance();
auto false_expr = False::Instance();
- auto and_expr = std::make_shared<And>(true_expr, false_expr);
+ auto and_expr_result = And::Make(true_expr, false_expr);
+ ASSERT_THAT(and_expr_result, IsOk());
+ auto and_expr =
std::shared_ptr<Expression>(std::move(and_expr_result.value()));
auto negated_and_result = and_expr->Negate();
ASSERT_THAT(negated_and_result, IsOk());
auto negated_and = negated_and_result.value();
@@ -134,20 +155,31 @@ TEST(ANDTest, Equals) {
auto false_expr = False::Instance();
// Test basic equality
- auto and_expr1 = std::make_shared<And>(true_expr, false_expr);
- auto and_expr2 = std::make_shared<And>(true_expr, false_expr);
+ auto and_expr1_result = And::Make(true_expr, false_expr);
+ ASSERT_THAT(and_expr1_result, IsOk());
+ auto and_expr1 =
std::shared_ptr<Expression>(std::move(and_expr1_result.value()));
+
+ auto and_expr2_result = And::Make(true_expr, false_expr);
+ ASSERT_THAT(and_expr2_result, IsOk());
+ auto and_expr2 =
std::shared_ptr<Expression>(std::move(and_expr2_result.value()));
EXPECT_TRUE(and_expr1->Equals(*and_expr2));
// Test commutativity: (A and B) equals (B and A)
- auto and_expr3 = std::make_shared<And>(false_expr, true_expr);
+ auto and_expr3_result = And::Make(false_expr, true_expr);
+ ASSERT_THAT(and_expr3_result, IsOk());
+ auto and_expr3 =
std::shared_ptr<Expression>(std::move(and_expr3_result.value()));
EXPECT_TRUE(and_expr1->Equals(*and_expr3));
// Test inequality with different expressions
- auto and_expr4 = std::make_shared<And>(true_expr, true_expr);
+ auto and_expr4_result = And::Make(true_expr, true_expr);
+ ASSERT_THAT(and_expr4_result, IsOk());
+ auto and_expr4 =
std::shared_ptr<Expression>(std::move(and_expr4_result.value()));
EXPECT_FALSE(and_expr1->Equals(*and_expr4));
// Test inequality with different operation types
- auto or_expr = std::make_shared<Or>(true_expr, false_expr);
+ auto or_expr_result = Or::Make(true_expr, false_expr);
+ ASSERT_THAT(or_expr_result, IsOk());
+ auto or_expr =
std::shared_ptr<Expression>(std::move(or_expr_result.value()));
EXPECT_FALSE(and_expr1->Equals(*or_expr));
}
@@ -168,17 +200,22 @@ TEST(ExpressionTest, BaseClassNegateErrorOut) {
TEST(NotTest, Basic) {
auto true_expr = True::Instance();
- auto not_expr = std::make_shared<Not>(true_expr);
+ auto not_expr_result = Not::Make(true_expr);
+ ASSERT_THAT(not_expr_result, IsOk());
+ auto not_expr =
std::shared_ptr<Expression>(std::move(not_expr_result.value()));
EXPECT_EQ(not_expr->op(), Expression::Operation::kNot);
EXPECT_EQ(not_expr->ToString(), "not(true)");
- EXPECT_EQ(not_expr->child()->op(), Expression::Operation::kTrue);
+ auto& not_ref = static_cast<const Not&>(*not_expr);
+ EXPECT_EQ(not_ref.child()->op(), Expression::Operation::kTrue);
}
TEST(NotTest, Negation) {
// Test that not(not(x)) = x
auto true_expr = True::Instance();
- auto not_expr = std::make_shared<Not>(true_expr);
+ auto not_expr_result = Not::Make(true_expr);
+ ASSERT_THAT(not_expr_result, IsOk());
+ auto not_expr =
std::shared_ptr<Expression>(std::move(not_expr_result.value()));
auto negated_result = not_expr->Negate();
ASSERT_THAT(negated_result, IsOk());
@@ -193,16 +230,25 @@ TEST(NotTest, Equals) {
auto false_expr = False::Instance();
// Test basic equality
- auto not_expr1 = std::make_shared<Not>(true_expr);
- auto not_expr2 = std::make_shared<Not>(true_expr);
+ auto not_expr1_result = Not::Make(true_expr);
+ ASSERT_THAT(not_expr1_result, IsOk());
+ auto not_expr1 =
std::shared_ptr<Expression>(std::move(not_expr1_result.value()));
+
+ auto not_expr2_result = Not::Make(true_expr);
+ ASSERT_THAT(not_expr2_result, IsOk());
+ auto not_expr2 =
std::shared_ptr<Expression>(std::move(not_expr2_result.value()));
EXPECT_TRUE(not_expr1->Equals(*not_expr2));
// Test inequality with different child expressions
- auto not_expr3 = std::make_shared<Not>(false_expr);
+ auto not_expr3_result = Not::Make(false_expr);
+ ASSERT_THAT(not_expr3_result, IsOk());
+ auto not_expr3 =
std::shared_ptr<Expression>(std::move(not_expr3_result.value()));
EXPECT_FALSE(not_expr1->Equals(*not_expr3));
// Test inequality with different operation types
- auto and_expr = std::make_shared<And>(true_expr, false_expr);
+ auto and_expr_result = And::Make(true_expr, false_expr);
+ ASSERT_THAT(and_expr_result, IsOk());
+ auto and_expr =
std::shared_ptr<Expression>(std::move(and_expr_result.value()));
EXPECT_FALSE(not_expr1->Equals(*and_expr));
}
diff --git a/src/iceberg/test/predicate_test.cc
b/src/iceberg/test/predicate_test.cc
index 5ab7908..532e908 100644
--- a/src/iceberg/test/predicate_test.cc
+++ b/src/iceberg/test/predicate_test.cc
@@ -218,14 +218,14 @@ TEST_F(PredicateTest, ReferenceFactory) {
}
TEST_F(PredicateTest, NamedReferenceBasics) {
- auto ref = std::make_shared<NamedReference>("id");
+ auto ref = Expressions::Ref("id");
EXPECT_EQ(ref->name(), "id");
EXPECT_EQ(ref->ToString(), "ref(name=\"id\")");
EXPECT_EQ(ref->reference(), ref);
}
TEST_F(PredicateTest, NamedReferenceBind) {
- auto ref = std::make_shared<NamedReference>("id");
+ auto ref = Expressions::Ref("id");
auto bound_result = ref->Bind(*schema_, /*case_sensitive=*/true);
ASSERT_THAT(bound_result, IsOk());
@@ -237,15 +237,15 @@ TEST_F(PredicateTest, NamedReferenceBind) {
}
TEST_F(PredicateTest, NamedReferenceBindNonExistentField) {
- auto ref = std::make_shared<NamedReference>("non_existent_field");
+ auto ref = Expressions::Ref("non_existent_field");
auto bound_result = ref->Bind(*schema_, /*case_sensitive=*/true);
EXPECT_THAT(bound_result, IsError(ErrorKind::kInvalidExpression));
}
TEST_F(PredicateTest, BoundReferenceEquality) {
- auto ref1 = std::make_shared<NamedReference>("id");
- auto ref2 = std::make_shared<NamedReference>("id");
- auto ref3 = std::make_shared<NamedReference>("name");
+ auto ref1 = Expressions::Ref("id");
+ auto ref2 = Expressions::Ref("id");
+ auto ref3 = Expressions::Ref("name");
auto bound1 = ref1->Bind(*schema_, true).value();
auto bound2 = ref2->Bind(*schema_, true).value();
diff --git a/src/iceberg/util/macros.h b/src/iceberg/util/macros.h
index 733b07f..50ac13f 100644
--- a/src/iceberg/util/macros.h
+++ b/src/iceberg/util/macros.h
@@ -44,9 +44,16 @@
#define ICEBERG_DCHECK(expr, message) assert((expr) && (message))
+#define ERROR_TO_EXCEPTION(error) \
+ if (error.kind == iceberg::ErrorKind::kInvalidExpression) { \
+ throw iceberg::ExpressionError(error.message); \
+ } else { \
+ throw iceberg::IcebergError(error.message); \
+ }
+
#define ICEBERG_THROW_NOT_OK(result) \
if (auto&& result_name = result; !result_name) [[unlikely]] { \
- throw iceberg::IcebergError(result_name.error().message); \
+ ERROR_TO_EXCEPTION(result_name.error()); \
}
#define ICEBERG_ASSIGN_OR_THROW_IMPL(result_name, lhs, rexpr) \
