From ef0e808af1817a5c1d8ef7b0d14b2271e30678c7 Mon Sep 17 00:00:00 2001 From: Junwang Zhao Date: Fri, 18 Apr 2025 23:45:18 +0800 Subject: [PATCH 1/3] chore: use chrono::milliseconds in snapshot and consolidate error usage reduce direct unexpected usage by using more Error wrappers defined in result.h Signed-off-by: Junwang Zhao --- src/iceberg/CMakeLists.txt | 3 +- src/iceberg/expression/expression.h | 3 +- src/iceberg/file_io.h | 9 +-- src/iceberg/json_internal.cc | 23 +++---- src/iceberg/result.h | 98 ++++++++++++++++++++++++----- src/iceberg/schema_internal.cc | 90 +++++++++----------------- src/iceberg/snapshot.h | 7 +-- src/iceberg/sort_field.h | 7 +-- src/iceberg/table_metadata.cc | 10 --- src/iceberg/table_metadata.h | 13 +--- src/iceberg/transform.cc | 26 +++----- src/iceberg/transform_function.cc | 53 +++++----------- src/iceberg/util/timepoint.cc | 46 ++++++++++++++ src/iceberg/util/timepoint.h | 49 +++++++++++++++ test/json_internal_test.cc | 3 +- test/snapshot_test.cc | 17 ++--- test/sort_field_test.cc | 1 - test/transform_test.cc | 3 +- 18 files changed, 267 insertions(+), 194 deletions(-) create mode 100644 src/iceberg/util/timepoint.cc create mode 100644 src/iceberg/util/timepoint.h diff --git a/src/iceberg/CMakeLists.txt b/src/iceberg/CMakeLists.txt index 1f521181c..0f105a12e 100644 --- a/src/iceberg/CMakeLists.txt +++ b/src/iceberg/CMakeLists.txt @@ -36,7 +36,8 @@ set(ICEBERG_SOURCES transform_function.cc type.cc snapshot.cc - util/murmurhash3_internal.cc) + util/murmurhash3_internal.cc + util/timepoint.cc) set(ICEBERG_STATIC_BUILD_INTERFACE_LIBS) set(ICEBERG_SHARED_BUILD_INTERFACE_LIBS) diff --git a/src/iceberg/expression/expression.h b/src/iceberg/expression/expression.h index 3113fb8bd..82164ece0 100644 --- a/src/iceberg/expression/expression.h +++ b/src/iceberg/expression/expression.h @@ -68,8 +68,7 @@ class ICEBERG_EXPORT Expression { /// \brief Returns the negation of this expression, equivalent to not(this). virtual Result> Negate() const { - return unexpected( - Error(ErrorKind::kInvalidExpression, "Expression cannot be negated")); + return InvalidExpressionError("Expression cannot be negated"); } /// \brief Returns whether this expression will accept the same values as another. diff --git a/src/iceberg/file_io.h b/src/iceberg/file_io.h index c31d39bb3..4bf290d02 100644 --- a/src/iceberg/file_io.h +++ b/src/iceberg/file_io.h @@ -52,8 +52,7 @@ class ICEBERG_EXPORT FileIO { virtual Result ReadFile(const std::string& file_location, std::optional length) { // We provide a default implementation to avoid Windows linker error LNK2019. - return unexpected{ - {.kind = ErrorKind::kNotImplemented, .message = "ReadFile not implemented"}}; + return NotImplementedError("ReadFile not implemented"); } /// \brief Write the given content to the file at the given location. @@ -64,8 +63,7 @@ class ICEBERG_EXPORT FileIO { /// file exists. /// \return void if the write succeeded, an error code if the write failed. virtual Status WriteFile(const std::string& file_location, std::string_view content) { - return unexpected{ - {.kind = ErrorKind::kNotImplemented, .message = "WriteFile not implemented"}}; + return NotImplementedError("WriteFile not implemented"); } /// \brief Delete a file at the given location. @@ -73,8 +71,7 @@ class ICEBERG_EXPORT FileIO { /// \param file_location The location of the file to delete. /// \return void if the delete succeeded, an error code if the delete failed. virtual Status DeleteFile(const std::string& file_location) { - return unexpected{ - {.kind = ErrorKind::kNotImplemented, .message = "DeleteFile not implemented"}}; + return NotImplementedError("DeleteFile not implemented"); } }; diff --git a/src/iceberg/json_internal.cc b/src/iceberg/json_internal.cc index 965892ccc..1336bd0ec 100644 --- a/src/iceberg/json_internal.cc +++ b/src/iceberg/json_internal.cc @@ -28,6 +28,7 @@ #include #include +#include #include #include "iceberg/partition_field.h" @@ -502,7 +503,7 @@ nlohmann::json ToJson(const Snapshot& snapshot) { if (snapshot.sequence_number > TableMetadata::kInitialSequenceNumber) { json[kSequenceNumber] = snapshot.sequence_number; } - json[kTimestampMs] = snapshot.timestamp_ms; + json[kTimestampMs] = UnixMsFromTimePointMs(snapshot.timestamp_ms); json[kManifestList] = snapshot.manifest_list; // If there is an operation, write the summary map if (snapshot.operation().has_value()) { @@ -722,7 +723,9 @@ Result> SnapshotFromJson(const nlohmann::json& json) { ICEBERG_ASSIGN_OR_RAISE(auto snapshot_id, GetJsonValue(json, kSnapshotId)); ICEBERG_ASSIGN_OR_RAISE(auto sequence_number, GetJsonValueOptional(json, kSequenceNumber)); - ICEBERG_ASSIGN_OR_RAISE(auto timestamp_ms, GetJsonValue(json, kTimestampMs)); + ICEBERG_ASSIGN_OR_RAISE( + auto timestamp_ms, + GetJsonValue(json, kTimestampMs).and_then(TimePointMsFromUnixMs)); ICEBERG_ASSIGN_OR_RAISE(auto manifest_list, GetJsonValue(json, kManifestList)); @@ -735,24 +738,14 @@ Result> SnapshotFromJson(const nlohmann::json& json) { if (summary_json.has_value()) { for (const auto& [key, value] : summary_json->items()) { if (!kValidSnapshotSummaryFields.contains(key)) { - return unexpected({ - .kind = ErrorKind::kJsonParseError, - .message = std::format("Invalid snapshot summary field: {}", key), - }); + return JsonParseError("Invalid snapshot summary field: {}", key); } if (!value.is_string()) { - return unexpected({ - .kind = ErrorKind::kJsonParseError, - .message = - std::format("Invalid snapshot summary field value: {}", value.dump()), - }); + return JsonParseError("Invalid snapshot summary field value: {}", value.dump()); } if (key == SnapshotSummaryFields::kOperation && !kValidDataOperation.contains(value.get())) { - return unexpected({ - .kind = ErrorKind::kJsonParseError, - .message = std::format("Invalid snapshot operation: {}", value.dump()), - }); + return JsonParseError("Invalid snapshot operation: {}", value.dump()); } summary[key] = value.get(); } diff --git a/src/iceberg/result.h b/src/iceberg/result.h index bedaccc47..3571e0839 100644 --- a/src/iceberg/result.h +++ b/src/iceberg/result.h @@ -28,21 +28,20 @@ namespace iceberg { /// \brief Error types for iceberg. -/// TODO: add more and sort them based on some rules. enum class ErrorKind { - kNoSuchNamespace, kAlreadyExists, - kNoSuchTable, kCommitStateUnknown, - kInvalidSchema, kInvalidArgument, - kIOError, - kNotImplemented, - kUnknownError, - kNotSupported, kInvalidExpression, + kInvalidSchema, + kIOError, kJsonParseError, + kNoSuchNamespace, + kNoSuchTable, kNotFound, + kNotImplemented, + kNotSupported, + kUnknownError, }; /// \brief Error with a kind and a message. @@ -63,19 +62,27 @@ using Result = expected; using Status = Result; -/// \brief Create an unexpected error with kNotImplemented +/// \brief Create an unexpected error with kAlreadyExists template -auto NotImplementedError(const std::format_string fmt, Args&&... args) +auto AlreadyExistsError(const std::format_string fmt, Args&&... args) -> unexpected { - return unexpected({.kind = ErrorKind::kNotImplemented, + return unexpected({.kind = ErrorKind::kAlreadyExists, .message = std::format(fmt, std::forward(args)...)}); } -/// \brief Create an unexpected error with kJsonParseError +/// \brief Create an unexpected error with kCommitStateUnknown template -auto JsonParseError(const std::format_string fmt, Args&&... args) +auto CommitStateUnknownError(const std::format_string fmt, Args&&... args) -> unexpected { - return unexpected({.kind = ErrorKind::kJsonParseError, + return unexpected({.kind = ErrorKind::kCommitStateUnknown, + .message = std::format(fmt, std::forward(args)...)}); +} + +/// \brief Create an unexpected error with kInvalidArgument +template +auto InvalidArgumentError(const std::format_string fmt, Args&&... args) + -> unexpected { + return unexpected({.kind = ErrorKind::kInvalidArgument, .message = std::format(fmt, std::forward(args)...)}); } @@ -87,4 +94,67 @@ auto InvalidExpressionError(const std::format_string fmt, Args&&... arg .message = std::format(fmt, std::forward(args)...)}); } +/// \brief Create an unexpected error with kInvalidSchema +template +auto InvalidSchemaError(const std::format_string fmt, Args&&... args) + -> unexpected { + return unexpected({.kind = ErrorKind::kInvalidSchema, + .message = std::format(fmt, std::forward(args)...)}); +} + +/// \brief Create an unexpected error with kIOError +template +auto IOError(const std::format_string fmt, Args&&... args) -> unexpected { + return unexpected({.kind = ErrorKind::kIOError, + .message = std::format(fmt, std::forward(args)...)}); +} + +/// \brief Create an unexpected error with kJsonParseError +template +auto JsonParseError(const std::format_string fmt, Args&&... args) + -> unexpected { + return unexpected({.kind = ErrorKind::kJsonParseError, + .message = std::format(fmt, std::forward(args)...)}); +} + +/// \brief Create an unexpected error with kNoSuchNamespace +template +auto NoSuchNamespaceError(const std::format_string fmt, Args&&... args) + -> unexpected { + return unexpected({.kind = ErrorKind::kNoSuchNamespace, + .message = std::format(fmt, std::forward(args)...)}); +} + +/// \brief Create an unexpected error with kNoSuchTable +template +auto NoSuchTableError(const std::format_string fmt, Args&&... args) + -> unexpected { + return unexpected({.kind = ErrorKind::kNoSuchTable, + .message = std::format(fmt, std::forward(args)...)}); +} + +/// \brief Create an unexpected error with kNotImplemented +template +auto NotImplementedError(const std::format_string fmt, Args&&... args) + -> unexpected { + return unexpected({.kind = ErrorKind::kNotImplemented, + .message = std::format(fmt, std::forward(args)...)}); +} + +/// \brief Create an unexpected error with kNotSupported +template +auto NotSupportedError(const std::format_string fmt, Args&&... args) + -> unexpected { + return unexpected({.kind = ErrorKind::kNotSupported, + .message = std::format(fmt, std::forward(args)...)}); +} + +/// \brief Create an unexpected error with kUnknownError +template +auto UnknownError(const std::format_string fmt, Args&&... args) + -> unexpected { + return unexpected({.kind = ErrorKind::kUnknownError, + .message = std::format(fmt, std::forward(args)...)}); +} + } // namespace iceberg diff --git a/src/iceberg/schema_internal.cc b/src/iceberg/schema_internal.cc index 266d8d2b8..c3d562de9 100644 --- a/src/iceberg/schema_internal.cc +++ b/src/iceberg/schema_internal.cc @@ -24,8 +24,11 @@ #include #include +#include + #include "iceberg/schema.h" #include "iceberg/type.h" +#include "iceberg/util/macros.h" namespace iceberg { @@ -170,18 +173,14 @@ ArrowErrorCode ToArrowSchema(const Type& type, bool optional, std::string_view n Status ToArrowSchema(const Schema& schema, ArrowSchema* out) { if (out == nullptr) [[unlikely]] { - return unexpected{{.kind = ErrorKind::kInvalidArgument, - .message = "Output Arrow schema cannot be null"}}; + return InvalidArgumentError("Output Arrow schema cannot be null"); } if (ArrowErrorCode errorCode = ToArrowSchema(schema, /*optional=*/false, /*name=*/"", /*field_id=*/std::nullopt, out); errorCode != NANOARROW_OK) { - return unexpected{ - {.kind = ErrorKind::kInvalidSchema, - .message = std::format( - "Failed to convert Iceberg schema to Arrow schema, error code: {}", - errorCode)}}; + return InvalidSchemaError( + "Failed to convert Iceberg schema to Arrow schema, error code: {}", errorCode); } return {}; @@ -208,15 +207,12 @@ int32_t GetFieldId(const ArrowSchema& schema) { Result> FromArrowSchema(const ArrowSchema& schema) { auto to_schema_field = [](const ArrowSchema& schema) -> Result> { - auto field_type_result = FromArrowSchema(schema); - if (!field_type_result) { - return unexpected(field_type_result.error()); - } + ICEBERG_ASSIGN_OR_RAISE(auto field_type, FromArrowSchema(schema)); auto field_id = GetFieldId(schema); bool is_optional = (schema.flags & ARROW_FLAG_NULLABLE) != 0; - return std::make_unique( - field_id, schema.name, std::move(field_type_result.value()), is_optional); + return std::make_unique(field_id, schema.name, std::move(field_type), + is_optional); }; ArrowError arrow_error; @@ -225,10 +221,8 @@ Result> FromArrowSchema(const ArrowSchema& schema) { ArrowSchemaView schema_view; if (auto error_code = ArrowSchemaViewInit(&schema_view, &schema, &arrow_error); error_code != NANOARROW_OK) { - return unexpected{ - {.kind = ErrorKind::kInvalidSchema, - .message = std::format("Failed to read Arrow schema, code: {}, message: {}", - error_code, arrow_error.message)}}; + return InvalidSchemaError("Failed to read Arrow schema, code: {}, message: {}", + error_code, arrow_error.message); } switch (schema_view.type) { @@ -237,35 +231,24 @@ Result> FromArrowSchema(const ArrowSchema& schema) { fields.reserve(schema.n_children); for (int i = 0; i < schema.n_children; i++) { - auto field_result = to_schema_field(*schema.children[i]); - if (!field_result) { - return unexpected(field_result.error()); - } - fields.emplace_back(std::move(*field_result.value())); + ICEBERG_ASSIGN_OR_RAISE(auto field, to_schema_field(*schema.children[i])); + fields.emplace_back(std::move(*field)); } return std::make_shared(std::move(fields)); } case NANOARROW_TYPE_LIST: { - auto element_field_result = to_schema_field(*schema.children[0]); - if (!element_field_result) { - return unexpected(element_field_result.error()); - } - return std::make_shared(std::move(*element_field_result.value())); + ICEBERG_ASSIGN_OR_RAISE(auto element_field_result, + to_schema_field(*schema.children[0])); + return std::make_shared(std::move(*element_field_result)); } case NANOARROW_TYPE_MAP: { - auto key_field_result = to_schema_field(*schema.children[0]->children[0]); - if (!key_field_result) { - return unexpected(key_field_result.error()); - } - - auto value_field_result = to_schema_field(*schema.children[0]->children[1]); - if (!value_field_result) { - return unexpected(value_field_result.error()); - } + ICEBERG_ASSIGN_OR_RAISE(auto key_field, + to_schema_field(*schema.children[0]->children[0])); + ICEBERG_ASSIGN_OR_RAISE(auto value_field, + to_schema_field(*schema.children[0]->children[1])); - return std::make_shared(std::move(*key_field_result.value()), - std::move(*value_field_result.value())); + return std::make_shared(std::move(*key_field), std::move(*value_field)); } case NANOARROW_TYPE_BOOL: return std::make_shared(); @@ -284,20 +267,16 @@ Result> FromArrowSchema(const ArrowSchema& schema) { return std::make_shared(); case NANOARROW_TYPE_TIME64: if (schema_view.time_unit != NANOARROW_TIME_UNIT_MICRO) { - return unexpected{ - {.kind = ErrorKind::kInvalidSchema, - .message = std::format("Unsupported time unit for Arrow time type: {}", - static_cast(schema_view.time_unit))}}; + return InvalidSchemaError("Unsupported time unit for Arrow time type: {}", + static_cast(schema_view.time_unit)); } return std::make_shared(); case NANOARROW_TYPE_TIMESTAMP: { bool with_timezone = schema_view.timezone != nullptr && std::strlen(schema_view.timezone) > 0; if (schema_view.time_unit != NANOARROW_TIME_UNIT_MICRO) { - return unexpected{ - {.kind = ErrorKind::kInvalidSchema, - .message = std::format("Unsupported time unit for Arrow timestamp type: {}", - static_cast(schema_view.time_unit))}}; + return InvalidSchemaError("Unsupported time unit for Arrow timestamp type: {}", + static_cast(schema_view.time_unit)); } if (with_timezone) { return std::make_shared(); @@ -314,18 +293,15 @@ Result> FromArrowSchema(const ArrowSchema& schema) { schema_view.extension_name.size_bytes); extension_name == kArrowUuidExtensionName) { if (schema_view.fixed_size != 16) { - return unexpected{{.kind = ErrorKind::kInvalidSchema, - .message = "UUID type must have a fixed size of 16"}}; + return InvalidSchemaError("UUID type must have a fixed size of 16"); } return std::make_shared(); } return std::make_shared(schema_view.fixed_size); } default: - return unexpected{ - {.kind = ErrorKind::kInvalidSchema, - .message = std::format("Unsupported Arrow type: {}", - ArrowTypeString(schema_view.type))}}; + return InvalidSchemaError("Unsupported Arrow type: {}", + ArrowTypeString(schema_view.type)); } } @@ -343,16 +319,10 @@ std::unique_ptr FromStructType(StructType&& struct_type, Result> FromArrowSchema(const ArrowSchema& schema, std::optional schema_id) { - auto type_result = FromArrowSchema(schema); - if (!type_result) { - return unexpected(type_result.error()); - } + ICEBERG_ASSIGN_OR_RAISE(auto type, FromArrowSchema(schema)); - auto& type = type_result.value(); if (type->type_id() != TypeId::kStruct) { - return unexpected{ - {.kind = ErrorKind::kInvalidSchema, - .message = "Arrow schema must be a struct type for Iceberg schema"}}; + return InvalidSchemaError("Arrow schema must be a struct type for Iceberg schema"); } auto& struct_type = static_cast(*type); diff --git a/src/iceberg/snapshot.h b/src/iceberg/snapshot.h index bc68a4949..7be33eb8f 100644 --- a/src/iceberg/snapshot.h +++ b/src/iceberg/snapshot.h @@ -27,6 +27,7 @@ #include "iceberg/iceberg_export.h" #include "iceberg/result.h" +#include "iceberg/util/timepoint.h" namespace iceberg { @@ -55,9 +56,7 @@ ICEBERG_EXPORT constexpr Result SnapshotRefTypeFromString( std::string_view str) noexcept { if (str == "branch") return SnapshotRefType::kBranch; if (str == "tag") return SnapshotRefType::kTag; - return unexpected( - {.kind = ErrorKind::kInvalidArgument, - .message = "Invalid snapshot reference type: {}" + std::string(str)}); + return InvalidArgumentError("Invalid snapshot reference type: {}", str); } /// \brief A reference to a snapshot, either a branch or a tag. @@ -243,7 +242,7 @@ struct ICEBERG_EXPORT Snapshot { int64_t sequence_number; /// A timestamp when the snapshot was created, used for garbage collection and table /// inspection. - int64_t timestamp_ms; + TimePointMs timestamp_ms; /// The location of a manifest list for this snapshot that tracks manifest files with /// additional metadata. std::string manifest_list; diff --git a/src/iceberg/sort_field.h b/src/iceberg/sort_field.h index 3ca70625d..1470bfb0c 100644 --- a/src/iceberg/sort_field.h +++ b/src/iceberg/sort_field.h @@ -57,9 +57,7 @@ ICEBERG_EXPORT constexpr Result SortDirectionFromString( std::string_view str) { if (str == "asc") return SortDirection::kAscending; if (str == "desc") return SortDirection::kDescending; - return unexpected( - {.kind = ErrorKind::kInvalidArgument, - .message = "Invalid SortDirection string: " + std::string(str)}); + return InvalidArgumentError("Invalid SortDirection string: {}", str); } enum class NullOrder { @@ -83,8 +81,7 @@ ICEBERG_EXPORT constexpr std::string_view NullOrderToString(NullOrder null_order ICEBERG_EXPORT constexpr Result NullOrderFromString(std::string_view str) { if (str == "nulls-first") return NullOrder::kFirst; if (str == "nulls-last") return NullOrder::kLast; - return unexpected({.kind = ErrorKind::kInvalidArgument, - .message = "Invalid NullOrder string: " + std::string(str)}); + return InvalidArgumentError("Invalid NullOrder string: {}", str); } /// \brief a field with its transform. diff --git a/src/iceberg/table_metadata.cc b/src/iceberg/table_metadata.cc index 494db0c9d..326e57422 100644 --- a/src/iceberg/table_metadata.cc +++ b/src/iceberg/table_metadata.cc @@ -78,14 +78,4 @@ Result> TableMetadata::SortOrder() const { return *iter; } -Result TimePointMsFromUnixMs(int64_t unix_ms) { - return TimePointMs{std::chrono::milliseconds(unix_ms)}; -} - -int64_t UnixMsFromTimePointMs(const TimePointMs& time_point_ms) { - return std::chrono::duration_cast( - time_point_ms.time_since_epoch()) - .count(); -} - } // namespace iceberg diff --git a/src/iceberg/table_metadata.h b/src/iceberg/table_metadata.h index 3081d56ae..e993e275d 100644 --- a/src/iceberg/table_metadata.h +++ b/src/iceberg/table_metadata.h @@ -22,28 +22,17 @@ /// \file iceberg/table_metadata.h /// Table metadata for Iceberg tables. -#include #include #include #include #include #include "iceberg/iceberg_export.h" -#include "iceberg/result.h" #include "iceberg/type_fwd.h" +#include "iceberg/util/timepoint.h" namespace iceberg { -/// \brief A time point in milliseconds -using TimePointMs = - std::chrono::time_point; - -/// \brief Returns a TimePointMs from a Unix timestamp in milliseconds -ICEBERG_EXPORT Result TimePointMsFromUnixMs(int64_t unix_ms); - -/// \brief Returns a Unix timestamp in milliseconds from a TimePointMs -ICEBERG_EXPORT int64_t UnixMsFromTimePointMs(const TimePointMs& time_point_ms); - /// \brief Represents a snapshot log entry struct ICEBERG_EXPORT SnapshotLogEntry { /// The timestamp in milliseconds of the change diff --git a/src/iceberg/transform.cc b/src/iceberg/transform.cc index 8ba12ce6b..fb1205c7b 100644 --- a/src/iceberg/transform.cc +++ b/src/iceberg/transform.cc @@ -22,6 +22,8 @@ #include #include +#include + #include "iceberg/transform_function.h" #include "iceberg/type.h" @@ -119,22 +121,16 @@ Result> Transform::Bind( if (auto param = std::get_if(¶m_)) { return std::make_unique(source_type, *param); } - return unexpected({ - .kind = ErrorKind::kInvalidArgument, - .message = std::format( - "Bucket requires int32 param, none found in transform '{}'", type_str), - }); + return InvalidArgumentError( + "Bucket requires int32 param, none found in transform '{}'", type_str); } case TransformType::kTruncate: { if (auto param = std::get_if(¶m_)) { return std::make_unique(source_type, *param); } - return unexpected({ - .kind = ErrorKind::kInvalidArgument, - .message = std::format( - "Truncate requires int32 param, none found in transform '{}'", type_str), - }); + return InvalidArgumentError( + "Truncate requires int32 param, none found in transform '{}'", type_str); } case TransformType::kYear: @@ -149,10 +145,7 @@ Result> Transform::Bind( return std::make_unique(source_type); default: - return unexpected({ - .kind = ErrorKind::kNotSupported, - .message = std::format("Unsupported transform type: '{}'", type_str), - }); + return NotSupportedError("Unsupported transform type: '{}'", type_str); } } @@ -216,10 +209,7 @@ Result> TransformFromString(std::string_view transfor } } - return unexpected({ - .kind = ErrorKind::kInvalidArgument, - .message = std::format("Invalid Transform string: {}", transform_str), - }); + return InvalidArgumentError("Invalid Transform string: {}", transform_str); } } // namespace iceberg diff --git a/src/iceberg/transform_function.cc b/src/iceberg/transform_function.cc index eb01fb8ba..8746020fa 100644 --- a/src/iceberg/transform_function.cc +++ b/src/iceberg/transform_function.cc @@ -19,8 +19,6 @@ #include "iceberg/transform_function.h" -#include - #include "iceberg/type.h" namespace iceberg { @@ -29,17 +27,14 @@ IdentityTransform::IdentityTransform(std::shared_ptr const& source_type) : TransformFunction(TransformType::kIdentity, source_type) {} Result IdentityTransform::Transform(const ArrowArray& input) { - return unexpected( - {.kind = ErrorKind::kNotImplemented, .message = "IdentityTransform::Transform"}); + return NotImplementedError("IdentityTransform::Transform"); } Result> IdentityTransform::ResultType() const { auto src_type = source_type(); if (!src_type || !src_type->is_primitive()) { - return unexpected(Error{ - .kind = ErrorKind::kNotSupported, - .message = std::format("{} is not a valid input type for identity transform", - src_type ? src_type->ToString() : "null")}); + return NotSupportedError("{} is not a valid input type for identity transform", + src_type ? src_type->ToString() : "null"); } return src_type; } @@ -49,13 +44,11 @@ BucketTransform::BucketTransform(std::shared_ptr const& source_type, : TransformFunction(TransformType::kBucket, source_type), num_buckets_(num_buckets) {} Result BucketTransform::Transform(const ArrowArray& input) { - return unexpected( - {.kind = ErrorKind::kNotImplemented, .message = "BucketTransform::Transform"}); + return NotImplementedError("BucketTransform::Transform"); } Result> BucketTransform::ResultType() const { - return unexpected( - {.kind = ErrorKind::kNotImplemented, .message = "BucketTransform::result_type"}); + return NotImplementedError("BucketTransform::result_type"); } TruncateTransform::TruncateTransform(std::shared_ptr const& source_type, @@ -63,78 +56,66 @@ TruncateTransform::TruncateTransform(std::shared_ptr const& source_type, : TransformFunction(TransformType::kTruncate, source_type), width_(width) {} Result TruncateTransform::Transform(const ArrowArray& input) { - return unexpected( - {.kind = ErrorKind::kNotImplemented, .message = "TruncateTransform::Transform"}); + return NotImplementedError("TruncateTransform::Transform"); } Result> TruncateTransform::ResultType() const { - return unexpected( - {.kind = ErrorKind::kNotImplemented, .message = "TruncateTransform::result_type"}); + return NotImplementedError("TruncateTransform::result_type"); } YearTransform::YearTransform(std::shared_ptr const& source_type) : TransformFunction(TransformType::kTruncate, source_type) {} Result YearTransform::Transform(const ArrowArray& input) { - return unexpected( - {.kind = ErrorKind::kNotImplemented, .message = "YearTransform::Transform"}); + return NotImplementedError("YearTransform::Transform"); } Result> YearTransform::ResultType() const { - return unexpected( - {.kind = ErrorKind::kNotImplemented, .message = "YearTransform::result_type"}); + return NotImplementedError("YearTransform::result_type"); } MonthTransform::MonthTransform(std::shared_ptr const& source_type) : TransformFunction(TransformType::kMonth, source_type) {} Result MonthTransform::Transform(const ArrowArray& input) { - return unexpected( - {.kind = ErrorKind::kNotImplemented, .message = "MonthTransform::Transform"}); + return NotImplementedError("MonthTransform::Transform"); } Result> MonthTransform::ResultType() const { - return unexpected( - {.kind = ErrorKind::kNotImplemented, .message = "MonthTransform::result_type"}); + return NotImplementedError("MonthTransform::result_type"); } DayTransform::DayTransform(std::shared_ptr const& source_type) : TransformFunction(TransformType::kDay, source_type) {} Result DayTransform::Transform(const ArrowArray& input) { - return unexpected( - {.kind = ErrorKind::kNotImplemented, .message = "DayTransform::Transform"}); + return NotImplementedError("DayTransform::Transform"); } Result> DayTransform::ResultType() const { - return unexpected( - {.kind = ErrorKind::kNotImplemented, .message = "DayTransform::result_type"}); + return NotImplementedError("DayTransform::result_type"); } HourTransform::HourTransform(std::shared_ptr const& source_type) : TransformFunction(TransformType::kHour, source_type) {} Result HourTransform::Transform(const ArrowArray& input) { - return unexpected( - {.kind = ErrorKind::kNotImplemented, .message = "HourTransform::Transform"}); + return NotImplementedError("HourTransform::Transform"); } Result> HourTransform::ResultType() const { - return unexpected( - {.kind = ErrorKind::kNotImplemented, .message = "HourTransform::result_type"}); + return NotImplementedError("HourTransform::result_type"); } VoidTransform::VoidTransform(std::shared_ptr const& source_type) : TransformFunction(TransformType::kVoid, source_type) {} Result VoidTransform::Transform(const ArrowArray& input) { - return unexpected( - {.kind = ErrorKind::kNotImplemented, .message = "VoidTransform::Transform"}); + return NotImplementedError("VoidTransform::Transform"); } Result> VoidTransform::ResultType() const { - return unexpected( - {.kind = ErrorKind::kNotImplemented, .message = "VoidTransform::result_type"}); + return NotImplementedError("VoidTransform::result_type"); } } // namespace iceberg diff --git a/src/iceberg/util/timepoint.cc b/src/iceberg/util/timepoint.cc new file mode 100644 index 000000000..07fb2d310 --- /dev/null +++ b/src/iceberg/util/timepoint.cc @@ -0,0 +1,46 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +#include "iceberg/util/timepoint.h" + +#include + +namespace iceberg { + +Result TimePointMsFromUnixMs(int64_t unix_ms) { + return TimePointMs{std::chrono::milliseconds(unix_ms)}; +} + +int64_t UnixMsFromTimePointMs(const TimePointMs& time_point_ms) { + return std::chrono::duration_cast( + time_point_ms.time_since_epoch()) + .count(); +} + +Result TimePointNsFromUnixNs(int64_t unix_ns) { + return TimePointNs{std::chrono::nanoseconds(unix_ns)}; +} + +int64_t UnixNsFromTimePointNs(const TimePointNs& time_point_ns) { + return std::chrono::duration_cast( + time_point_ns.time_since_epoch()) + .count(); +} + +} // namespace iceberg diff --git a/src/iceberg/util/timepoint.h b/src/iceberg/util/timepoint.h new file mode 100644 index 000000000..89cbd8210 --- /dev/null +++ b/src/iceberg/util/timepoint.h @@ -0,0 +1,49 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +#pragma once + +#include + +#include "iceberg/iceberg_export.h" +#include "iceberg/result.h" + +namespace iceberg { + +/// \brief A time point in milliseconds +using TimePointMs = + std::chrono::time_point; + +/// \brief A time point in nanoseconds +using TimePointNs = + std::chrono::time_point; + +/// \brief Returns a TimePointMs from a Unix timestamp in milliseconds +ICEBERG_EXPORT Result TimePointMsFromUnixMs(int64_t unix_ms); + +/// \brief Returns a Unix timestamp in milliseconds from a TimePointMs +ICEBERG_EXPORT int64_t UnixMsFromTimePointMs(const TimePointMs& time_point_ms); + +/// \brief Returns a TimePointNs from a Unix timestamp in nanoseconds +ICEBERG_EXPORT Result TimePointNsFromUnixNs(int64_t unix_ns); + +/// \brief Returns a Unix timestamp in nanoseconds from a TimePointNs +ICEBERG_EXPORT int64_t UnixNsFromTimePointNs(const TimePointNs& time_point_ns); + +} // namespace iceberg diff --git a/test/json_internal_test.cc b/test/json_internal_test.cc index 0ff158e5e..20e379476 100644 --- a/test/json_internal_test.cc +++ b/test/json_internal_test.cc @@ -23,6 +23,7 @@ #include #include +#include #include #include "gmock/gmock.h" @@ -198,7 +199,7 @@ TEST(JsonInternalTest, Snapshot) { Snapshot snapshot{.snapshot_id = 1234567890, .parent_snapshot_id = 9876543210, .sequence_number = 99, - .timestamp_ms = 1234567890123, + .timestamp_ms = TimePointMsFromUnixMs(1234567890123).value(), .manifest_list = "/path/to/manifest_list", .summary = summary, .schema_id = 42}; diff --git a/test/snapshot_test.cc b/test/snapshot_test.cc index 995c64a86..a4895440f 100644 --- a/test/snapshot_test.cc +++ b/test/snapshot_test.cc @@ -19,6 +19,7 @@ #include "iceberg/snapshot.h" #include +#include namespace iceberg { @@ -83,7 +84,7 @@ TEST_F(SnapshotTest, ConstructionAndFieldAccess) { Snapshot snapshot{.snapshot_id = 12345, .parent_snapshot_id = 54321, .sequence_number = 1, - .timestamp_ms = 1615569200000, + .timestamp_ms = TimePointMsFromUnixMs(1615569200000).value(), .manifest_list = "s3://example/manifest_list.avro", .summary = summary1, .schema_id = 10}; @@ -92,7 +93,7 @@ TEST_F(SnapshotTest, ConstructionAndFieldAccess) { EXPECT_TRUE(snapshot.parent_snapshot_id.has_value()); EXPECT_EQ(*snapshot.parent_snapshot_id, 54321); EXPECT_EQ(snapshot.sequence_number, 1); - EXPECT_EQ(snapshot.timestamp_ms, 1615569200000); + EXPECT_EQ(snapshot.timestamp_ms.time_since_epoch().count(), 1615569200000); EXPECT_EQ(snapshot.manifest_list, "s3://example/manifest_list.avro"); EXPECT_EQ(snapshot.operation().value(), DataOperation::kAppend); EXPECT_EQ(snapshot.summary.at(std::string(SnapshotSummaryFields::kAddedDataFiles)), @@ -105,14 +106,14 @@ TEST_F(SnapshotTest, ConstructionAndFieldAccess) { TEST_F(SnapshotTest, EqualityComparison) { // Test the == and != operators - Snapshot snapshot1(12345, {}, 1, 1615569200000, "s3://example/manifest_list.avro", - summary1, {}); + Snapshot snapshot1(12345, {}, 1, TimePointMsFromUnixMs(1615569200000).value(), + "s3://example/manifest_list.avro", summary1, {}); - Snapshot snapshot2(12345, {}, 1, 1615569200000, "s3://example/manifest_list.avro", - summary2, {}); + Snapshot snapshot2(12345, {}, 1, TimePointMsFromUnixMs(1615569200000).value(), + "s3://example/manifest_list.avro", summary2, {}); - Snapshot snapshot3(67890, {}, 1, 1615569200000, "s3://example/manifest_list.avro", - summary3, {}); + Snapshot snapshot3(67890, {}, 1, TimePointMsFromUnixMs(1615569200000).value(), + "s3://example/manifest_list.avro", summary3, {}); EXPECT_EQ(snapshot1, snapshot2); EXPECT_NE(snapshot1, snapshot3); diff --git a/test/sort_field_test.cc b/test/sort_field_test.cc index e0bd7f106..d8be7997e 100644 --- a/test/sort_field_test.cc +++ b/test/sort_field_test.cc @@ -24,7 +24,6 @@ #include #include "iceberg/transform.h" -#include "iceberg/type.h" #include "iceberg/util/formatter.h" // IWYU pragma: keep namespace iceberg { diff --git a/test/transform_test.cc b/test/transform_test.cc index fb7b86672..f4c68a699 100644 --- a/test/transform_test.cc +++ b/test/transform_test.cc @@ -27,6 +27,7 @@ #include "iceberg/type.h" #include "iceberg/util/formatter.h" // IWYU pragma: keep +#include "matchers.h" namespace iceberg { @@ -112,7 +113,7 @@ TEST(TransformFromStringTest, NegativeCases) { for (const auto& str : invalid_cases) { auto result = TransformFromString(str); EXPECT_FALSE(result.has_value()) << "Unexpected success for: " << str; - EXPECT_EQ(result.error().kind, ErrorKind::kInvalidArgument); + EXPECT_THAT(result, IsError(ErrorKind::kInvalidArgument)); } } From 33d1fd733acd68f04c78abb36f095e3c0db593d1 Mon Sep 17 00:00:00 2001 From: Junwang Zhao Date: Sat, 19 Apr 2025 16:13:25 +0800 Subject: [PATCH 2/3] fix: use macro to define the Error wrappers Signed-off-by: Junwang Zhao --- src/iceberg/expression/expression.cc | 2 +- src/iceberg/expression/expression.h | 2 +- src/iceberg/file_io.h | 6 +- src/iceberg/result.h | 117 ++++++--------------------- src/iceberg/schema_internal.cc | 27 +++---- src/iceberg/snapshot.h | 2 +- src/iceberg/sort_field.h | 4 +- src/iceberg/transform.cc | 12 ++- src/iceberg/transform_function.cc | 34 ++++---- test/json_internal_test.cc | 3 +- test/snapshot_test.cc | 3 +- 11 files changed, 68 insertions(+), 144 deletions(-) diff --git a/src/iceberg/expression/expression.cc b/src/iceberg/expression/expression.cc index 2914c81f4..77f341eb5 100644 --- a/src/iceberg/expression/expression.cc +++ b/src/iceberg/expression/expression.cc @@ -51,7 +51,7 @@ std::string And::ToString() const { Result> And::Negate() const { // TODO(yingcai-cy): Implement Or expression - return InvalidExpressionError("And negation not yet implemented"); + return InvalidExpression("And negation not yet implemented"); } bool And::Equals(const Expression& expr) const { diff --git a/src/iceberg/expression/expression.h b/src/iceberg/expression/expression.h index 82164ece0..258c9ee2a 100644 --- a/src/iceberg/expression/expression.h +++ b/src/iceberg/expression/expression.h @@ -68,7 +68,7 @@ class ICEBERG_EXPORT Expression { /// \brief Returns the negation of this expression, equivalent to not(this). virtual Result> Negate() const { - return InvalidExpressionError("Expression cannot be negated"); + return InvalidExpression("Expression cannot be negated"); } /// \brief Returns whether this expression will accept the same values as another. diff --git a/src/iceberg/file_io.h b/src/iceberg/file_io.h index 4bf290d02..259da7556 100644 --- a/src/iceberg/file_io.h +++ b/src/iceberg/file_io.h @@ -52,7 +52,7 @@ class ICEBERG_EXPORT FileIO { virtual Result ReadFile(const std::string& file_location, std::optional length) { // We provide a default implementation to avoid Windows linker error LNK2019. - return NotImplementedError("ReadFile not implemented"); + return NotImplemented("ReadFile not implemented"); } /// \brief Write the given content to the file at the given location. @@ -63,7 +63,7 @@ class ICEBERG_EXPORT FileIO { /// file exists. /// \return void if the write succeeded, an error code if the write failed. virtual Status WriteFile(const std::string& file_location, std::string_view content) { - return NotImplementedError("WriteFile not implemented"); + return NotImplemented("WriteFile not implemented"); } /// \brief Delete a file at the given location. @@ -71,7 +71,7 @@ class ICEBERG_EXPORT FileIO { /// \param file_location The location of the file to delete. /// \return void if the delete succeeded, an error code if the delete failed. virtual Status DeleteFile(const std::string& file_location) { - return NotImplementedError("DeleteFile not implemented"); + return NotImplemented("DeleteFile not implemented"); } }; diff --git a/src/iceberg/result.h b/src/iceberg/result.h index 3571e0839..f2198e5f3 100644 --- a/src/iceberg/result.h +++ b/src/iceberg/result.h @@ -62,99 +62,28 @@ using Result = expected; using Status = Result; -/// \brief Create an unexpected error with kAlreadyExists -template -auto AlreadyExistsError(const std::format_string fmt, Args&&... args) - -> unexpected { - return unexpected({.kind = ErrorKind::kAlreadyExists, - .message = std::format(fmt, std::forward(args)...)}); -} - -/// \brief Create an unexpected error with kCommitStateUnknown -template -auto CommitStateUnknownError(const std::format_string fmt, Args&&... args) - -> unexpected { - return unexpected({.kind = ErrorKind::kCommitStateUnknown, - .message = std::format(fmt, std::forward(args)...)}); -} - -/// \brief Create an unexpected error with kInvalidArgument -template -auto InvalidArgumentError(const std::format_string fmt, Args&&... args) - -> unexpected { - return unexpected({.kind = ErrorKind::kInvalidArgument, - .message = std::format(fmt, std::forward(args)...)}); -} - -/// \brief Create an unexpected error with kInvalidExpression -template -auto InvalidExpressionError(const std::format_string fmt, Args&&... args) - -> unexpected { - return unexpected({.kind = ErrorKind::kInvalidExpression, - .message = std::format(fmt, std::forward(args)...)}); -} - -/// \brief Create an unexpected error with kInvalidSchema -template -auto InvalidSchemaError(const std::format_string fmt, Args&&... args) - -> unexpected { - return unexpected({.kind = ErrorKind::kInvalidSchema, - .message = std::format(fmt, std::forward(args)...)}); -} - -/// \brief Create an unexpected error with kIOError -template -auto IOError(const std::format_string fmt, Args&&... args) -> unexpected { - return unexpected({.kind = ErrorKind::kIOError, - .message = std::format(fmt, std::forward(args)...)}); -} - -/// \brief Create an unexpected error with kJsonParseError -template -auto JsonParseError(const std::format_string fmt, Args&&... args) - -> unexpected { - return unexpected({.kind = ErrorKind::kJsonParseError, - .message = std::format(fmt, std::forward(args)...)}); -} - -/// \brief Create an unexpected error with kNoSuchNamespace -template -auto NoSuchNamespaceError(const std::format_string fmt, Args&&... args) - -> unexpected { - return unexpected({.kind = ErrorKind::kNoSuchNamespace, - .message = std::format(fmt, std::forward(args)...)}); -} - -/// \brief Create an unexpected error with kNoSuchTable -template -auto NoSuchTableError(const std::format_string fmt, Args&&... args) - -> unexpected { - return unexpected({.kind = ErrorKind::kNoSuchTable, - .message = std::format(fmt, std::forward(args)...)}); -} - -/// \brief Create an unexpected error with kNotImplemented -template -auto NotImplementedError(const std::format_string fmt, Args&&... args) - -> unexpected { - return unexpected({.kind = ErrorKind::kNotImplemented, - .message = std::format(fmt, std::forward(args)...)}); -} - -/// \brief Create an unexpected error with kNotSupported -template -auto NotSupportedError(const std::format_string fmt, Args&&... args) - -> unexpected { - return unexpected({.kind = ErrorKind::kNotSupported, - .message = std::format(fmt, std::forward(args)...)}); -} - -/// \brief Create an unexpected error with kUnknownError -template -auto UnknownError(const std::format_string fmt, Args&&... args) - -> unexpected { - return unexpected({.kind = ErrorKind::kUnknownError, - .message = std::format(fmt, std::forward(args)...)}); -} +/// \brief Macro to define error creation functions +#define DEFINE_ERROR_FUNCTION(name) \ + template \ + auto name(const std::format_string fmt, Args&&... args) \ + -> unexpected { \ + return unexpected( \ + {ErrorKind::k##name, std::format(fmt, std::forward(args)...)}); \ + } + +DEFINE_ERROR_FUNCTION(AlreadyExists) +DEFINE_ERROR_FUNCTION(CommitStateUnknown) +DEFINE_ERROR_FUNCTION(InvalidArgument) +DEFINE_ERROR_FUNCTION(InvalidExpression) +DEFINE_ERROR_FUNCTION(InvalidSchema) +DEFINE_ERROR_FUNCTION(IOError) +DEFINE_ERROR_FUNCTION(JsonParseError) +DEFINE_ERROR_FUNCTION(NoSuchNamespace) +DEFINE_ERROR_FUNCTION(NoSuchTable) +DEFINE_ERROR_FUNCTION(NotImplemented) +DEFINE_ERROR_FUNCTION(NotSupported) +DEFINE_ERROR_FUNCTION(UnknownError) + +#undef DEFINE_ERROR_FUNCTION } // namespace iceberg diff --git a/src/iceberg/schema_internal.cc b/src/iceberg/schema_internal.cc index c3d562de9..621807278 100644 --- a/src/iceberg/schema_internal.cc +++ b/src/iceberg/schema_internal.cc @@ -20,12 +20,9 @@ #include "iceberg/schema_internal.h" #include -#include #include #include -#include - #include "iceberg/schema.h" #include "iceberg/type.h" #include "iceberg/util/macros.h" @@ -173,13 +170,13 @@ ArrowErrorCode ToArrowSchema(const Type& type, bool optional, std::string_view n Status ToArrowSchema(const Schema& schema, ArrowSchema* out) { if (out == nullptr) [[unlikely]] { - return InvalidArgumentError("Output Arrow schema cannot be null"); + return InvalidArgument("Output Arrow schema cannot be null"); } if (ArrowErrorCode errorCode = ToArrowSchema(schema, /*optional=*/false, /*name=*/"", /*field_id=*/std::nullopt, out); errorCode != NANOARROW_OK) { - return InvalidSchemaError( + return InvalidSchema( "Failed to convert Iceberg schema to Arrow schema, error code: {}", errorCode); } @@ -221,8 +218,8 @@ Result> FromArrowSchema(const ArrowSchema& schema) { ArrowSchemaView schema_view; if (auto error_code = ArrowSchemaViewInit(&schema_view, &schema, &arrow_error); error_code != NANOARROW_OK) { - return InvalidSchemaError("Failed to read Arrow schema, code: {}, message: {}", - error_code, arrow_error.message); + return InvalidSchema("Failed to read Arrow schema, code: {}, message: {}", error_code, + arrow_error.message); } switch (schema_view.type) { @@ -267,16 +264,16 @@ Result> FromArrowSchema(const ArrowSchema& schema) { return std::make_shared(); case NANOARROW_TYPE_TIME64: if (schema_view.time_unit != NANOARROW_TIME_UNIT_MICRO) { - return InvalidSchemaError("Unsupported time unit for Arrow time type: {}", - static_cast(schema_view.time_unit)); + return InvalidSchema("Unsupported time unit for Arrow time type: {}", + static_cast(schema_view.time_unit)); } return std::make_shared(); case NANOARROW_TYPE_TIMESTAMP: { bool with_timezone = schema_view.timezone != nullptr && std::strlen(schema_view.timezone) > 0; if (schema_view.time_unit != NANOARROW_TIME_UNIT_MICRO) { - return InvalidSchemaError("Unsupported time unit for Arrow timestamp type: {}", - static_cast(schema_view.time_unit)); + return InvalidSchema("Unsupported time unit for Arrow timestamp type: {}", + static_cast(schema_view.time_unit)); } if (with_timezone) { return std::make_shared(); @@ -293,15 +290,15 @@ Result> FromArrowSchema(const ArrowSchema& schema) { schema_view.extension_name.size_bytes); extension_name == kArrowUuidExtensionName) { if (schema_view.fixed_size != 16) { - return InvalidSchemaError("UUID type must have a fixed size of 16"); + return InvalidSchema("UUID type must have a fixed size of 16"); } return std::make_shared(); } return std::make_shared(schema_view.fixed_size); } default: - return InvalidSchemaError("Unsupported Arrow type: {}", - ArrowTypeString(schema_view.type)); + return InvalidSchema("Unsupported Arrow type: {}", + ArrowTypeString(schema_view.type)); } } @@ -322,7 +319,7 @@ Result> FromArrowSchema(const ArrowSchema& schema, ICEBERG_ASSIGN_OR_RAISE(auto type, FromArrowSchema(schema)); if (type->type_id() != TypeId::kStruct) { - return InvalidSchemaError("Arrow schema must be a struct type for Iceberg schema"); + return InvalidSchema("Arrow schema must be a struct type for Iceberg schema"); } auto& struct_type = static_cast(*type); diff --git a/src/iceberg/snapshot.h b/src/iceberg/snapshot.h index 7be33eb8f..924f52247 100644 --- a/src/iceberg/snapshot.h +++ b/src/iceberg/snapshot.h @@ -56,7 +56,7 @@ ICEBERG_EXPORT constexpr Result SnapshotRefTypeFromString( std::string_view str) noexcept { if (str == "branch") return SnapshotRefType::kBranch; if (str == "tag") return SnapshotRefType::kTag; - return InvalidArgumentError("Invalid snapshot reference type: {}", str); + return InvalidArgument("Invalid snapshot reference type: {}", str); } /// \brief A reference to a snapshot, either a branch or a tag. diff --git a/src/iceberg/sort_field.h b/src/iceberg/sort_field.h index 1470bfb0c..263bbc65a 100644 --- a/src/iceberg/sort_field.h +++ b/src/iceberg/sort_field.h @@ -57,7 +57,7 @@ ICEBERG_EXPORT constexpr Result SortDirectionFromString( std::string_view str) { if (str == "asc") return SortDirection::kAscending; if (str == "desc") return SortDirection::kDescending; - return InvalidArgumentError("Invalid SortDirection string: {}", str); + return InvalidArgument("Invalid SortDirection string: {}", str); } enum class NullOrder { @@ -81,7 +81,7 @@ ICEBERG_EXPORT constexpr std::string_view NullOrderToString(NullOrder null_order ICEBERG_EXPORT constexpr Result NullOrderFromString(std::string_view str) { if (str == "nulls-first") return NullOrder::kFirst; if (str == "nulls-last") return NullOrder::kLast; - return InvalidArgumentError("Invalid NullOrder string: {}", str); + return InvalidArgument("Invalid NullOrder string: {}", str); } /// \brief a field with its transform. diff --git a/src/iceberg/transform.cc b/src/iceberg/transform.cc index fb1205c7b..dc0529766 100644 --- a/src/iceberg/transform.cc +++ b/src/iceberg/transform.cc @@ -22,8 +22,6 @@ #include #include -#include - #include "iceberg/transform_function.h" #include "iceberg/type.h" @@ -121,15 +119,15 @@ Result> Transform::Bind( if (auto param = std::get_if(¶m_)) { return std::make_unique(source_type, *param); } - return InvalidArgumentError( - "Bucket requires int32 param, none found in transform '{}'", type_str); + return InvalidArgument("Bucket requires int32 param, none found in transform '{}'", + type_str); } case TransformType::kTruncate: { if (auto param = std::get_if(¶m_)) { return std::make_unique(source_type, *param); } - return InvalidArgumentError( + return InvalidArgument( "Truncate requires int32 param, none found in transform '{}'", type_str); } @@ -145,7 +143,7 @@ Result> Transform::Bind( return std::make_unique(source_type); default: - return NotSupportedError("Unsupported transform type: '{}'", type_str); + return NotSupported("Unsupported transform type: '{}'", type_str); } } @@ -209,7 +207,7 @@ Result> TransformFromString(std::string_view transfor } } - return InvalidArgumentError("Invalid Transform string: {}", transform_str); + return InvalidArgument("Invalid Transform string: {}", transform_str); } } // namespace iceberg diff --git a/src/iceberg/transform_function.cc b/src/iceberg/transform_function.cc index 8746020fa..6aa49bff6 100644 --- a/src/iceberg/transform_function.cc +++ b/src/iceberg/transform_function.cc @@ -27,14 +27,14 @@ IdentityTransform::IdentityTransform(std::shared_ptr const& source_type) : TransformFunction(TransformType::kIdentity, source_type) {} Result IdentityTransform::Transform(const ArrowArray& input) { - return NotImplementedError("IdentityTransform::Transform"); + return NotImplemented("IdentityTransform::Transform"); } Result> IdentityTransform::ResultType() const { auto src_type = source_type(); if (!src_type || !src_type->is_primitive()) { - return NotSupportedError("{} is not a valid input type for identity transform", - src_type ? src_type->ToString() : "null"); + return NotSupported("{} is not a valid input type for identity transform", + src_type ? src_type->ToString() : "null"); } return src_type; } @@ -44,11 +44,11 @@ BucketTransform::BucketTransform(std::shared_ptr const& source_type, : TransformFunction(TransformType::kBucket, source_type), num_buckets_(num_buckets) {} Result BucketTransform::Transform(const ArrowArray& input) { - return NotImplementedError("BucketTransform::Transform"); + return NotImplemented("BucketTransform::Transform"); } Result> BucketTransform::ResultType() const { - return NotImplementedError("BucketTransform::result_type"); + return NotImplemented("BucketTransform::result_type"); } TruncateTransform::TruncateTransform(std::shared_ptr const& source_type, @@ -56,66 +56,66 @@ TruncateTransform::TruncateTransform(std::shared_ptr const& source_type, : TransformFunction(TransformType::kTruncate, source_type), width_(width) {} Result TruncateTransform::Transform(const ArrowArray& input) { - return NotImplementedError("TruncateTransform::Transform"); + return NotImplemented("TruncateTransform::Transform"); } Result> TruncateTransform::ResultType() const { - return NotImplementedError("TruncateTransform::result_type"); + return NotImplemented("TruncateTransform::result_type"); } YearTransform::YearTransform(std::shared_ptr const& source_type) : TransformFunction(TransformType::kTruncate, source_type) {} Result YearTransform::Transform(const ArrowArray& input) { - return NotImplementedError("YearTransform::Transform"); + return NotImplemented("YearTransform::Transform"); } Result> YearTransform::ResultType() const { - return NotImplementedError("YearTransform::result_type"); + return NotImplemented("YearTransform::result_type"); } MonthTransform::MonthTransform(std::shared_ptr const& source_type) : TransformFunction(TransformType::kMonth, source_type) {} Result MonthTransform::Transform(const ArrowArray& input) { - return NotImplementedError("MonthTransform::Transform"); + return NotImplemented("MonthTransform::Transform"); } Result> MonthTransform::ResultType() const { - return NotImplementedError("MonthTransform::result_type"); + return NotImplemented("MonthTransform::result_type"); } DayTransform::DayTransform(std::shared_ptr const& source_type) : TransformFunction(TransformType::kDay, source_type) {} Result DayTransform::Transform(const ArrowArray& input) { - return NotImplementedError("DayTransform::Transform"); + return NotImplemented("DayTransform::Transform"); } Result> DayTransform::ResultType() const { - return NotImplementedError("DayTransform::result_type"); + return NotImplemented("DayTransform::result_type"); } HourTransform::HourTransform(std::shared_ptr const& source_type) : TransformFunction(TransformType::kHour, source_type) {} Result HourTransform::Transform(const ArrowArray& input) { - return NotImplementedError("HourTransform::Transform"); + return NotImplemented("HourTransform::Transform"); } Result> HourTransform::ResultType() const { - return NotImplementedError("HourTransform::result_type"); + return NotImplemented("HourTransform::result_type"); } VoidTransform::VoidTransform(std::shared_ptr const& source_type) : TransformFunction(TransformType::kVoid, source_type) {} Result VoidTransform::Transform(const ArrowArray& input) { - return NotImplementedError("VoidTransform::Transform"); + return NotImplemented("VoidTransform::Transform"); } Result> VoidTransform::ResultType() const { - return NotImplementedError("VoidTransform::result_type"); + return NotImplemented("VoidTransform::result_type"); } } // namespace iceberg diff --git a/test/json_internal_test.cc b/test/json_internal_test.cc index 20e379476..fc08d486b 100644 --- a/test/json_internal_test.cc +++ b/test/json_internal_test.cc @@ -22,8 +22,6 @@ #include #include -#include -#include #include #include "gmock/gmock.h" @@ -34,6 +32,7 @@ #include "iceberg/sort_order.h" #include "iceberg/transform.h" #include "iceberg/util/formatter.h" // IWYU pragma: keep +#include "iceberg/util/timepoint.h" #include "matchers.h" namespace iceberg { diff --git a/test/snapshot_test.cc b/test/snapshot_test.cc index a4895440f..a3c28f89f 100644 --- a/test/snapshot_test.cc +++ b/test/snapshot_test.cc @@ -19,7 +19,8 @@ #include "iceberg/snapshot.h" #include -#include + +#include "iceberg/util/timepoint.h" namespace iceberg { From ff33b9a40270c8481ae2744c3e0be9c217c82bbc Mon Sep 17 00:00:00 2001 From: Junwang Zhao Date: Sat, 19 Apr 2025 16:50:14 +0800 Subject: [PATCH 3/3] add inline to the macro Signed-off-by: Junwang Zhao --- src/iceberg/json_internal.cc | 4 ++-- src/iceberg/result.h | 3 ++- src/iceberg/table_metadata.cc | 17 ++++------------- test/metadata_serde_test.cc | 33 +++++++++++++++++---------------- 4 files changed, 25 insertions(+), 32 deletions(-) diff --git a/src/iceberg/json_internal.cc b/src/iceberg/json_internal.cc index 1336bd0ec..2e254e5d3 100644 --- a/src/iceberg/json_internal.cc +++ b/src/iceberg/json_internal.cc @@ -27,8 +27,6 @@ #include #include -#include -#include #include #include "iceberg/partition_field.h" @@ -39,11 +37,13 @@ #include "iceberg/snapshot.h" #include "iceberg/sort_order.h" #include "iceberg/statistics_file.h" +#include "iceberg/table.h" #include "iceberg/table_metadata.h" #include "iceberg/transform.h" #include "iceberg/type.h" #include "iceberg/util/formatter.h" // IWYU pragma: keep #include "iceberg/util/macros.h" +#include "iceberg/util/timepoint.h" namespace iceberg { diff --git a/src/iceberg/result.h b/src/iceberg/result.h index f2198e5f3..38d9e381f 100644 --- a/src/iceberg/result.h +++ b/src/iceberg/result.h @@ -65,7 +65,7 @@ using Status = Result; /// \brief Macro to define error creation functions #define DEFINE_ERROR_FUNCTION(name) \ template \ - auto name(const std::format_string fmt, Args&&... args) \ + inline auto name(const std::format_string fmt, Args&&... args) \ -> unexpected { \ return unexpected( \ {ErrorKind::k##name, std::format(fmt, std::forward(args)...)}); \ @@ -80,6 +80,7 @@ DEFINE_ERROR_FUNCTION(IOError) DEFINE_ERROR_FUNCTION(JsonParseError) DEFINE_ERROR_FUNCTION(NoSuchNamespace) DEFINE_ERROR_FUNCTION(NoSuchTable) +DEFINE_ERROR_FUNCTION(NotFound) DEFINE_ERROR_FUNCTION(NotImplemented) DEFINE_ERROR_FUNCTION(NotSupported) DEFINE_ERROR_FUNCTION(UnknownError) diff --git a/src/iceberg/table_metadata.cc b/src/iceberg/table_metadata.cc index 326e57422..5640939c2 100644 --- a/src/iceberg/table_metadata.cc +++ b/src/iceberg/table_metadata.cc @@ -23,8 +23,8 @@ #include #include -#include "iceberg/expected.h" #include "iceberg/partition_spec.h" +#include "iceberg/result.h" #include "iceberg/schema.h" #include "iceberg/sort_order.h" namespace iceberg { @@ -44,10 +44,7 @@ Result> TableMetadata::Schema() const { return schema->schema_id() == current_schema_id; }); if (iter == schemas.end()) { - return unexpected({ - .kind = ErrorKind::kNotFound, - .message = std::format("Current schema is not found"), - }); + return NotFound("Current schema is not found"); } return *iter; } @@ -57,10 +54,7 @@ Result> TableMetadata::PartitionSpec() const { return spec->spec_id() == default_spec_id; }); if (iter == partition_specs.end()) { - return unexpected({ - .kind = ErrorKind::kNotFound, - .message = std::format("Default partition spec is not found"), - }); + return NotFound("Default partition spec is not found"); } return *iter; } @@ -70,10 +64,7 @@ Result> TableMetadata::SortOrder() const { return order->order_id() == default_sort_order_id; }); if (iter == sort_orders.end()) { - return unexpected({ - .kind = ErrorKind::kNotFound, - .message = std::format("Default sort order is not found"), - }); + return NotFound("Default sort order is not found"); } return *iter; } diff --git a/test/metadata_serde_test.cc b/test/metadata_serde_test.cc index d187bafb1..4a78e8ce4 100644 --- a/test/metadata_serde_test.cc +++ b/test/metadata_serde_test.cc @@ -166,22 +166,23 @@ TEST_F(MetadataSerdeTest, DeserializeV2Valid) { EXPECT_EQ(metadata->current_snapshot_id, 3055729675574597004); // Compare snapshots - std::vector expected_snapshots{{ - .snapshot_id = 3051729675574597004, - .sequence_number = 0, - .timestamp_ms = 1515100955770, - .manifest_list = "s3://a/b/1.avro", - .summary = {{"operation", "append"}}, - }, - { - .snapshot_id = 3055729675574597004, - .parent_snapshot_id = 3051729675574597004, - .sequence_number = 1, - .timestamp_ms = 1555100955770, - .manifest_list = "s3://a/b/2.avro", - .summary = {{"operation", "append"}}, - .schema_id = 1, - }}; + std::vector expected_snapshots{ + { + .snapshot_id = 3051729675574597004, + .sequence_number = 0, + .timestamp_ms = TimePointMsFromUnixMs(1515100955770).value(), + .manifest_list = "s3://a/b/1.avro", + .summary = {{"operation", "append"}}, + }, + { + .snapshot_id = 3055729675574597004, + .parent_snapshot_id = 3051729675574597004, + .sequence_number = 1, + .timestamp_ms = TimePointMsFromUnixMs(1555100955770).value(), + .manifest_list = "s3://a/b/2.avro", + .summary = {{"operation", "append"}}, + .schema_id = 1, + }}; EXPECT_EQ(metadata->snapshots.size(), expected_snapshots.size()); for (size_t i = 0; i < expected_snapshots.size(); ++i) { EXPECT_EQ(*metadata->snapshots[i], expected_snapshots[i]);