Skip to content

Commit ef04fd5

Browse files
committed
fix: rewrite files review comments
1 parent a3068ff commit ef04fd5

5 files changed

Lines changed: 35 additions & 3 deletions

File tree

src/iceberg/table.cc

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -367,6 +367,10 @@ Result<std::shared_ptr<OverwriteFiles>> StaticTable::NewOverwrite() {
367367
return NotSupported("Cannot create an overwrite for a static table");
368368
}
369369

370+
Result<std::shared_ptr<RewriteFiles>> StaticTable::NewRewriteFiles() {
371+
return NotSupported("Cannot create a rewrite files for a static table");
372+
}
373+
370374
Result<std::shared_ptr<SnapshotManager>> StaticTable::NewSnapshotManager() {
371375
return NotSupported("Cannot create a snapshot manager for a static table");
372376
}

src/iceberg/table.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -267,6 +267,8 @@ class ICEBERG_EXPORT StaticTable : public Table {
267267

268268
Result<std::shared_ptr<OverwriteFiles>> NewOverwrite() override;
269269

270+
Result<std::shared_ptr<RewriteFiles>> NewRewriteFiles() override;
271+
270272
Result<std::shared_ptr<SnapshotManager>> NewSnapshotManager() override;
271273

272274
private:

src/iceberg/test/rewrite_files_test.cc

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -127,6 +127,30 @@ TEST_F(RewriteFilesTest, AddAndDelete) {
127127
EXPECT_EQ(std::stoll(snapshot->summary.at(SnapshotSummaryFields::kAddedDataFiles)), 1);
128128
}
129129

130+
TEST_F(RewriteFilesTest, DeleteDataFileCopiesCallerFile) {
131+
CommitFileA();
132+
133+
const std::string original_path = file_a_->file_path;
134+
135+
ICEBERG_UNWRAP_OR_FAIL(auto rw, NewRewriteFiles());
136+
rw->DeleteDataFile(file_a_);
137+
rw->AddDataFile(rewritten_file_a_);
138+
139+
file_a_->file_path = file_b_->file_path;
140+
141+
EXPECT_THAT(rw->Commit(), IsOk());
142+
EXPECT_THAT(table_->Refresh(), IsOk());
143+
144+
ICEBERG_UNWRAP_OR_FAIL(auto rw_missing_original, NewRewriteFiles());
145+
auto missing_file = std::make_shared<DataFile>(*file_a_);
146+
missing_file->file_path = original_path;
147+
rw_missing_original->DeleteDataFile(missing_file);
148+
rw_missing_original->AddDataFile(file_b_);
149+
auto result = rw_missing_original->Commit();
150+
EXPECT_THAT(result, IsError(ErrorKind::kValidationFailed));
151+
EXPECT_THAT(result, HasErrorMessage("Missing required files to delete"));
152+
}
153+
130154
// Rewrite one of several data files, verifying only the target is affected.
131155
TEST_F(RewriteFilesTest, AddAndDeletePartialRewrite) {
132156
CommitFileA();

src/iceberg/test/table_test.cc

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -164,6 +164,7 @@ TEST(StaticTableTest, NewMutatingOperationsAreNotSupported) {
164164
EXPECT_THAT(table->NewDeleteFiles(), IsError(ErrorKind::kNotSupported));
165165
EXPECT_THAT(table->NewRowDelta(), IsError(ErrorKind::kNotSupported));
166166
EXPECT_THAT(table->NewOverwrite(), IsError(ErrorKind::kNotSupported));
167+
EXPECT_THAT(table->NewRewriteFiles(), IsError(ErrorKind::kNotSupported));
167168
EXPECT_THAT(table->NewSnapshotManager(), IsError(ErrorKind::kNotSupported));
168169
}
169170

src/iceberg/update/rewrite_files.cc

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -48,9 +48,10 @@ Result<std::unique_ptr<RewriteFiles>> RewriteFiles::Make(
4848
}
4949

5050
RewriteFiles& RewriteFiles::DeleteDataFile(const std::shared_ptr<DataFile>& data_file) {
51-
ICEBERG_BUILDER_RETURN_IF_ERROR(MergingSnapshotUpdate::DeleteDataFile(data_file));
52-
// Track replaced data files for conflict detection
53-
replaced_data_files_.insert(data_file);
51+
auto staged_file =
52+
data_file == nullptr ? nullptr : std::make_shared<DataFile>(*data_file);
53+
ICEBERG_BUILDER_RETURN_IF_ERROR(MergingSnapshotUpdate::DeleteDataFile(staged_file));
54+
replaced_data_files_.insert(std::move(staged_file));
5455
return *this;
5556
}
5657

0 commit comments

Comments
 (0)