Skip to content

Commit 628ebe3

Browse files
author
shuxu.li
committed
fix(update): validate DeleteFiles content types and fix Meson build
1 parent 80b8d13 commit 628ebe3

3 files changed

Lines changed: 54 additions & 4 deletions

File tree

src/iceberg/test/meson.build

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -120,10 +120,6 @@ iceberg_tests = {
120120
),
121121
'use_data': true,
122122
},
123-
'overwrite_files_test': {
124-
'sources': files('overwrite_files_test.cc'),
125-
'use_data': true,
126-
},
127123
}
128124

129125
if get_option('rest').enabled()

src/iceberg/test/overwrite_files_test.cc

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -526,6 +526,48 @@ TEST_F(OverwriteFilesTest, BulkDeleteFilesEquivalentToRepeatedDeleteFile) {
526526
EXPECT_EQ(snapshot->summary.at(SnapshotSummaryFields::kTotalDataFiles), "0");
527527
}
528528

529+
// Content validation: because both sets hold std::shared_ptr<DataFile>, DeleteFiles
530+
// guards content so a data file cannot be passed as a delete file (or vice versa). A
531+
// delete file (position/equality) in the data-file set is rejected; the error surfaces
532+
// at Commit().
533+
TEST_F(OverwriteFilesTest, BulkDeleteFilesRejectsDeleteFileInDataSet) {
534+
auto del_file =
535+
MakeDeleteFile("/delete/del_a.parquet", 1L); // content = positionDeletes
536+
DataFileSet data_files;
537+
data_files.insert(del_file);
538+
539+
ICEBERG_UNWRAP_OR_FAIL(auto op, NewOverwrite());
540+
op->DeleteFiles(data_files, DeleteFileSet{});
541+
auto result = op->Commit();
542+
EXPECT_THAT(result, IsError(ErrorKind::kValidationFailed));
543+
EXPECT_THAT(result, HasErrorMessage("has delete-file content"));
544+
}
545+
546+
// A data file (content kData) in the delete-file set is rejected.
547+
TEST_F(OverwriteFilesTest, BulkDeleteFilesRejectsDataFileInDeleteSet) {
548+
DeleteFileSet delete_files;
549+
delete_files.insert(file_a_); // content = kData
550+
551+
ICEBERG_UNWRAP_OR_FAIL(auto op, NewOverwrite());
552+
op->DeleteFiles(DataFileSet{}, delete_files);
553+
auto result = op->Commit();
554+
EXPECT_THAT(result, IsError(ErrorKind::kValidationFailed));
555+
EXPECT_THAT(result, HasErrorMessage("has data-file content"));
556+
}
557+
558+
// An equality delete file is a valid delete file (content != kData) and is accepted in
559+
// the delete-file set.
560+
TEST_F(OverwriteFilesTest, BulkDeleteFilesAcceptsEqualityDeleteInDeleteSet) {
561+
auto eq_delete = MakeEqualityDeleteFile("/delete/eq_a.parquet", 1L);
562+
DeleteFileSet delete_files;
563+
delete_files.insert(eq_delete);
564+
565+
ICEBERG_UNWRAP_OR_FAIL(auto op, NewOverwrite());
566+
op->DeleteFiles(DataFileSet{}, delete_files);
567+
op->AddFile(file_b_);
568+
EXPECT_THAT(op->Commit(), IsOk());
569+
}
570+
529571
// =====================================================================================
530572
// 9.6 Concurrency-validation tests (Req 8.2, 8.3, 9.2-9.5; Properties 6, 7)
531573
// =====================================================================================

src/iceberg/update/overwrite_files.cc

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -76,14 +76,26 @@ OverwriteFiles& OverwriteFiles::DeleteFiles(const DataFileSet& data_files_to_del
7676
const DeleteFileSet& delete_files_to_delete) {
7777
// Bulk equivalent of repeated DeleteFile(...) plus explicit delete-file removal. Empty
7878
// sets are no-ops; the set types handle deduplication of repeated entries.
79+
//
80+
// Because both sets hold std::shared_ptr<DataFile>, content is validated here so a data
81+
// file cannot be registered as a delete file (or vice versa): a data file must have
82+
// content kData, and a delete file must be a position/equality delete (content !=
83+
// kData). This restores the type safety the C++ shared DataFile representation cannot
84+
// enforce at compile time.
7985
for (const auto& file : data_files_to_delete) {
8086
ICEBERG_BUILDER_CHECK(file != nullptr, "Invalid data file: null");
87+
ICEBERG_BUILDER_CHECK(file->content == DataFile::Content::kData,
88+
"Invalid data file to delete: {} has delete-file content",
89+
file->file_path);
8190
// Dual-track: record for delete-conflict validation AND register for removal.
8291
deleted_data_files_.insert(file);
8392
ICEBERG_BUILDER_RETURN_IF_ERROR(DeleteDataFile(file));
8493
}
8594
for (const auto& file : delete_files_to_delete) {
8695
ICEBERG_BUILDER_CHECK(file != nullptr, "Invalid delete file: null");
96+
ICEBERG_BUILDER_CHECK(file->content != DataFile::Content::kData,
97+
"Invalid delete file to delete: {} has data-file content",
98+
file->file_path);
8799
ICEBERG_BUILDER_RETURN_IF_ERROR(DeleteDeleteFile(file));
88100
}
89101
return *this;

0 commit comments

Comments
 (0)