Skip to content

Commit 0abb6df

Browse files
manuzhangcodex
andcommitted
fix row delta delete validation
Co-authored-by: Codex <codex@openai.com>
1 parent f5e70ea commit 0abb6df

3 files changed

Lines changed: 34 additions & 4 deletions

File tree

src/iceberg/test/row_delta_test.cc

Lines changed: 25 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -274,7 +274,7 @@ TEST_F(RowDeltaTest, ValidateNoConflictingDeleteFilesFailsForConcurrentDelete) {
274274
EXPECT_THAT(result, HasErrorMessage(delete_file->file_path));
275275
}
276276

277-
TEST_F(RowDeltaTest, ValidateDataFilesExistFailsForConcurrentDelete) {
277+
TEST_F(RowDeltaTest, ValidateDataFilesExistSkipsConcurrentDeleteByDefault) {
278278
CommitFileA();
279279
ICEBERG_UNWRAP_OR_FAIL(auto starting_snapshot, table_->current_snapshot());
280280

@@ -294,6 +294,30 @@ TEST_F(RowDeltaTest, ValidateDataFilesExistFailsForConcurrentDelete) {
294294
row_delta->ValidateDataFilesExist(referenced_files);
295295
row_delta->AddDeletes(delete_file);
296296

297+
EXPECT_THAT(row_delta->Commit(), IsOk());
298+
}
299+
300+
TEST_F(RowDeltaTest, ValidateDataFilesExistFailsForConcurrentDeleteWithValidateDeletedFiles) {
301+
CommitFileA();
302+
ICEBERG_UNWRAP_OR_FAIL(auto starting_snapshot, table_->current_snapshot());
303+
304+
ICEBERG_UNWRAP_OR_FAIL(auto delete_files, table_->NewDeleteFiles());
305+
delete_files->DeleteFile(file_a_);
306+
EXPECT_THAT(delete_files->Commit(), IsOk());
307+
EXPECT_THAT(table_->Refresh(), IsOk());
308+
309+
auto delete_file = MakeDeleteFile("/delete/file_a_pos_deletes.parquet",
310+
/*partition_x=*/1L);
311+
delete_file->referenced_data_file = file_a_->file_path;
312+
313+
std::shared_ptr<RowDelta> row_delta;
314+
ICEBERG_UNWRAP_OR_FAIL(row_delta, table_->NewRowDelta());
315+
row_delta->ValidateFromSnapshot(starting_snapshot->snapshot_id);
316+
std::vector<std::string> referenced_files{file_a_->file_path};
317+
row_delta->ValidateDataFilesExist(referenced_files);
318+
row_delta->ValidateDeletedFiles();
319+
row_delta->AddDeletes(delete_file);
320+
297321
auto result = row_delta->Commit();
298322
EXPECT_THAT(result, IsError(ErrorKind::kValidationFailed));
299323
EXPECT_THAT(result, HasErrorMessage("Cannot commit, missing data files"));

src/iceberg/update/row_delta.cc

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -143,7 +143,7 @@ Status RowDelta::Validate(const TableMetadata& current_metadata,
143143
if (!referenced_data_files_.empty()) {
144144
ICEBERG_RETURN_UNEXPECTED(MergingSnapshotUpdate::ValidateDataFilesExist(
145145
current_metadata, starting_snapshot_id_, referenced_data_files_,
146-
/*skip_deletes=*/false, conflict_detection_filter_, snapshot, io,
146+
/*skip_deletes=*/!validate_deletes_, conflict_detection_filter_, snapshot, io,
147147
IsCaseSensitive()));
148148
}
149149

src/iceberg/update/row_delta.h

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -66,10 +66,16 @@ class ICEBERG_EXPORT RowDelta : public MergingSnapshotUpdate {
6666
RowDelta& CaseSensitive(bool case_sensitive);
6767

6868
/// \brief Validate that referenced data files still exist.
69+
///
70+
/// By default, this validation checks overwrite and replace commits. To apply
71+
/// validation to delete commits, call ValidateDeletedFiles().
6972
RowDelta& ValidateDataFilesExist(std::span<const std::string> referenced_files);
7073

71-
/// \brief Fail if any requested data/delete-file removal is missing from
72-
/// manifests when the table has a current snapshot.
74+
/// \brief Enable validation for missing delete paths and delete-operation conflicts.
75+
///
76+
/// This fails if any requested data/delete-file removal is missing from
77+
/// manifests when the table has a current snapshot. It also makes
78+
/// ValidateDataFilesExist() check delete-operation snapshots.
7379
RowDelta& ValidateDeletedFiles();
7480

7581
/// \brief Set the conflict detection filter used by validation methods.

0 commit comments

Comments
 (0)