From 6c1d183bae054592a9bd802634f25d3bac008722 Mon Sep 17 00:00:00 2001 From: Minh Vu Date: Sun, 15 Nov 2026 19:10:13 +0100 Subject: [PATCH 1/2] fix(scan): validate path-scoped delete partitions --- src/iceberg/delete_file_index.cc | 29 +++++- src/iceberg/test/delete_file_index_test.cc | 103 +++++++++++++++++++++ 2 files changed, 131 insertions(+), 1 deletion(-) diff --git a/src/iceberg/delete_file_index.cc b/src/iceberg/delete_file_index.cc index a8c4ef126..972eca1eb 100644 --- a/src/iceberg/delete_file_index.cc +++ b/src/iceberg/delete_file_index.cc @@ -44,6 +44,28 @@ namespace iceberg { +namespace { + +// A path reference alone does not make a file-scoped delete applicable. Iceberg also +// requires the delete and data files to use the same partition spec and partition. +Status ValidateDeletePartition(const DataFile& delete_file, const DataFile& data_file) { + ICEBERG_CHECK(delete_file.partition_spec_id.has_value() && + data_file.partition_spec_id.has_value(), + "Missing partition spec ID for delete file {} or data file {}", + delete_file.file_path, data_file.file_path); + ICEBERG_CHECK( + delete_file.partition_spec_id == data_file.partition_spec_id, + "Delete file {} has partition spec ID {}, but data file {} has spec ID {}", + delete_file.file_path, delete_file.partition_spec_id.value(), data_file.file_path, + data_file.partition_spec_id.value()); + ICEBERG_CHECK(delete_file.partition == data_file.partition, + "Delete file {} partition values do not match data file {}", + delete_file.file_path, data_file.file_path); + return {}; +} + +} // namespace + namespace internal { Status EqualityDeleteFile::ConvertBoundsIfNeeded() const { @@ -439,7 +461,11 @@ Result>> DeleteFileIndex::FindPathDeletes( return {}; } - return it->second->Filter(seq); + auto deletes = it->second->Filter(seq); + for (const auto& delete_file : deletes) { + ICEBERG_RETURN_UNEXPECTED(ValidateDeletePartition(*delete_file, data_file)); + } + return deletes; } Result> DeleteFileIndex::FindDV( @@ -457,6 +483,7 @@ Result> DeleteFileIndex::FindDV( "DV data sequence number {} must be greater than or equal to data file " "sequence number {}", it->second.sequence_number.value(), seq); + ICEBERG_RETURN_UNEXPECTED(ValidateDeletePartition(*it->second.data_file, data_file)); return it->second.data_file; } diff --git a/src/iceberg/test/delete_file_index_test.cc b/src/iceberg/test/delete_file_index_test.cc index 7f77c638a..50a48ec66 100644 --- a/src/iceberg/test/delete_file_index_test.cc +++ b/src/iceberg/test/delete_file_index_test.cc @@ -67,6 +67,14 @@ class DeleteFileIndexTest : public testing::TestWithParam { /*spec_id=*/1, {PartitionField(/*source_id=*/2, /*field_id=*/1000, "data_bucket", Transform::Bucket(16))})); + // A second spec with the same partition transform lets tests verify that a + // path-matched delete still needs the exact spec ID to match the data file. + ICEBERG_UNWRAP_OR_FAIL( + other_partitioned_spec_, + PartitionSpec::Make( + /*spec_id=*/2, {PartitionField(/*source_id=*/2, /*field_id=*/1001, + "data_bucket", Transform::Bucket(16))})); + // Unpartitioned spec unpartitioned_spec_ = PartitionSpec::Unpartitioned(); @@ -188,6 +196,7 @@ class DeleteFileIndexTest : public testing::TestWithParam { std::unordered_map> GetSpecsById() { return {{partitioned_spec_->spec_id(), partitioned_spec_}, + {other_partitioned_spec_->spec_id(), other_partitioned_spec_}, {unpartitioned_spec_->spec_id(), unpartitioned_spec_}}; } @@ -210,6 +219,7 @@ class DeleteFileIndexTest : public testing::TestWithParam { std::shared_ptr file_io_; std::shared_ptr schema_; std::shared_ptr partitioned_spec_; + std::shared_ptr other_partitioned_spec_; std::shared_ptr unpartitioned_spec_; std::shared_ptr file_a_; @@ -1088,6 +1098,99 @@ TEST_P(DeleteFileIndexTest, TestMixDeleteFilesAndDVs) { } } +TEST_P(DeleteFileIndexTest, TestDVRejectsMismatchedPartitionValues) { + if (GetParam() < 3) { + GTEST_SKIP() << "DVs only supported in V3+"; + } + + auto dv = MakeDV("/path/to/dv.puffin", PartitionValues({Literal::Int(1)}), + partitioned_spec_->spec_id(), file_a_->file_path); + auto manifest = WriteDeleteManifest( + /*format_version=*/3, /*snapshot_id=*/1000L, + {MakeDeleteEntry(/*snapshot_id=*/1000L, /*sequence_number=*/1, dv)}, + partitioned_spec_); + ICEBERG_UNWRAP_OR_FAIL(auto index, BuildIndex({manifest})); + + auto result = index->ForDataFile(0, *file_a_); + EXPECT_THAT(result, IsError(ErrorKind::kValidationFailed)); + EXPECT_THAT(result, HasErrorMessage("partition values")); +} + +TEST_P(DeleteFileIndexTest, TestDVRejectsMismatchedPartitionSpecId) { + if (GetParam() < 3) { + GTEST_SKIP() << "DVs only supported in V3+"; + } + + auto dv = MakeDV("/path/to/dv.puffin", PartitionValues({Literal::Int(0)}), + other_partitioned_spec_->spec_id(), file_a_->file_path); + auto manifest = WriteDeleteManifest( + /*format_version=*/3, /*snapshot_id=*/1000L, + {MakeDeleteEntry(/*snapshot_id=*/1000L, /*sequence_number=*/1, dv)}, + other_partitioned_spec_); + ICEBERG_UNWRAP_OR_FAIL(auto index, BuildIndex({manifest})); + + auto result = index->ForDataFile(0, *file_a_); + EXPECT_THAT(result, IsError(ErrorKind::kValidationFailed)); + EXPECT_THAT(result, HasErrorMessage("partition spec IDs")); +} + +TEST_P(DeleteFileIndexTest, TestPathPositionDeleteMatchesPartition) { + if (GetParam() >= 3) { + GTEST_SKIP() << "Path-scoped position deletes use DVs in V3"; + } + + auto pos_delete = MakePositionDeleteFile( + "/path/to/pos-delete.parquet", PartitionValues({Literal::Int(0)}), + partitioned_spec_->spec_id(), file_a_->file_path); + auto manifest = WriteDeleteManifest( + /*format_version=*/2, /*snapshot_id=*/1000L, + {MakeDeleteEntry(/*snapshot_id=*/1000L, /*sequence_number=*/1, pos_delete)}, + partitioned_spec_); + ICEBERG_UNWRAP_OR_FAIL(auto index, BuildIndex({manifest})); + + ICEBERG_UNWRAP_OR_FAIL(auto deletes, index->ForDataFile(0, *file_a_)); + ASSERT_EQ(deletes.size(), 1); + EXPECT_EQ(deletes[0]->file_path, "/path/to/pos-delete.parquet"); +} + +TEST_P(DeleteFileIndexTest, TestPathPositionDeleteRejectsMismatchedPartitionValues) { + if (GetParam() >= 3) { + GTEST_SKIP() << "Path-scoped position deletes use DVs in V3"; + } + + auto pos_delete = MakePositionDeleteFile( + "/path/to/pos-delete.parquet", PartitionValues({Literal::Int(1)}), + partitioned_spec_->spec_id(), file_a_->file_path); + auto manifest = WriteDeleteManifest( + /*format_version=*/2, /*snapshot_id=*/1000L, + {MakeDeleteEntry(/*snapshot_id=*/1000L, /*sequence_number=*/1, pos_delete)}, + partitioned_spec_); + ICEBERG_UNWRAP_OR_FAIL(auto index, BuildIndex({manifest})); + + auto result = index->ForDataFile(0, *file_a_); + EXPECT_THAT(result, IsError(ErrorKind::kValidationFailed)); + EXPECT_THAT(result, HasErrorMessage("partition values")); +} + +TEST_P(DeleteFileIndexTest, TestPathPositionDeleteRejectsMismatchedPartitionSpecId) { + if (GetParam() >= 3) { + GTEST_SKIP() << "Path-scoped position deletes use DVs in V3"; + } + + auto pos_delete = MakePositionDeleteFile( + "/path/to/pos-delete.parquet", PartitionValues({Literal::Int(0)}), + other_partitioned_spec_->spec_id(), file_a_->file_path); + auto manifest = WriteDeleteManifest( + /*format_version=*/2, /*snapshot_id=*/1000L, + {MakeDeleteEntry(/*snapshot_id=*/1000L, /*sequence_number=*/1, pos_delete)}, + other_partitioned_spec_); + ICEBERG_UNWRAP_OR_FAIL(auto index, BuildIndex({manifest})); + + auto result = index->ForDataFile(0, *file_a_); + EXPECT_THAT(result, IsError(ErrorKind::kValidationFailed)); + EXPECT_THAT(result, HasErrorMessage("partition spec IDs")); +} + TEST_P(DeleteFileIndexTest, TestMultipleDVs) { auto version = GetParam(); if (version < 3) { From 600a39bd3f3e21cc82b4242483972320d022c3a4 Mon Sep 17 00:00:00 2001 From: Minh Vu Date: Sun, 15 Nov 2026 19:25:47 +0100 Subject: [PATCH 2/2] fix(scan): compare null delete partitions correctly --- src/iceberg/delete_file_index.cc | 25 +++++- src/iceberg/test/delete_file_index_test.cc | 91 ++++++++++++++++++---- 2 files changed, 101 insertions(+), 15 deletions(-) diff --git a/src/iceberg/delete_file_index.cc b/src/iceberg/delete_file_index.cc index 972eca1eb..e7460adbb 100644 --- a/src/iceberg/delete_file_index.cc +++ b/src/iceberg/delete_file_index.cc @@ -48,6 +48,29 @@ namespace { // A path reference alone does not make a file-scoped delete applicable. Iceberg also // requires the delete and data files to use the same partition spec and partition. +bool PartitionValuesMatch(const PartitionValues& left, const PartitionValues& right) { + if (left.num_fields() != right.num_fields()) { + return false; + } + + for (size_t i = 0; i < left.num_fields(); ++i) { + const auto& left_value = left.values()[i]; + const auto& right_value = right.values()[i]; + if (left_value.IsNull() || right_value.IsNull()) { + if (left_value.IsNull() != right_value.IsNull()) { + return false; + } + continue; + } + + if (left_value != right_value) { + return false; + } + } + + return true; +} + Status ValidateDeletePartition(const DataFile& delete_file, const DataFile& data_file) { ICEBERG_CHECK(delete_file.partition_spec_id.has_value() && data_file.partition_spec_id.has_value(), @@ -58,7 +81,7 @@ Status ValidateDeletePartition(const DataFile& delete_file, const DataFile& data "Delete file {} has partition spec ID {}, but data file {} has spec ID {}", delete_file.file_path, delete_file.partition_spec_id.value(), data_file.file_path, data_file.partition_spec_id.value()); - ICEBERG_CHECK(delete_file.partition == data_file.partition, + ICEBERG_CHECK(PartitionValuesMatch(delete_file.partition, data_file.partition), "Delete file {} partition values do not match data file {}", delete_file.file_path, data_file.file_path); return {}; diff --git a/src/iceberg/test/delete_file_index_test.cc b/src/iceberg/test/delete_file_index_test.cc index 50a48ec66..854eae7bd 100644 --- a/src/iceberg/test/delete_file_index_test.cc +++ b/src/iceberg/test/delete_file_index_test.cc @@ -67,13 +67,13 @@ class DeleteFileIndexTest : public testing::TestWithParam { /*spec_id=*/1, {PartitionField(/*source_id=*/2, /*field_id=*/1000, "data_bucket", Transform::Bucket(16))})); - // A second spec with the same partition transform lets tests verify that a - // path-matched delete still needs the exact spec ID to match the data file. + // A second, evolved bucket spec lets tests verify that a path-matched delete still + // needs the exact spec ID to match the data file. ICEBERG_UNWRAP_OR_FAIL( other_partitioned_spec_, PartitionSpec::Make( /*spec_id=*/2, {PartitionField(/*source_id=*/2, /*field_id=*/1001, - "data_bucket", Transform::Bucket(16))})); + "data_bucket_32", Transform::Bucket(32))})); // Unpartitioned spec unpartitioned_spec_ = PartitionSpec::Unpartitioned(); @@ -160,6 +160,17 @@ class DeleteFileIndexTest : public testing::TestWithParam { }); } + void SetNullableIdentityPartitionSpec() { + schema_ = std::make_shared(std::vector{ + SchemaField::MakeRequired(/*field_id=*/1, "id", int32()), + SchemaField::MakeOptional(/*field_id=*/2, "category", string())}); + ICEBERG_UNWRAP_OR_FAIL( + partitioned_spec_, + PartitionSpec::Make( + /*spec_id=*/1, {PartitionField(/*source_id=*/2, /*field_id=*/1000, "category", + Transform::Identity())})); + } + ManifestEntry MakeDeleteEntry(int64_t snapshot_id, int64_t sequence_number, std::shared_ptr file, ManifestStatus status = ManifestStatus::kAdded) { @@ -1134,11 +1145,52 @@ TEST_P(DeleteFileIndexTest, TestDVRejectsMismatchedPartitionSpecId) { EXPECT_THAT(result, HasErrorMessage("partition spec IDs")); } -TEST_P(DeleteFileIndexTest, TestPathPositionDeleteMatchesPartition) { - if (GetParam() >= 3) { - GTEST_SKIP() << "Path-scoped position deletes use DVs in V3"; +TEST_P(DeleteFileIndexTest, TestDVMatchesNullPartition) { + if (GetParam() < 3) { + GTEST_SKIP() << "DVs only supported in V3+"; + } + + SetNullableIdentityPartitionSpec(); + auto null_partition = PartitionValues(Literal::Null(string())); + auto data_file = MakeDataFile("/path/to/data-null.parquet", null_partition, + partitioned_spec_->spec_id()); + auto dv = MakeDV("/path/to/dv-null.puffin", null_partition, + partitioned_spec_->spec_id(), data_file->file_path); + auto manifest = WriteDeleteManifest( + /*format_version=*/3, /*snapshot_id=*/1000L, + {MakeDeleteEntry(/*snapshot_id=*/1000L, /*sequence_number=*/1, dv)}, + partitioned_spec_); + ICEBERG_UNWRAP_OR_FAIL(auto index, BuildIndex({manifest})); + + ICEBERG_UNWRAP_OR_FAIL(auto deletes, index->ForDataFile(0, *data_file)); + ASSERT_EQ(deletes.size(), 1); + EXPECT_EQ(deletes[0]->file_path, "/path/to/dv-null.puffin"); +} + +TEST_P(DeleteFileIndexTest, TestDVRejectsNullValuePartitionMismatch) { + if (GetParam() < 3) { + GTEST_SKIP() << "DVs only supported in V3+"; } + SetNullableIdentityPartitionSpec(); + auto null_partition = PartitionValues(Literal::Null(string())); + auto data_file = MakeDataFile("/path/to/data-null.parquet", null_partition, + partitioned_spec_->spec_id()); + auto dv = + MakeDV("/path/to/dv-non-null.puffin", PartitionValues(Literal::String("category")), + partitioned_spec_->spec_id(), data_file->file_path); + auto manifest = WriteDeleteManifest( + /*format_version=*/3, /*snapshot_id=*/1000L, + {MakeDeleteEntry(/*snapshot_id=*/1000L, /*sequence_number=*/1, dv)}, + partitioned_spec_); + ICEBERG_UNWRAP_OR_FAIL(auto index, BuildIndex({manifest})); + + auto result = index->ForDataFile(0, *data_file); + EXPECT_THAT(result, IsError(ErrorKind::kValidationFailed)); + EXPECT_THAT(result, HasErrorMessage("partition values")); +} + +TEST_P(DeleteFileIndexTest, TestPathPositionDeleteMatchesPartition) { auto pos_delete = MakePositionDeleteFile( "/path/to/pos-delete.parquet", PartitionValues({Literal::Int(0)}), partitioned_spec_->spec_id(), file_a_->file_path); @@ -1153,11 +1205,26 @@ TEST_P(DeleteFileIndexTest, TestPathPositionDeleteMatchesPartition) { EXPECT_EQ(deletes[0]->file_path, "/path/to/pos-delete.parquet"); } -TEST_P(DeleteFileIndexTest, TestPathPositionDeleteRejectsMismatchedPartitionValues) { - if (GetParam() >= 3) { - GTEST_SKIP() << "Path-scoped position deletes use DVs in V3"; - } +TEST_P(DeleteFileIndexTest, TestPathPositionDeleteMatchesNullPartition) { + SetNullableIdentityPartitionSpec(); + auto null_partition = PartitionValues(Literal::Null(string())); + auto data_file = MakeDataFile("/path/to/data-null.parquet", null_partition, + partitioned_spec_->spec_id()); + auto pos_delete = + MakePositionDeleteFile("/path/to/pos-delete-null.parquet", null_partition, + partitioned_spec_->spec_id(), data_file->file_path); + auto manifest = WriteDeleteManifest( + /*format_version=*/2, /*snapshot_id=*/1000L, + {MakeDeleteEntry(/*snapshot_id=*/1000L, /*sequence_number=*/1, pos_delete)}, + partitioned_spec_); + ICEBERG_UNWRAP_OR_FAIL(auto index, BuildIndex({manifest})); + ICEBERG_UNWRAP_OR_FAIL(auto deletes, index->ForDataFile(0, *data_file)); + ASSERT_EQ(deletes.size(), 1); + EXPECT_EQ(deletes[0]->file_path, "/path/to/pos-delete-null.parquet"); +} + +TEST_P(DeleteFileIndexTest, TestPathPositionDeleteRejectsMismatchedPartitionValues) { auto pos_delete = MakePositionDeleteFile( "/path/to/pos-delete.parquet", PartitionValues({Literal::Int(1)}), partitioned_spec_->spec_id(), file_a_->file_path); @@ -1173,10 +1240,6 @@ TEST_P(DeleteFileIndexTest, TestPathPositionDeleteRejectsMismatchedPartitionValu } TEST_P(DeleteFileIndexTest, TestPathPositionDeleteRejectsMismatchedPartitionSpecId) { - if (GetParam() >= 3) { - GTEST_SKIP() << "Path-scoped position deletes use DVs in V3"; - } - auto pos_delete = MakePositionDeleteFile( "/path/to/pos-delete.parquet", PartitionValues({Literal::Int(0)}), other_partitioned_spec_->spec_id(), file_a_->file_path);