Skip to content

Commit 897a473

Browse files
wgtmacwgtmac
authored
Merge branch 'main' into feat/table-requirements
2 parents 6af2034 + aabf3d0 commit 897a473

13 files changed

+199
-229
lines changed

‎src/iceberg/pending_update.h‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@
2525
#include "iceberg/iceberg_export.h"
2626
#include "iceberg/result.h"
2727
#include "iceberg/type_fwd.h"
28+
#include "iceberg/util/error_collector.h"
2829

2930
namespace iceberg {
3031

@@ -74,7 +75,7 @@ class ICEBERG_EXPORT PendingUpdate {
7475
///
7576
/// \tparam T The type of result returned by Apply()
7677
template <typename T>
77-
class ICEBERG_EXPORT PendingUpdateTyped : public PendingUpdate {
78+
class ICEBERG_EXPORT PendingUpdateTyped : public PendingUpdate, public ErrorCollector {
7879
public:
7980
~PendingUpdateTyped() override = default;
8081

‎src/iceberg/table_metadata.cc‎

Lines changed: 3 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -275,9 +275,6 @@ struct TableMetadataBuilder::Impl {
275275
// Change tracking
276276
std::vector<std::unique_ptr<TableUpdate>> changes;
277277

278-
// Error collection (since methods return *this and cannot throw)
279-
std::vector<Error> errors;
280-
281278
// Metadata location tracking
282279
std::optional<std::string> metadata_location;
283280
std::optional<std::string> previous_metadata_location;
@@ -348,8 +345,7 @@ TableMetadataBuilder& TableMetadataBuilder::AssignUUID(std::string_view uuid) {
348345

349346
// Validation: UUID cannot be empty
350347
if (uuid_str.empty()) {
351-
impl_->errors.emplace_back(ErrorKind::kInvalidArgument, "Cannot assign empty UUID");
352-
return *this;
348+
return AddError(ErrorKind::kInvalidArgument, "Cannot assign empty UUID");
353349
}
354350

355351
// Check if UUID is already set to the same value (no-op)
@@ -451,7 +447,7 @@ TableMetadataBuilder& TableMetadataBuilder::RemoveSnapshots(
451447
throw IcebergError(std::format("{} not implemented", __FUNCTION__));
452448
}
453449

454-
TableMetadataBuilder& TableMetadataBuilder::suppressHistoricalSnapshots() {
450+
TableMetadataBuilder& TableMetadataBuilder::SuppressHistoricalSnapshots() {
455451
throw IcebergError(std::format("{} not implemented", __FUNCTION__));
456452
}
457453

@@ -499,13 +495,7 @@ TableMetadataBuilder& TableMetadataBuilder::RemoveEncryptionKey(std::string_view
499495

500496
Result<std::unique_ptr<TableMetadata>> TableMetadataBuilder::Build() {
501497
// 1. Check for accumulated errors
502-
if (!impl_->errors.empty()) {
503-
std::string error_msg = "Failed to build TableMetadata due to validation errors:\n";
504-
for (const auto& [kind, message] : impl_->errors) {
505-
error_msg += " - " + message + "\n";
506-
}
507-
return CommitFailed("{}", error_msg);
508-
}
498+
ICEBERG_RETURN_UNEXPECTED(CheckErrors());
509499

510500
// 2. Validate metadata consistency through TableMetadata#Validate
511501

‎src/iceberg/table_metadata.h‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@
3030

3131
#include "iceberg/iceberg_export.h"
3232
#include "iceberg/type_fwd.h"
33+
#include "iceberg/util/error_collector.h"
3334
#include "iceberg/util/lazy.h"
3435
#include "iceberg/util/timepoint.h"
3536

@@ -193,7 +194,7 @@ ICEBERG_EXPORT std::string ToString(const MetadataLogEntry& entry);
193194
/// If a modification violates Iceberg table constraints (e.g., setting a current
194195
/// schema ID that does not exist), an error will be recorded and returned when
195196
/// Build() is called.
196-
class ICEBERG_EXPORT TableMetadataBuilder {
197+
class ICEBERG_EXPORT TableMetadataBuilder : public ErrorCollector {
197198
public:
198199
/// \brief Create a builder for a new table
199200
///
@@ -353,7 +354,7 @@ class ICEBERG_EXPORT TableMetadataBuilder {
353354
/// RemoveSnapshot changes are created. A snapshot is historical if no ref directly
354355
/// references its ID.
355356
/// \return Reference to this builder for method chaining
356-
TableMetadataBuilder& suppressHistoricalSnapshots();
357+
TableMetadataBuilder& SuppressHistoricalSnapshots();
357358

358359
/// \brief Set table statistics
359360
///

‎src/iceberg/test/CMakeLists.txt‎

Lines changed: 4 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -81,14 +81,12 @@ add_iceberg_test(schema_test
8181
add_iceberg_test(table_test
8282
SOURCES
8383
json_internal_test.cc
84-
pending_update_test.cc
8584
schema_json_test.cc
8685
table_test.cc
8786
table_metadata_builder_test.cc
8887
table_requirement_test.cc
8988
table_requirements_test.cc
90-
table_update_test.cc
91-
test_common.cc)
89+
table_update_test.cc)
9290

9391
add_iceberg_test(expression_test
9492
SOURCES
@@ -100,7 +98,6 @@ add_iceberg_test(expression_test
10098

10199
add_iceberg_test(json_serde_test
102100
SOURCES
103-
test_common.cc
104101
json_internal_test.cc
105102
metadata_serde_test.cc
106103
schema_json_test.cc)
@@ -128,8 +125,7 @@ if(ICEBERG_BUILD_BUNDLE)
128125
avro_schema_test.cc
129126
avro_stream_test.cc
130127
manifest_list_versions_test.cc
131-
manifest_writer_versions_test.cc
132-
test_common.cc)
128+
manifest_writer_versions_test.cc)
133129

134130
add_iceberg_test(arrow_test
135131
USE_BUNDLE
@@ -140,18 +136,13 @@ if(ICEBERG_BUILD_BUNDLE)
140136
metadata_io_test.cc
141137
struct_like_test.cc)
142138

143-
add_iceberg_test(catalog_test
144-
USE_BUNDLE
145-
SOURCES
146-
test_common.cc
147-
in_memory_catalog_test.cc)
139+
add_iceberg_test(catalog_test USE_BUNDLE SOURCES in_memory_catalog_test.cc)
148140

149141
add_iceberg_test(eval_expr_test
150142
USE_BUNDLE
151143
SOURCES
152144
eval_expr_test.cc
153-
evaluator_test.cc
154-
test_common.cc)
145+
evaluator_test.cc)
155146

156147
add_iceberg_test(parquet_test
157148
USE_BUNDLE

‎src/iceberg/test/in_memory_catalog_test.cc‎

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -31,15 +31,14 @@
3131
#include "iceberg/table_metadata.h"
3232
#include "iceberg/test/matchers.h"
3333
#include "iceberg/test/mock_catalog.h"
34-
#include "iceberg/test/test_common.h"
34+
#include "iceberg/test/test_resource.h"
3535

3636
namespace iceberg {
3737

3838
class InMemoryCatalogTest : public ::testing::Test {
3939
protected:
4040
void SetUp() override {
41-
file_io_ = std::make_shared<iceberg::arrow::ArrowFileSystemFileIO>(
42-
std::make_shared<::arrow::fs::LocalFileSystem>());
41+
file_io_ = arrow::ArrowFileSystemFileIO::MakeLocalFileIO();
4342
std::unordered_map<std::string, std::string> properties = {{"prop1", "val1"}};
4443
catalog_ = std::make_shared<InMemoryCatalog>("test_catalog", file_io_,
4544
"/tmp/warehouse/", properties);
@@ -103,8 +102,8 @@ TEST_F(InMemoryCatalogTest, TableExists) {
103102
TEST_F(InMemoryCatalogTest, RegisterTable) {
104103
TableIdentifier tableIdent{.ns = {}, .name = "t1"};
105104

106-
std::unique_ptr<TableMetadata> metadata;
107-
ASSERT_NO_FATAL_FAILURE(ReadTableMetadata("TableMetadataV2Valid.json", &metadata));
105+
ICEBERG_UNWRAP_OR_FAIL(auto metadata,
106+
ReadTableMetadataFromResource("TableMetadataV2Valid.json"));
108107

109108
auto table_location = GenerateTestTableLocation(tableIdent.name);
110109
auto metadata_location = std::format("{}v1.metadata.json", table_location);

‎src/iceberg/test/meson.build‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,6 @@ iceberg_tests = {
5252
'table_requirement_test.cc',
5353
'table_test.cc',
5454
'table_update_test.cc',
55-
'test_common.cc',
5655
),
5756
},
5857
'expression_test': {
@@ -69,7 +68,6 @@ iceberg_tests = {
6968
'json_internal_test.cc',
7069
'metadata_serde_test.cc',
7170
'schema_json_test.cc',
72-
'test_common.cc',
7371
),
7472
},
7573
'util_test': {

‎src/iceberg/test/metadata_serde_test.cc‎

Lines changed: 13 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@
3232
#include "iceberg/statistics_file.h"
3333
#include "iceberg/table_metadata.h"
3434
#include "iceberg/test/matchers.h"
35-
#include "iceberg/test/test_common.h"
35+
#include "iceberg/test/test_resource.h"
3636
#include "iceberg/transform.h"
3737
#include "iceberg/type.h"
3838

@@ -42,7 +42,7 @@ namespace {
4242

4343
void ReadTableMetadataExpectError(const std::string& file_name,
4444
const std::string& expected_error_substr) {
45-
auto result = ReadTableMetadata(file_name);
45+
auto result = ReadTableMetadataFromResource(file_name);
4646
ASSERT_FALSE(result.has_value()) << "Expected parsing to fail for " << file_name;
4747
EXPECT_THAT(result, HasErrorMessage(expected_error_substr));
4848
}
@@ -90,8 +90,8 @@ void AssertSnapshotById(const TableMetadata& metadata, int64_t snapshot_id,
9090
} // namespace
9191

9292
TEST(MetadataSerdeTest, DeserializeV1Valid) {
93-
std::unique_ptr<TableMetadata> metadata;
94-
ASSERT_NO_FATAL_FAILURE(ReadTableMetadata("TableMetadataV1Valid.json", &metadata));
93+
ICEBERG_UNWRAP_OR_FAIL(auto metadata,
94+
ReadTableMetadataFromResource("TableMetadataV1Valid.json"));
9595

9696
auto expected_schema = std::make_shared<Schema>(
9797
std::vector<SchemaField>{SchemaField::MakeRequired(1, "x", int64()),
@@ -132,8 +132,8 @@ TEST(MetadataSerdeTest, DeserializeV1Valid) {
132132
}
133133

134134
TEST(MetadataSerdeTest, DeserializeV2Valid) {
135-
std::unique_ptr<TableMetadata> metadata;
136-
ASSERT_NO_FATAL_FAILURE(ReadTableMetadata("TableMetadataV2Valid.json", &metadata));
135+
ICEBERG_UNWRAP_OR_FAIL(auto metadata,
136+
ReadTableMetadataFromResource("TableMetadataV2Valid.json"));
137137

138138
auto expected_schema_1 = std::make_shared<Schema>(
139139
std::vector<SchemaField>{SchemaField(/*field_id=*/1, "x", int64(),
@@ -224,9 +224,8 @@ TEST(MetadataSerdeTest, DeserializeV2Valid) {
224224
}
225225

226226
TEST(MetadataSerdeTest, DeserializeV2ValidMinimal) {
227-
std::unique_ptr<TableMetadata> metadata;
228-
ASSERT_NO_FATAL_FAILURE(
229-
ReadTableMetadata("TableMetadataV2ValidMinimal.json", &metadata));
227+
ICEBERG_UNWRAP_OR_FAIL(
228+
auto metadata, ReadTableMetadataFromResource("TableMetadataV2ValidMinimal.json"));
230229

231230
auto expected_schema = std::make_shared<Schema>(
232231
std::vector<SchemaField>{SchemaField::MakeRequired(1, "x", int64()),
@@ -281,9 +280,8 @@ TEST(MetadataSerdeTest, DeserializeV2ValidMinimal) {
281280
}
282281

283282
TEST(MetadataSerdeTest, DeserializeStatisticsFiles) {
284-
std::unique_ptr<TableMetadata> metadata;
285-
ASSERT_NO_FATAL_FAILURE(
286-
ReadTableMetadata("TableMetadataStatisticsFiles.json", &metadata));
283+
ICEBERG_UNWRAP_OR_FAIL(
284+
auto metadata, ReadTableMetadataFromResource("TableMetadataStatisticsFiles.json"));
287285

288286
auto expected_schema = std::make_shared<Schema>(
289287
std::vector<SchemaField>{SchemaField(/*field_id=*/1, "x", int64(),
@@ -353,9 +351,9 @@ TEST(MetadataSerdeTest, DeserializeStatisticsFiles) {
353351
}
354352

355353
TEST(MetadataSerdeTest, DeserializePartitionStatisticsFiles) {
356-
std::unique_ptr<TableMetadata> metadata;
357-
ASSERT_NO_FATAL_FAILURE(
358-
ReadTableMetadata("TableMetadataPartitionStatisticsFiles.json", &metadata));
354+
ICEBERG_UNWRAP_OR_FAIL(
355+
auto metadata,
356+
ReadTableMetadataFromResource("TableMetadataPartitionStatisticsFiles.json"));
359357

360358
TableMetadata expected{
361359
.format_version = 2,

‎src/iceberg/test/pending_update_test.cc‎

Lines changed: 0 additions & 99 deletions
This file was deleted.

‎src/iceberg/test/table_test.cc‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -32,13 +32,14 @@
3232
#include "iceberg/schema.h"
3333
#include "iceberg/snapshot.h"
3434
#include "iceberg/table_metadata.h"
35-
#include "iceberg/test/test_common.h"
35+
#include "iceberg/test/matchers.h"
36+
#include "iceberg/test/test_resource.h"
3637

3738
namespace iceberg {
3839

3940
TEST(Table, TableV1) {
40-
std::unique_ptr<TableMetadata> metadata;
41-
ASSERT_NO_FATAL_FAILURE(ReadTableMetadata("TableMetadataV1Valid.json", &metadata));
41+
ICEBERG_UNWRAP_OR_FAIL(auto metadata,
42+
ReadTableMetadataFromResource("TableMetadataV1Valid.json"));
4243
TableIdentifier tableIdent{.ns = {}, .name = "test_table_v1"};
4344
Table table(tableIdent, std::move(metadata), "s3://bucket/test/location/meta/", nullptr,
4445
nullptr);
@@ -76,8 +77,8 @@ TEST(Table, TableV1) {
7677
}
7778

7879
TEST(Table, TableV2) {
79-
std::unique_ptr<TableMetadata> metadata;
80-
ASSERT_NO_FATAL_FAILURE(ReadTableMetadata("TableMetadataV2Valid.json", &metadata));
80+
ICEBERG_UNWRAP_OR_FAIL(auto metadata,
81+
ReadTableMetadataFromResource("TableMetadataV2Valid.json"));
8182
TableIdentifier tableIdent{.ns = {}, .name = "test_table_v2"};
8283

8384
Table table(tableIdent, std::move(metadata), "s3://bucket/test/location/meta/", nullptr,

0 commit comments

Comments
 (0)