Skip to content

fix(scan): validate path-scoped delete partitions #1136

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
iceberg-kitten wants to merge 2 commits into
base: main
Choose a base branch
from
+217 −1
Open

fix(scan): validate path-scoped delete partitions #1136

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6c1d183
fix(scan): validate path-scoped delete partitions
fallintoplace Nov 15, 2026
600a39b
fix(scan): compare null delete partitions correctly
fallintoplace Nov 15, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 51 additions & 1 deletion src/iceberg/delete_file_index.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,51 @@

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.
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(),
"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(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 {};
}

} // namespace

namespace internal {

Status EqualityDeleteFile::ConvertBoundsIfNeeded() const {
Expand Down Expand Up @@ -439,7 +484,11 @@ Result<std::vector<std::shared_ptr<DataFile>>> 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<std::shared_ptr<DataFile>> DeleteFileIndex::FindDV(
Expand All @@ -457,6 +506,7 @@ Result<std::shared_ptr<DataFile>> 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;
}
Expand Down
166 changes: 166 additions & 0 deletions src/iceberg/test/delete_file_index_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,14 @@ class DeleteFileIndexTest : public testing::TestWithParam<int8_t> {
/*spec_id=*/1, {PartitionField(/*source_id=*/2, /*field_id=*/1000,
"data_bucket", Transform::Bucket(16))}));

// 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_32", Transform::Bucket(32))}));

// Unpartitioned spec
unpartitioned_spec_ = PartitionSpec::Unpartitioned();

Expand Down Expand Up @@ -152,6 +160,17 @@ class DeleteFileIndexTest : public testing::TestWithParam<int8_t> {
});
}

void SetNullableIdentityPartitionSpec() {
schema_ = std::make_shared<Schema>(std::vector<SchemaField>{
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<DataFile> file,
ManifestStatus status = ManifestStatus::kAdded) {
Expand Down Expand Up @@ -188,6 +207,7 @@ class DeleteFileIndexTest : public testing::TestWithParam<int8_t> {

std::unordered_map<int32_t, std::shared_ptr<PartitionSpec>> GetSpecsById() {
return {{partitioned_spec_->spec_id(), partitioned_spec_},
{other_partitioned_spec_->spec_id(), other_partitioned_spec_},
{unpartitioned_spec_->spec_id(), unpartitioned_spec_}};
}

Expand All @@ -210,6 +230,7 @@ class DeleteFileIndexTest : public testing::TestWithParam<int8_t> {
std::shared_ptr<FileIO> file_io_;
std::shared_ptr<Schema> schema_;
std::shared_ptr<PartitionSpec> partitioned_spec_;
std::shared_ptr<PartitionSpec> other_partitioned_spec_;
std::shared_ptr<PartitionSpec> unpartitioned_spec_;

std::shared_ptr<DataFile> file_a_;
Expand Down Expand Up @@ -1088,6 +1109,151 @@ 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, 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);
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, 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);
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) {
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) {
Expand Down