refine error handling

Author: wgtmac

src/iceberg/avro/avro_stream_internal.cc

@@ -66,8 +66,9 @@ bool AvroInputStream::next(const uint8_t** data, size_t* len) {
 }
 
 void AvroInputStream::backup(size_t len) {
-  ICEBERG_CHECK(len <= buffer_pos_, "Cannot backup {} bytes, only {} bytes available",
-                len, buffer_pos_);
+  ICEBERG_CHECK_OR_DIE(len <= buffer_pos_,
+                       "Cannot backup {} bytes, only {} bytes available", len,
+                       buffer_pos_);
 
   buffer_pos_ -= len;
   byte_count_ -= len;
@@ -88,7 +89,8 @@ size_t AvroInputStream::byteCount() const { return byte_count_; }
 
 void AvroInputStream::seek(int64_t position) {
   auto status = input_stream_->Seek(position);
-  ICEBERG_CHECK(status.ok(), "Failed to seek to {}, got {}", position, status.ToString());
+  ICEBERG_CHECK_OR_DIE(status.ok(), "Failed to seek to {}, got {}", position,
+                       status.ToString());
 
   buffer_pos_ = 0;
   available_bytes_ = 0;
@@ -116,8 +118,9 @@ bool AvroOutputStream::next(uint8_t** data, size_t* len) {
 }
 
 void AvroOutputStream::backup(size_t len) {
-  ICEBERG_CHECK(len <= buffer_pos_, "Cannot backup {} bytes, only {} bytes available",
-                len, buffer_pos_);
+  ICEBERG_CHECK_OR_DIE(len <= buffer_pos_,
+                       "Cannot backup {} bytes, only {} bytes available", len,
+                       buffer_pos_);
   buffer_pos_ -= len;
 }
 
@@ -126,12 +129,12 @@ uint64_t AvroOutputStream::byteCount() const { return flushed_bytes_ + buffer_po
 void AvroOutputStream::flush() {
   if (buffer_pos_ > 0) {
     auto status = output_stream_->Write(buffer_.data(), buffer_pos_);
-    ICEBERG_CHECK(status.ok(), "Write failed {}", status.ToString());
+    ICEBERG_CHECK_OR_DIE(status.ok(), "Write failed {}", status.ToString());
     flushed_bytes_ += buffer_pos_;
     buffer_pos_ = 0;
   }
   auto status = output_stream_->Flush();
-  ICEBERG_CHECK(status.ok(), "Flush failed {}", status.ToString());
+  ICEBERG_CHECK_OR_DIE(status.ok(), "Flush failed {}", status.ToString());
 }
 
 const std::shared_ptr<::arrow::io::OutputStream>& AvroOutputStream::arrow_output_stream()

src/iceberg/exception.h

@@ -44,7 +44,7 @@ class ICEBERG_EXPORT ExpressionError : public IcebergError {
   explicit ExpressionError(const std::string& what) : IcebergError(what) {}
 };
 
-#define ICEBERG_CHECK(condition, ...)                        \
+#define ICEBERG_CHECK_OR_DIE(condition, ...)                 \
   do {                                                       \
     if (!(condition)) [[unlikely]] {                         \
       throw iceberg::IcebergError(std::format(__VA_ARGS__)); \

src/iceberg/table_metadata.cc

@@ -69,9 +69,9 @@ Result<std::shared_ptr<Schema>> TableMetadata::Schema() const {
 }
 
 Result<std::shared_ptr<Schema>> TableMetadata::SchemaById(
-    const std::optional<int32_t>& schema_id) const {
+    std::optional<int32_t> schema_id) const {
   auto iter = std::ranges::find_if(schemas, [schema_id](const auto& schema) {
-    return schema->schema_id() == schema_id;
+    return schema != nullptr && schema->schema_id() == schema_id;
   });
   if (iter == schemas.end()) {
     return NotFound("Schema with ID {} is not found", schema_id.value_or(-1));
@@ -81,7 +81,7 @@ Result<std::shared_ptr<Schema>> TableMetadata::SchemaById(
 
 Result<std::shared_ptr<PartitionSpec>> TableMetadata::PartitionSpec() const {
   auto iter = std::ranges::find_if(partition_specs, [this](const auto& spec) {
-    return spec->spec_id() == default_spec_id;
+    return spec != nullptr && spec->spec_id() == default_spec_id;
   });
   if (iter == partition_specs.end()) {
     return NotFound("Default partition spec is not found");
@@ -91,7 +91,7 @@ Result<std::shared_ptr<PartitionSpec>> TableMetadata::PartitionSpec() const {
 
 Result<std::shared_ptr<SortOrder>> TableMetadata::SortOrder() const {
   auto iter = std::ranges::find_if(sort_orders, [this](const auto& order) {
-    return order->order_id() == default_sort_order_id;
+    return order != nullptr && order->order_id() == default_sort_order_id;
   });
   if (iter == sort_orders.end()) {
     return NotFound("Default sort order is not found");
@@ -105,7 +105,7 @@ Result<std::shared_ptr<Snapshot>> TableMetadata::Snapshot() const {
 
 Result<std::shared_ptr<Snapshot>> TableMetadata::SnapshotById(int64_t snapshot_id) const {
   auto iter = std::ranges::find_if(snapshots, [snapshot_id](const auto& snapshot) {
-    return snapshot->snapshot_id == snapshot_id;
+    return snapshot != nullptr && snapshot->snapshot_id == snapshot_id;
   });
   if (iter == snapshots.end()) {
     return NotFound("Snapshot with ID {} is not found", snapshot_id);

src/iceberg/table_metadata.h

@@ -128,7 +128,7 @@ struct ICEBERG_EXPORT TableMetadata {
   Result<std::shared_ptr<iceberg::Schema>> Schema() const;
   /// \brief Get the current schema by ID, return NotFoundError if not found
   Result<std::shared_ptr<iceberg::Schema>> SchemaById(
-      const std::optional<int32_t>& schema_id) const;
+      std::optional<int32_t> schema_id) const;
   /// \brief Get the current partition spec, return NotFoundError if not found
   Result<std::shared_ptr<iceberg::PartitionSpec>> PartitionSpec() const;
   /// \brief Get the current sort order, return NotFoundError if not found

src/iceberg/test/snapshot_util_test.cc

@@ -337,11 +337,11 @@ TEST_F(SnapshotUtilTest, SnapshotAfter) {
 TEST_F(SnapshotUtilTest, SnapshotIdAsOfTime) {
   // Test with timestamp before any snapshot
   auto early_timestamp = base_timestamp_ - std::chrono::milliseconds(1000);
-  auto snapshot_id = SnapshotUtil::NullableSnapshotIdAsOfTime(*table_, early_timestamp);
+  auto snapshot_id = SnapshotUtil::OptionalSnapshotIdAsOfTime(*table_, early_timestamp);
   EXPECT_FALSE(snapshot_id.has_value());
 
   // Test with timestamp at base snapshot
-  auto snapshot_id1 = SnapshotUtil::NullableSnapshotIdAsOfTime(*table_, base_timestamp_);
+  auto snapshot_id1 = SnapshotUtil::OptionalSnapshotIdAsOfTime(*table_, base_timestamp_);
   ASSERT_TRUE(snapshot_id1.has_value());
   EXPECT_EQ(snapshot_id1.value(), base_snapshot_id_);
 
@@ -357,21 +357,18 @@ TEST_F(SnapshotUtilTest, LatestSnapshot) {
   ICEBERG_UNWRAP_OR_FAIL(
       auto main_snapshot,
       SnapshotUtil::LatestSnapshot(*table_, std::string(SnapshotRef::kMainBranch)));
-  ASSERT_TRUE(main_snapshot.has_value());
-  EXPECT_EQ(main_snapshot.value()->snapshot_id, main2_snapshot_id_);
+  EXPECT_EQ(main_snapshot->snapshot_id, main2_snapshot_id_);
 
   // Test branch
   ICEBERG_UNWRAP_OR_FAIL(auto branch_snapshot,
                          SnapshotUtil::LatestSnapshot(*table_, "b1"));
-  ASSERT_TRUE(branch_snapshot.has_value());
-  EXPECT_EQ(branch_snapshot.value()->snapshot_id, branch_snapshot_id_);
+  EXPECT_EQ(branch_snapshot->snapshot_id, branch_snapshot_id_);
 
   // Test non-existing branch
   ICEBERG_UNWRAP_OR_FAIL(auto non_existing,
                          SnapshotUtil::LatestSnapshot(*table_, "non-existing"));
-  ASSERT_TRUE(non_existing.has_value());
   // Should return current snapshot
-  EXPECT_EQ(non_existing.value()->snapshot_id, main2_snapshot_id_);
+  EXPECT_EQ(non_existing->snapshot_id, main2_snapshot_id_);
 }
 
 }  // namespace iceberg

src/iceberg/type.cc

@@ -141,9 +141,9 @@ StructType::InitFieldByLowerCaseName(const StructType& self) {
 }
 
 ListType::ListType(SchemaField element) : element_(std::move(element)) {
-  ICEBERG_CHECK(element_.name() == kElementName,
-                "ListType: child field name should be '{}', was '{}'", kElementName,
-                element_.name());
+  ICEBERG_CHECK_OR_DIE(element_.name() == kElementName,
+                       "ListType: child field name should be '{}', was '{}'",
+                       kElementName, element_.name());
 }
 
 ListType::ListType(int32_t field_id, std::shared_ptr<Type> type, bool optional)
@@ -200,12 +200,12 @@ bool ListType::Equals(const Type& other) const {
 
 MapType::MapType(SchemaField key, SchemaField value)
     : fields_{std::move(key), std::move(value)} {
-  ICEBERG_CHECK(this->key().name() == kKeyName,
-                "MapType: key field name should be '{}', was '{}'", kKeyName,
-                this->key().name());
-  ICEBERG_CHECK(this->value().name() == kValueName,
-                "MapType: value field name should be '{}', was '{}'", kValueName,
-                this->value().name());
+  ICEBERG_CHECK_OR_DIE(this->key().name() == kKeyName,
+                       "MapType: key field name should be '{}', was '{}'", kKeyName,
+                       this->key().name());
+  ICEBERG_CHECK_OR_DIE(this->value().name() == kValueName,
+                       "MapType: value field name should be '{}', was '{}'", kValueName,
+                       this->value().name());
 }
 
 const SchemaField& MapType::key() const { return fields_[0]; }
@@ -292,8 +292,8 @@ bool DoubleType::Equals(const Type& other) const { return other.type_id() == kTy
 
 DecimalType::DecimalType(int32_t precision, int32_t scale)
     : precision_(precision), scale_(scale) {
-  ICEBERG_CHECK(precision >= 0 && precision <= kMaxPrecision,
-                "DecimalType: precision must be in [0, 38], was {}", precision);
+  ICEBERG_CHECK_OR_DIE(precision >= 0 && precision <= kMaxPrecision,
+                       "DecimalType: precision must be in [0, 38], was {}", precision);
 }
 
 int32_t DecimalType::precision() const { return precision_; }
@@ -341,7 +341,7 @@ std::string UuidType::ToString() const { return "uuid"; }
 bool UuidType::Equals(const Type& other) const { return other.type_id() == kTypeId; }
 
 FixedType::FixedType(int32_t length) : length_(length) {
-  ICEBERG_CHECK(length >= 0, "FixedType: length must be >= 0, was {}", length);
+  ICEBERG_CHECK_OR_DIE(length >= 0, "FixedType: length must be >= 0, was {}", length);
 }
 
 int32_t FixedType::length() const { return length_; }

src/iceberg/util/decimal.cc

@@ -300,8 +300,8 @@ bool RescaleWouldCauseDataLoss(const Decimal& value, int32_t delta_scale,
 
 Decimal::Decimal(std::string_view str) {
   auto result = Decimal::FromString(str);
-  ICEBERG_CHECK(result, "Failed to parse Decimal from string: {}, error: {}", str,
-                result.error().message);
+  ICEBERG_CHECK_OR_DIE(result, "Failed to parse Decimal from string: {}, error: {}", str,
+                       result.error().message);
   *this = std::move(result.value());
 }
 

src/iceberg/util/macros.h

@@ -42,8 +42,25 @@
   ICEBERG_ASSIGN_OR_RAISE_IMPL(ICEBERG_ASSIGN_OR_RAISE_NAME(result_, __COUNTER__), lhs, \
                                rexpr)
 
+// Macro for debug checks
 #define ICEBERG_DCHECK(expr, message) assert((expr) && (message))
 
+// Macro for precondition checks, usually used for function arguments
+#define ICEBERG_PRECHECK(expr, ...)        \
+  do {                                     \
+    if (!(expr)) [[unlikely]] {            \
+      return InvalidArgument(__VA_ARGS__); \
+    }                                      \
+  } while (0)
+
+// Macro for state checks, usually used for unexpected states
+#define ICEBERG_CHECK(expr, ...)   \
+  do {                             \
+    if (!(expr)) [[unlikely]] {    \
+      return Invalid(__VA_ARGS__); \
+    }                              \
+  } while (0)
+
 #define ERROR_TO_EXCEPTION(error)                             \
   if (error.kind == iceberg::ErrorKind::kInvalidExpression) { \
     throw iceberg::ExpressionError(error.message);            \