wgtmac commented on code in PR #777: URL: https://github.com/apache/iceberg-cpp/pull/777#discussion_r3504128876
########## src/iceberg/test/deletion_vector_writer_test.cc: ########## @@ -0,0 +1,303 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +#include "iceberg/data/deletion_vector_writer.h" + +#include <memory> +#include <string> +#include <vector> + +#include <gtest/gtest.h> + +#include "iceberg/data/delete_loader.h" +#include "iceberg/deletes/position_delete_index.h" +#include "iceberg/deletes/roaring_position_bitmap.h" +#include "iceberg/file_format.h" +#include "iceberg/manifest/manifest_entry.h" +#include "iceberg/partition_spec.h" +#include "iceberg/row/partition_values.h" +#include "iceberg/test/matchers.h" +#include "iceberg/test/mock_io.h" + +namespace iceberg { + +namespace { + +std::shared_ptr<DataFile> FindByReferencedFile( + const std::vector<std::shared_ptr<DataFile>>& files, const std::string& ref) { + for (const auto& file : files) { + if (file->referenced_data_file == ref) { + return file; + } + } + return nullptr; +} + +std::shared_ptr<PartitionSpec> UnpartitionedSpec() { + return PartitionSpec::Unpartitioned(); +} + +} // namespace + +// Full write -> read round trip: write deletion vectors with the writer, then +// load them back through DeleteLoader using the produced DataFile metadata. +TEST(DeletionVectorWriterTest, WriteThenLoadEndToEnd) { + auto io = std::make_shared<MockFileIO>(); + auto spec = UnpartitionedSpec(); + + std::vector<std::shared_ptr<DataFile>> delete_files; + { + ICEBERG_UNWRAP_OR_FAIL(auto writer, + DeletionVectorWriter::Make(DeletionVectorWriterOptions{ + .path = "memory://deletes.puffin", + .io = io, + .properties = {{"created-by", "iceberg-cpp-test"}}, + })); + + ASSERT_THAT(writer->Delete("data-a.parquet", 0, spec, PartitionValues{}), IsOk()); + ASSERT_THAT(writer->Delete("data-a.parquet", 5, spec, PartitionValues{}), IsOk()); + ASSERT_THAT(writer->Delete("data-a.parquet", 10, spec, PartitionValues{}), IsOk()); + ASSERT_THAT(writer->Delete("data-b.parquet", 1, spec, PartitionValues{}), IsOk()); + ASSERT_THAT(writer->Delete("data-b.parquet", 2, spec, PartitionValues{}), IsOk()); + ASSERT_THAT(writer->Close(), IsOk()); + + ICEBERG_UNWRAP_OR_FAIL(auto result, writer->Metadata()); + delete_files = result.delete_files; + // Each referenced data file is reported once. + EXPECT_EQ(result.referenced_data_files.size(), 2u); + // No previous deletes were loaded, so nothing was rewritten. + EXPECT_TRUE(result.rewritten_delete_files.empty()); + } + + // One DataFile per referenced data file. + ASSERT_EQ(delete_files.size(), 2u); + + auto dv_a = FindByReferencedFile(delete_files, "data-a.parquet"); + auto dv_b = FindByReferencedFile(delete_files, "data-b.parquet"); + ASSERT_NE(dv_a, nullptr); + ASSERT_NE(dv_b, nullptr); + + // Metadata is spec-compliant for a deletion vector. + EXPECT_EQ(dv_a->content, DataFile::Content::kPositionDeletes); + EXPECT_EQ(dv_a->file_format, FileFormatType::kPuffin); + EXPECT_TRUE(dv_a->IsDeletionVector()); + EXPECT_EQ(dv_a->file_path, "memory://deletes.puffin"); + EXPECT_EQ(dv_a->record_count, 3); + EXPECT_TRUE(dv_a->content_offset.has_value()); + EXPECT_TRUE(dv_a->content_size_in_bytes.has_value()); + EXPECT_GT(dv_a->file_size_in_bytes, 0); + EXPECT_EQ(dv_a->partition_spec_id, spec->spec_id()); + EXPECT_EQ(dv_b->record_count, 2); + + // Both blobs live in the same Puffin file but at different offsets. + EXPECT_EQ(dv_a->file_path, dv_b->file_path); + EXPECT_NE(dv_a->content_offset.value(), dv_b->content_offset.value()); + + // Read back through DeleteLoader for data-a.parquet. + DeleteLoader loader(io); + { + auto result = loader.LoadPositionDeletes(delete_files, "data-a.parquet"); + ASSERT_THAT(result, IsOk()); + auto& index = result.value(); + EXPECT_EQ(index.Cardinality(), 3); + EXPECT_TRUE(index.IsDeleted(0)); + EXPECT_TRUE(index.IsDeleted(5)); + EXPECT_TRUE(index.IsDeleted(10)); + EXPECT_FALSE(index.IsDeleted(1)); + } + + // And for data-b.parquet (the loader filters by referenced_data_file). + { + auto result = loader.LoadPositionDeletes(delete_files, "data-b.parquet"); + ASSERT_THAT(result, IsOk()); + auto& index = result.value(); + EXPECT_EQ(index.Cardinality(), 2); + EXPECT_TRUE(index.IsDeleted(1)); + EXPECT_TRUE(index.IsDeleted(2)); + EXPECT_FALSE(index.IsDeleted(0)); + } +} + +// The PositionDeleteIndex overload bulk-adds positions for a data file. +TEST(DeletionVectorWriterTest, DeleteFromIndex) { + auto io = std::make_shared<MockFileIO>(); + auto spec = UnpartitionedSpec(); + + PositionDeleteIndex positions; + positions.Delete(0); + positions.Delete(3, 6); // [3, 6) -> 3, 4, 5 + + ICEBERG_UNWRAP_OR_FAIL(auto writer, + DeletionVectorWriter::Make(DeletionVectorWriterOptions{ + .path = "memory://from-index.puffin", .io = io})); + ASSERT_THAT(writer->Delete("data.parquet", positions, spec, PartitionValues{}), IsOk()); + ASSERT_THAT(writer->Close(), IsOk()); + + ICEBERG_UNWRAP_OR_FAIL(auto result, writer->Metadata()); + ASSERT_EQ(result.delete_files.size(), 1u); + EXPECT_EQ(result.delete_files[0]->record_count, 4); + + DeleteLoader loader(io); + auto loaded = loader.LoadPositionDeletes(result.delete_files, "data.parquet"); + ASSERT_THAT(loaded, IsOk()); + EXPECT_EQ(loaded.value().Cardinality(), 4); + EXPECT_TRUE(loaded.value().IsDeleted(0)); + EXPECT_TRUE(loaded.value().IsDeleted(5)); + EXPECT_FALSE(loaded.value().IsDeleted(6)); +} + +// Previously written deletes are merged into the new vector, and the file-scoped +// delete files they came from are reported as rewritten. +TEST(DeletionVectorWriterTest, LoadPreviousDeletesMergesAndReportsRewritten) { + auto io = std::make_shared<MockFileIO>(); + auto spec = UnpartitionedSpec(); + + auto previous_dv = std::make_shared<DataFile>(DataFile{ + .content = DataFile::Content::kPositionDeletes, + .file_path = "memory://old.puffin", + .file_format = FileFormatType::kPuffin, + .referenced_data_file = "data.parquet", + }); + + ICEBERG_UNWRAP_OR_FAIL( + auto writer, + DeletionVectorWriter::Make(DeletionVectorWriterOptions{ + .path = "memory://merged.puffin", + .io = io, + .load_previous_deletes = [&](std::string_view path) -> Result<PreviousDeletes> { + if (path != "data.parquet") { + return PreviousDeletes{}; + } + auto index = std::make_shared<PositionDeleteIndex>(); + index->Delete(100); + index->Delete(200); + return PreviousDeletes{.index = index, .delete_files = {previous_dv}}; + }, + })); + + ASSERT_THAT(writer->Delete("data.parquet", 0, spec, PartitionValues{}), IsOk()); + ASSERT_THAT(writer->Close(), IsOk()); + + ICEBERG_UNWRAP_OR_FAIL(auto result, writer->Metadata()); + ASSERT_EQ(result.delete_files.size(), 1u); + // New position plus the two previous positions. + EXPECT_EQ(result.delete_files[0]->record_count, 3); + // The previous DV is file-scoped, so it is reported for removal. + ASSERT_EQ(result.rewritten_delete_files.size(), 1u); + EXPECT_EQ(result.rewritten_delete_files[0]->file_path, "memory://old.puffin"); + + DeleteLoader loader(io); + auto loaded = loader.LoadPositionDeletes(result.delete_files, "data.parquet"); + ASSERT_THAT(loaded, IsOk()); + EXPECT_EQ(loaded.value().Cardinality(), 3); + EXPECT_TRUE(loaded.value().IsDeleted(0)); + EXPECT_TRUE(loaded.value().IsDeleted(100)); + EXPECT_TRUE(loaded.value().IsDeleted(200)); +} + +// A previous delete that is not file-scoped (e.g. a partition-scoped position +// delete) is merged into the new vector but is NOT reported as rewritten. +TEST(DeletionVectorWriterTest, PartitionScopedPreviousDeleteMergesButNotRewritten) { + auto io = std::make_shared<MockFileIO>(); + auto spec = UnpartitionedSpec(); + + // No referenced_data_file and no equal file_path bounds -> not file-scoped. + auto previous_position_delete = std::make_shared<DataFile>(DataFile{ + .content = DataFile::Content::kPositionDeletes, + .file_path = "memory://partition-deletes.parquet", + .file_format = FileFormatType::kParquet, + }); + + ICEBERG_UNWRAP_OR_FAIL( + auto writer, + DeletionVectorWriter::Make(DeletionVectorWriterOptions{ + .path = "memory://merged-partition.puffin", + .io = io, + .load_previous_deletes = [&](std::string_view path) -> Result<PreviousDeletes> { + auto index = std::make_shared<PositionDeleteIndex>(); + index->Delete(50); + return PreviousDeletes{.index = index, + .delete_files = {previous_position_delete}}; + }, + })); + + ASSERT_THAT(writer->Delete("data.parquet", 0, spec, PartitionValues{}), IsOk()); + ASSERT_THAT(writer->Close(), IsOk()); + + ICEBERG_UNWRAP_OR_FAIL(auto result, writer->Metadata()); + ASSERT_EQ(result.delete_files.size(), 1u); + // The previous position was merged in. + EXPECT_EQ(result.delete_files[0]->record_count, 2); + // The previous delete is partition-scoped, so it is not rewritten. + EXPECT_TRUE(result.rewritten_delete_files.empty()); + + DeleteLoader loader(io); + auto loaded = loader.LoadPositionDeletes(result.delete_files, "data.parquet"); + ASSERT_THAT(loaded, IsOk()); + EXPECT_EQ(loaded.value().Cardinality(), 2); + EXPECT_TRUE(loaded.value().IsDeleted(0)); + EXPECT_TRUE(loaded.value().IsDeleted(50)); +} + +TEST(DeletionVectorWriterTest, EmptyWriterProducesNoDataFiles) { + auto io = std::make_shared<MockFileIO>(); + ICEBERG_UNWRAP_OR_FAIL(auto writer, + DeletionVectorWriter::Make(DeletionVectorWriterOptions{ + .path = "memory://empty.puffin", .io = io})); + ASSERT_THAT(writer->Close(), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto result, writer->Metadata()); + EXPECT_TRUE(result.delete_files.empty()); + + // No blobs were written, so no (orphan) Puffin file should have been created. + EXPECT_THAT(io->NewInputFile("memory://empty.puffin"), IsError(ErrorKind::kNotFound)); +} + +TEST(DeletionVectorWriterTest, DeleteRejectsInvalidPosition) { Review Comment: Please rename this test or add real invalid-position cases. It only checks an empty referenced path, while negative and out-of-range positions are the Java-parity cases. ########## src/iceberg/test/deletion_vector_writer_test.cc: ########## @@ -0,0 +1,303 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +#include "iceberg/data/deletion_vector_writer.h" + +#include <memory> +#include <string> +#include <vector> + +#include <gtest/gtest.h> + +#include "iceberg/data/delete_loader.h" +#include "iceberg/deletes/position_delete_index.h" +#include "iceberg/deletes/roaring_position_bitmap.h" +#include "iceberg/file_format.h" +#include "iceberg/manifest/manifest_entry.h" +#include "iceberg/partition_spec.h" +#include "iceberg/row/partition_values.h" +#include "iceberg/test/matchers.h" +#include "iceberg/test/mock_io.h" + +namespace iceberg { + +namespace { + +std::shared_ptr<DataFile> FindByReferencedFile( + const std::vector<std::shared_ptr<DataFile>>& files, const std::string& ref) { + for (const auto& file : files) { + if (file->referenced_data_file == ref) { + return file; + } + } + return nullptr; +} + +std::shared_ptr<PartitionSpec> UnpartitionedSpec() { + return PartitionSpec::Unpartitioned(); +} + +} // namespace + +// Full write -> read round trip: write deletion vectors with the writer, then +// load them back through DeleteLoader using the produced DataFile metadata. +TEST(DeletionVectorWriterTest, WriteThenLoadEndToEnd) { + auto io = std::make_shared<MockFileIO>(); + auto spec = UnpartitionedSpec(); + + std::vector<std::shared_ptr<DataFile>> delete_files; + { + ICEBERG_UNWRAP_OR_FAIL(auto writer, + DeletionVectorWriter::Make(DeletionVectorWriterOptions{ + .path = "memory://deletes.puffin", + .io = io, + .properties = {{"created-by", "iceberg-cpp-test"}}, + })); + + ASSERT_THAT(writer->Delete("data-a.parquet", 0, spec, PartitionValues{}), IsOk()); + ASSERT_THAT(writer->Delete("data-a.parquet", 5, spec, PartitionValues{}), IsOk()); + ASSERT_THAT(writer->Delete("data-a.parquet", 10, spec, PartitionValues{}), IsOk()); + ASSERT_THAT(writer->Delete("data-b.parquet", 1, spec, PartitionValues{}), IsOk()); + ASSERT_THAT(writer->Delete("data-b.parquet", 2, spec, PartitionValues{}), IsOk()); + ASSERT_THAT(writer->Close(), IsOk()); + + ICEBERG_UNWRAP_OR_FAIL(auto result, writer->Metadata()); + delete_files = result.delete_files; + // Each referenced data file is reported once. + EXPECT_EQ(result.referenced_data_files.size(), 2u); + // No previous deletes were loaded, so nothing was rewritten. + EXPECT_TRUE(result.rewritten_delete_files.empty()); + } + + // One DataFile per referenced data file. + ASSERT_EQ(delete_files.size(), 2u); + + auto dv_a = FindByReferencedFile(delete_files, "data-a.parquet"); + auto dv_b = FindByReferencedFile(delete_files, "data-b.parquet"); + ASSERT_NE(dv_a, nullptr); + ASSERT_NE(dv_b, nullptr); + + // Metadata is spec-compliant for a deletion vector. + EXPECT_EQ(dv_a->content, DataFile::Content::kPositionDeletes); + EXPECT_EQ(dv_a->file_format, FileFormatType::kPuffin); + EXPECT_TRUE(dv_a->IsDeletionVector()); + EXPECT_EQ(dv_a->file_path, "memory://deletes.puffin"); + EXPECT_EQ(dv_a->record_count, 3); + EXPECT_TRUE(dv_a->content_offset.has_value()); + EXPECT_TRUE(dv_a->content_size_in_bytes.has_value()); + EXPECT_GT(dv_a->file_size_in_bytes, 0); + EXPECT_EQ(dv_a->partition_spec_id, spec->spec_id()); + EXPECT_EQ(dv_b->record_count, 2); + + // Both blobs live in the same Puffin file but at different offsets. + EXPECT_EQ(dv_a->file_path, dv_b->file_path); + EXPECT_NE(dv_a->content_offset.value(), dv_b->content_offset.value()); + + // Read back through DeleteLoader for data-a.parquet. + DeleteLoader loader(io); + { + auto result = loader.LoadPositionDeletes(delete_files, "data-a.parquet"); + ASSERT_THAT(result, IsOk()); + auto& index = result.value(); + EXPECT_EQ(index.Cardinality(), 3); + EXPECT_TRUE(index.IsDeleted(0)); + EXPECT_TRUE(index.IsDeleted(5)); + EXPECT_TRUE(index.IsDeleted(10)); + EXPECT_FALSE(index.IsDeleted(1)); + } + + // And for data-b.parquet (the loader filters by referenced_data_file). + { + auto result = loader.LoadPositionDeletes(delete_files, "data-b.parquet"); + ASSERT_THAT(result, IsOk()); + auto& index = result.value(); + EXPECT_EQ(index.Cardinality(), 2); + EXPECT_TRUE(index.IsDeleted(1)); + EXPECT_TRUE(index.IsDeleted(2)); + EXPECT_FALSE(index.IsDeleted(0)); + } +} + +// The PositionDeleteIndex overload bulk-adds positions for a data file. +TEST(DeletionVectorWriterTest, DeleteFromIndex) { + auto io = std::make_shared<MockFileIO>(); + auto spec = UnpartitionedSpec(); + + PositionDeleteIndex positions; + positions.Delete(0); + positions.Delete(3, 6); // [3, 6) -> 3, 4, 5 + + ICEBERG_UNWRAP_OR_FAIL(auto writer, + DeletionVectorWriter::Make(DeletionVectorWriterOptions{ + .path = "memory://from-index.puffin", .io = io})); + ASSERT_THAT(writer->Delete("data.parquet", positions, spec, PartitionValues{}), IsOk()); + ASSERT_THAT(writer->Close(), IsOk()); + + ICEBERG_UNWRAP_OR_FAIL(auto result, writer->Metadata()); + ASSERT_EQ(result.delete_files.size(), 1u); + EXPECT_EQ(result.delete_files[0]->record_count, 4); + + DeleteLoader loader(io); + auto loaded = loader.LoadPositionDeletes(result.delete_files, "data.parquet"); + ASSERT_THAT(loaded, IsOk()); + EXPECT_EQ(loaded.value().Cardinality(), 4); + EXPECT_TRUE(loaded.value().IsDeleted(0)); + EXPECT_TRUE(loaded.value().IsDeleted(5)); + EXPECT_FALSE(loaded.value().IsDeleted(6)); +} + +// Previously written deletes are merged into the new vector, and the file-scoped +// delete files they came from are reported as rewritten. +TEST(DeletionVectorWriterTest, LoadPreviousDeletesMergesAndReportsRewritten) { + auto io = std::make_shared<MockFileIO>(); + auto spec = UnpartitionedSpec(); + + auto previous_dv = std::make_shared<DataFile>(DataFile{ + .content = DataFile::Content::kPositionDeletes, + .file_path = "memory://old.puffin", + .file_format = FileFormatType::kPuffin, + .referenced_data_file = "data.parquet", + }); + + ICEBERG_UNWRAP_OR_FAIL( + auto writer, + DeletionVectorWriter::Make(DeletionVectorWriterOptions{ + .path = "memory://merged.puffin", + .io = io, + .load_previous_deletes = [&](std::string_view path) -> Result<PreviousDeletes> { Review Comment: Please add a test where load_previous_deletes returns an error. Close should propagate it and should not return partial delete metadata. ########## src/iceberg/test/position_delete_index_test.cc: ########## @@ -200,4 +205,89 @@ TEST(PositionDeleteIndexTest, TestMergeIdempotence) { ASSERT_TRUE(index1.IsDeleted(20)); } +// ==================== deletion-vector-v1 serialization ==================== + +TEST(PositionDeleteIndexTest, SerializeRoundTrip) { + PositionDeleteIndex index; + for (int64_t pos : + {int64_t{0}, int64_t{1}, int64_t{5}, int64_t{100}, int64_t{4'000'000'000}}) { + index.Delete(pos); + } + + ICEBERG_UNWRAP_OR_FAIL(auto blob, index.Serialize()); + ICEBERG_UNWRAP_OR_FAIL(auto restored, PositionDeleteIndex::Deserialize(blob)); + + EXPECT_EQ(restored.Cardinality(), 5); + for (int64_t pos : + {int64_t{0}, int64_t{1}, int64_t{5}, int64_t{100}, int64_t{4'000'000'000}}) { + EXPECT_TRUE(restored.IsDeleted(pos)); + } +} + +TEST(PositionDeleteIndexTest, SerializeEmptyRoundTrip) { + PositionDeleteIndex index; + ICEBERG_UNWRAP_OR_FAIL(auto blob, index.Serialize()); + ICEBERG_UNWRAP_OR_FAIL(auto restored, PositionDeleteIndex::Deserialize(blob)); + EXPECT_TRUE(restored.IsEmpty()); +} + +// Spans two high-32-bit keys and exercises all Roaring container types +// (sparse "array", dense "bitset", and run containers after optimization). +TEST(PositionDeleteIndexTest, SerializeAllContainerTypesAcrossKeys) { + constexpr int64_t kKeyStride = 0x100000000LL; // 2^32: high-32-bit key + constexpr int64_t kContainerStride = 1 << 16; // 2^16: Roaring container + auto pos = [](int64_t key, int64_t container, int64_t value) { + return key * kKeyStride + container * kContainerStride + value; + }; + + PositionDeleteIndex index; + int64_t expected = 0; + for (int64_t key : {int64_t{0}, int64_t{1}}) { + index.Delete(pos(key, 0, 5)); + index.Delete(pos(key, 0, 7)); + expected += 2; + index.Delete(pos(key, 1, 1), pos(key, 1, 1000)); + expected += 999; + index.Delete(pos(key, 2, 1), pos(key, 2, kContainerStride)); + expected += kContainerStride - 1; + } + + ICEBERG_UNWRAP_OR_FAIL(auto blob, index.Serialize()); + ICEBERG_UNWRAP_OR_FAIL(auto restored, PositionDeleteIndex::Deserialize(blob)); + + EXPECT_EQ(restored.Cardinality(), expected); + EXPECT_TRUE(restored.IsDeleted(pos(0, 0, 5))); + EXPECT_TRUE(restored.IsDeleted(pos(1, 2, kContainerStride - 1))); + EXPECT_TRUE(restored.IsDeleted(pos(0, 1, 999))); + EXPECT_FALSE(restored.IsDeleted(pos(0, 0, 6))); + EXPECT_FALSE(restored.IsDeleted(pos(1, 1, 1000))); // range end is exclusive +} + +TEST(PositionDeleteIndexTest, DeserializeRejectsCorruptedCrc) { + PositionDeleteIndex index; + index.Delete(1); + index.Delete(2); + ICEBERG_UNWRAP_OR_FAIL(auto blob, index.Serialize()); + + blob.back() ^= 0xFF; + EXPECT_THAT(PositionDeleteIndex::Deserialize(blob), + IsError(ErrorKind::kInvalidArgument)); +} + +TEST(PositionDeleteIndexTest, DeserializeRejectsBadMagic) { + PositionDeleteIndex index; + index.Delete(1); + ICEBERG_UNWRAP_OR_FAIL(auto blob, index.Serialize()); + + blob[4] = 0x00; + EXPECT_THAT(PositionDeleteIndex::Deserialize(blob), + IsError(ErrorKind::kInvalidArgument)); +} + +TEST(PositionDeleteIndexTest, DeserializeRejectsTruncatedBlob) { Review Comment: Please add a blob-size mismatch case, not only a too-short blob. Java validates the length prefix against the DV content size, and this path should stay covered. ########## src/iceberg/test/delete_loader_test.cc: ########## @@ -239,15 +256,73 @@ TEST_F(DeleteLoaderTest, LoadPositionDeletesFastPathHonorsReferencedDataFile) { ASSERT_FALSE(index.IsDeleted(kRowCount)); } -TEST_F(DeleteLoaderTest, LoadPositionDeletesRejectsDV) { +TEST_F(DeleteLoaderTest, LoadDeletionVector) { + auto dv_file = + WriteDeletionVector("dv-a.puffin", "data.parquet", {0, 5, 10, 4'000'000'000LL}); + + std::vector<std::shared_ptr<DataFile>> files = {dv_file}; + auto result = loader_->LoadPositionDeletes(files, "data.parquet"); + ASSERT_THAT(result, IsOk()); + + auto& index = result.value(); + ASSERT_EQ(index.Cardinality(), 4); + ASSERT_TRUE(index.IsDeleted(0)); + ASSERT_TRUE(index.IsDeleted(5)); + ASSERT_TRUE(index.IsDeleted(10)); + ASSERT_TRUE(index.IsDeleted(4'000'000'000LL)); + ASSERT_FALSE(index.IsDeleted(1)); +} + +TEST_F(DeleteLoaderTest, LoadDeletionVectorSkipsMismatchedReferencedDataFile) { + auto dv_file = WriteDeletionVector("dv-b.puffin", "other-data.parquet", {1, 2, 3}); + + std::vector<std::shared_ptr<DataFile>> files = {dv_file}; + auto result = loader_->LoadPositionDeletes(files, "data.parquet"); + ASSERT_THAT(result, IsOk()); + ASSERT_TRUE(result.value().IsEmpty()); +} + +TEST_F(DeleteLoaderTest, LoadDeletionVectorRequiresContentOffsetAndSize) { auto dv_file = std::make_shared<DataFile>(DataFile{ .content = DataFile::Content::kPositionDeletes, .file_path = "dv.puffin", .file_format = FileFormatType::kPuffin, + .referenced_data_file = "data.parquet", }); std::vector<std::shared_ptr<DataFile>> files = {dv_file}; auto result = loader_->LoadPositionDeletes(files, "data.parquet"); - ASSERT_THAT(result, IsError(ErrorKind::kNotSupported)); + ASSERT_THAT(result, IsError(ErrorKind::kInvalidArgument)); +} + +TEST_F(DeleteLoaderTest, LoadDeletionVectorRejectsCardinalityMismatch) { + auto dv_file = WriteDeletionVector("dv-card.puffin", "data.parquet", {0, 1, 2}); + // Corrupt the recorded cardinality so it no longer matches the bitmap. + dv_file->record_count = 99; + + std::vector<std::shared_ptr<DataFile>> files = {dv_file}; + auto result = loader_->LoadPositionDeletes(files, "data.parquet"); + ASSERT_THAT(result, IsError(ErrorKind::kInvalidArgument)); +} + +// Iceberg uses either a deletion vector or position deletes for a data file, not Review Comment: Please remove this mixed DV + position delete case or move it to DeleteFileIndex if needed. A real scan should not pass matching position deletes once a DV applies, so this loader test teaches the wrong contract. ########## src/iceberg/test/deletion_vector_writer_test.cc: ########## @@ -0,0 +1,303 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +#include "iceberg/data/deletion_vector_writer.h" + +#include <memory> +#include <string> +#include <vector> + +#include <gtest/gtest.h> + +#include "iceberg/data/delete_loader.h" +#include "iceberg/deletes/position_delete_index.h" +#include "iceberg/deletes/roaring_position_bitmap.h" +#include "iceberg/file_format.h" +#include "iceberg/manifest/manifest_entry.h" +#include "iceberg/partition_spec.h" +#include "iceberg/row/partition_values.h" +#include "iceberg/test/matchers.h" +#include "iceberg/test/mock_io.h" + +namespace iceberg { + +namespace { + +std::shared_ptr<DataFile> FindByReferencedFile( + const std::vector<std::shared_ptr<DataFile>>& files, const std::string& ref) { + for (const auto& file : files) { + if (file->referenced_data_file == ref) { + return file; + } + } + return nullptr; +} + +std::shared_ptr<PartitionSpec> UnpartitionedSpec() { + return PartitionSpec::Unpartitioned(); +} + +} // namespace + +// Full write -> read round trip: write deletion vectors with the writer, then Review Comment: Please trim these narrative comments. The test name and assertions already explain the flow, so this adds noise. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
