Skip to content

Commit 607461a

Browse files
HeartLinkedHeartLinked
committed
fix review
1 parent d0ef9f3 commit 607461a

File tree

6 files changed

+58
-48
lines changed

6 files changed

+58
-48
lines changed

‎src/iceberg/CMakeLists.txt‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -84,11 +84,11 @@ set(ICEBERG_SOURCES
8484
update/expire_snapshots.cc
8585
update/pending_update.cc
8686
update/snapshot_update.cc
87+
update/update_location.cc
8788
update/update_partition_spec.cc
8889
update/update_properties.cc
8990
update/update_schema.cc
9091
update/update_sort_order.cc
91-
update/update_location.cc
9292
util/bucket_util.cc
9393
util/content_file_util.cc
9494
util/conversions.cc

‎src/iceberg/test/CMakeLists.txt‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -172,11 +172,11 @@ if(ICEBERG_BUILD_BUNDLE)
172172
SOURCES
173173
expire_snapshots_test.cc
174174
transaction_test.cc
175+
update_location_test.cc
175176
update_partition_spec_test.cc
176177
update_properties_test.cc
177178
update_schema_test.cc
178-
update_sort_order_test.cc
179-
update_location_test.cc)
179+
update_sort_order_test.cc)
180180

181181
add_iceberg_test(data_writer_test USE_BUNDLE SOURCES data_writer_test.cc)
182182

‎src/iceberg/test/update_location_test.cc‎

Lines changed: 51 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -36,66 +36,51 @@ namespace iceberg {
3636
class UpdateLocationTest : public UpdateTestBase {};
3737

3838
TEST_F(UpdateLocationTest, SetLocationSuccess) {
39-
ICEBERG_UNWRAP_OR_FAIL(auto update, table_->NewUpdateLocation());
4039
const std::string new_location = "/warehouse/new_location";
41-
update->SetLocation(new_location);
4240

43-
ICEBERG_UNWRAP_OR_FAIL(auto result, update->Apply());
44-
EXPECT_EQ(result, new_location);
45-
}
41+
// Create metadata directory for the new location
42+
auto arrow_fs = std::dynamic_pointer_cast<::arrow::fs::internal::MockFileSystem>(
43+
static_cast<arrow::ArrowFileSystemFileIO&>(*file_io_).fs());
44+
ASSERT_TRUE(arrow_fs != nullptr);
45+
ASSERT_TRUE(arrow_fs->CreateDir(new_location + "/metadata").ok());
4646

47-
TEST_F(UpdateLocationTest, SetLocationMultipleTimes) {
48-
// Test that setting location multiple times uses the last value
4947
ICEBERG_UNWRAP_OR_FAIL(auto update, table_->NewUpdateLocation());
50-
update->SetLocation("/warehouse/first_location")
51-
.SetLocation("/warehouse/second_location")
52-
.SetLocation("/warehouse/final_location");
48+
update->SetLocation(new_location);
5349

5450
ICEBERG_UNWRAP_OR_FAIL(auto result, update->Apply());
55-
EXPECT_EQ(result, "/warehouse/final_location");
56-
}
57-
58-
TEST_F(UpdateLocationTest, SetEmptyLocation) {
59-
ICEBERG_UNWRAP_OR_FAIL(auto update, table_->NewUpdateLocation());
60-
update->SetLocation("");
61-
62-
auto result = update->Apply();
63-
EXPECT_THAT(result, IsError(ErrorKind::kValidationFailed));
64-
EXPECT_THAT(result, HasErrorMessage("Location cannot be empty"));
65-
}
66-
67-
TEST_F(UpdateLocationTest, ApplyWithoutSettingLocation) {
68-
ICEBERG_UNWRAP_OR_FAIL(auto update, table_->NewUpdateLocation());
51+
EXPECT_EQ(result, new_location);
6952

70-
auto result = update->Apply();
71-
EXPECT_THAT(result, IsError(ErrorKind::kInvalidArgument));
72-
EXPECT_THAT(result, HasErrorMessage("Location must be set before applying"));
53+
// Commit and verify the location was persisted
54+
EXPECT_THAT(update->Commit(), IsOk());
55+
ICEBERG_UNWRAP_OR_FAIL(auto reloaded, catalog_->LoadTable(table_ident_));
56+
EXPECT_EQ(reloaded->location(), new_location);
7357
}
7458

75-
TEST_F(UpdateLocationTest, CommitSuccess) {
76-
// Test empty commit (should fail since location is not set)
77-
ICEBERG_UNWRAP_OR_FAIL(auto empty_update, table_->NewUpdateLocation());
78-
auto empty_commit_result = empty_update->Commit();
79-
EXPECT_THAT(empty_commit_result, IsError(ErrorKind::kInvalidArgument));
59+
TEST_F(UpdateLocationTest, SetLocationMultipleTimes) {
60+
// Test that setting location multiple times uses the last value
61+
const std::string final_location = "/warehouse/final_location";
8062

81-
// Test commit with location change
82-
// For MockFS, we need to create the metadata directory at the new location
83-
const std::string new_location = "/warehouse/new_table_location";
63+
// Create metadata directory for the new location
8464
auto arrow_fs = std::dynamic_pointer_cast<::arrow::fs::internal::MockFileSystem>(
8565
static_cast<arrow::ArrowFileSystemFileIO&>(*file_io_).fs());
8666
ASSERT_TRUE(arrow_fs != nullptr);
87-
ASSERT_TRUE(arrow_fs->CreateDir(new_location + "/metadata").ok());
67+
ASSERT_TRUE(arrow_fs->CreateDir(final_location + "/metadata").ok());
8868

8969
ICEBERG_UNWRAP_OR_FAIL(auto update, table_->NewUpdateLocation());
90-
update->SetLocation(new_location);
91-
EXPECT_THAT(update->Commit(), IsOk());
70+
update->SetLocation("/warehouse/first_location")
71+
.SetLocation("/warehouse/second_location")
72+
.SetLocation(final_location);
9273

93-
// Verify the location was committed
74+
ICEBERG_UNWRAP_OR_FAIL(auto result, update->Apply());
75+
EXPECT_EQ(result, final_location);
76+
77+
// Commit and verify the final location was persisted
78+
EXPECT_THAT(update->Commit(), IsOk());
9479
ICEBERG_UNWRAP_OR_FAIL(auto reloaded, catalog_->LoadTable(table_ident_));
95-
EXPECT_EQ(reloaded->location(), new_location);
80+
EXPECT_EQ(reloaded->location(), final_location);
9681
}
9782

98-
TEST_F(UpdateLocationTest, CommitWithRelativePath) {
83+
TEST_F(UpdateLocationTest, SetLocationWithRelativePath) {
9984
// Test that relative paths work
10085
const std::string relative_location = "warehouse/relative_location";
10186

@@ -108,12 +93,32 @@ TEST_F(UpdateLocationTest, CommitWithRelativePath) {
10893
ICEBERG_UNWRAP_OR_FAIL(auto update, table_->NewUpdateLocation());
10994
update->SetLocation(relative_location);
11095

111-
EXPECT_THAT(update->Commit(), IsOk());
96+
ICEBERG_UNWRAP_OR_FAIL(auto result, update->Apply());
97+
EXPECT_EQ(result, relative_location);
11298

99+
// Commit and verify
100+
EXPECT_THAT(update->Commit(), IsOk());
113101
ICEBERG_UNWRAP_OR_FAIL(auto reloaded, catalog_->LoadTable(table_ident_));
114102
EXPECT_EQ(reloaded->location(), relative_location);
115103
}
116104

105+
TEST_F(UpdateLocationTest, SetEmptyLocation) {
106+
ICEBERG_UNWRAP_OR_FAIL(auto update, table_->NewUpdateLocation());
107+
update->SetLocation("");
108+
109+
auto result = update->Apply();
110+
EXPECT_THAT(result, IsError(ErrorKind::kValidationFailed));
111+
EXPECT_THAT(result, HasErrorMessage("Location cannot be empty"));
112+
}
113+
114+
TEST_F(UpdateLocationTest, ApplyWithoutSettingLocation) {
115+
ICEBERG_UNWRAP_OR_FAIL(auto update, table_->NewUpdateLocation());
116+
117+
auto result = update->Apply();
118+
EXPECT_THAT(result, IsError(ErrorKind::kInvalidArgument));
119+
EXPECT_THAT(result, HasErrorMessage("Location must be set before applying"));
120+
}
121+
117122
TEST_F(UpdateLocationTest, MultipleUpdatesSequentially) {
118123
// Get arrow_fs for creating directories
119124
auto arrow_fs = std::dynamic_pointer_cast<::arrow::fs::internal::MockFileSystem>(
@@ -126,6 +131,8 @@ TEST_F(UpdateLocationTest, MultipleUpdatesSequentially) {
126131

127132
ICEBERG_UNWRAP_OR_FAIL(auto update1, table_->NewUpdateLocation());
128133
update1->SetLocation(first_location);
134+
ICEBERG_UNWRAP_OR_FAIL(auto result1, update1->Apply());
135+
EXPECT_EQ(result1, first_location);
129136
EXPECT_THAT(update1->Commit(), IsOk());
130137

131138
// Reload and verify
@@ -138,6 +145,8 @@ TEST_F(UpdateLocationTest, MultipleUpdatesSequentially) {
138145

139146
ICEBERG_UNWRAP_OR_FAIL(auto update2, reloaded1->NewUpdateLocation());
140147
update2->SetLocation(second_location);
148+
ICEBERG_UNWRAP_OR_FAIL(auto result2, update2->Apply());
149+
EXPECT_EQ(result2, second_location);
141150
EXPECT_THAT(update2->Commit(), IsOk());
142151

143152
// Reload and verify

‎src/iceberg/type_fwd.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -190,11 +190,11 @@ class Transaction;
190190
class ExpireSnapshots;
191191
class PendingUpdate;
192192
class SnapshotUpdate;
193+
class UpdateLocation;
193194
class UpdatePartitionSpec;
194195
class UpdateProperties;
195196
class UpdateSchema;
196197
class UpdateSortOrder;
197-
class UpdateLocation;
198198

199199
/// ----------------------------------------------------------------------------
200200
/// TODO: Forward declarations below are not added yet.

‎src/iceberg/update/meson.build‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,13 +17,14 @@
1717

1818
install_headers(
1919
[
20+
'expire_snapshots.h',
2021
'pending_update.h',
2122
'snapshot_update.h',
23+
'update_location.h',
2224
'update_partition_spec.h',
2325
'update_schema.h',
2426
'update_sort_order.h',
2527
'update_properties.h',
26-
'update_location.h',
2728
],
2829
subdir: 'iceberg/update',
2930
)

‎src/iceberg/update/pending_update.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -43,12 +43,12 @@ class ICEBERG_EXPORT PendingUpdate : public ErrorCollector {
4343
public:
4444
enum class Kind : uint8_t {
4545
kExpireSnapshots,
46+
kUpdateLocation,
4647
kUpdatePartitionSpec,
4748
kUpdateProperties,
4849
kUpdateSchema,
4950
kUpdateSnapshot,
5051
kUpdateSortOrder,
51-
kUpdateLocation,
5252
};
5353

5454
/// \brief Return the kind of this pending update.

0 commit comments

Comments
 (0)