wgtmac commented on code in PR #842:
URL: https://github.com/apache/iceberg-cpp/pull/842#discussion_r3985668761
##########
src/iceberg/data/position_delete_writer.cc:
##########
@@ -62,10 +62,43 @@ class PositionDeleteWriter::Impl {
}
Status Write(ArrowArray* data) {
+ ICEBERG_PRECHECK(data != nullptr, "Position delete data must not be null");
+ internal::ArrowArrayGuard data_guard(data);
+ ICEBERG_PRECHECK(data->offset == 0,
+ "Position delete data with a non-zero offset is not
supported");
ICEBERG_PRECHECK(buffered_paths_.empty(),
"Cannot write batch data when there are buffered
deletes.");
- // TODO(anyone): Extract file paths from ArrowArray to update
referenced_paths_.
- return writer_->Write(data);
+
+ ArrowSchema arrow_schema;
+ ICEBERG_RETURN_UNEXPECTED(ToArrowSchema(*delete_schema_, &arrow_schema));
+ internal::ArrowSchemaGuard schema_guard(&arrow_schema);
+
+ ArrowArrayView array_view;
+ ArrowError error;
+ ICEBERG_NANOARROW_RETURN_UNEXPECTED_WITH_ERROR(
+ ArrowArrayViewInitFromSchema(&array_view, &arrow_schema, &error),
error);
+ internal::ArrowArrayViewGuard view_guard(&array_view);
+ ICEBERG_NANOARROW_RETURN_UNEXPECTED_WITH_ERROR(
+ ArrowArrayViewSetArray(&array_view, data, &error), error);
+
+ const auto* path_view = array_view.children[0];
+ if (ArrowArrayViewComputeNullCount(path_view) != 0) {
Review Comment:
`ArrowArrayViewComputeNullCount(path_view)` scans the child view length, not
necessarily the parent batch length. Please check it only for i less than
data->length, and apply the same check to `pos` field.
##########
src/iceberg/data/position_delete_writer.cc:
##########
@@ -62,10 +62,43 @@ class PositionDeleteWriter::Impl {
}
Status Write(ArrowArray* data) {
+ ICEBERG_PRECHECK(data != nullptr, "Position delete data must not be null");
+ internal::ArrowArrayGuard data_guard(data);
+ ICEBERG_PRECHECK(data->offset == 0,
+ "Position delete data with a non-zero offset is not
supported");
ICEBERG_PRECHECK(buffered_paths_.empty(),
"Cannot write batch data when there are buffered
deletes.");
- // TODO(anyone): Extract file paths from ArrowArray to update
referenced_paths_.
- return writer_->Write(data);
+
+ ArrowSchema arrow_schema;
Review Comment:
Every Write rebuilds the same schema and allocates a fresh ArrowArrayView.
The schema is immutable for this writer, so this adds allocator work to every
batch. Perhaps we can initialize the schema and view once in Impl and only
rebind the incoming array here.
##########
src/iceberg/data/position_delete_writer.cc:
##########
@@ -62,10 +62,43 @@ class PositionDeleteWriter::Impl {
}
Status Write(ArrowArray* data) {
+ ICEBERG_PRECHECK(data != nullptr, "Position delete data must not be null");
+ internal::ArrowArrayGuard data_guard(data);
+ ICEBERG_PRECHECK(data->offset == 0,
+ "Position delete data with a non-zero offset is not
supported");
ICEBERG_PRECHECK(buffered_paths_.empty(),
"Cannot write batch data when there are buffered
deletes.");
- // TODO(anyone): Extract file paths from ArrowArray to update
referenced_paths_.
- return writer_->Write(data);
+
+ ArrowSchema arrow_schema;
+ ICEBERG_RETURN_UNEXPECTED(ToArrowSchema(*delete_schema_, &arrow_schema));
+ internal::ArrowSchemaGuard schema_guard(&arrow_schema);
+
+ ArrowArrayView array_view;
+ ArrowError error;
+ ICEBERG_NANOARROW_RETURN_UNEXPECTED_WITH_ERROR(
+ ArrowArrayViewInitFromSchema(&array_view, &arrow_schema, &error),
error);
+ internal::ArrowArrayViewGuard view_guard(&array_view);
+ ICEBERG_NANOARROW_RETURN_UNEXPECTED_WITH_ERROR(
+ ArrowArrayViewSetArray(&array_view, data, &error), error);
+
+ const auto* path_view = array_view.children[0];
+ if (ArrowArrayViewComputeNullCount(path_view) != 0) {
+ return InvalidArrowData("Position delete file paths must not contain
null values");
+ }
+
+ std::set<std::string> pending_paths;
Review Comment:
`pending_paths` allocates a tree node and copies each unique path for every
batch, then merges into another set. Batch writes can be frequent so a reusable
scratch set or a vector plus post-write insertion would avoid much of this
churn while keeping the failure-safe delayed merge.
##########
src/iceberg/data/position_delete_writer.cc:
##########
@@ -62,10 +62,43 @@ class PositionDeleteWriter::Impl {
}
Status Write(ArrowArray* data) {
+ ICEBERG_PRECHECK(data != nullptr, "Position delete data must not be null");
+ internal::ArrowArrayGuard data_guard(data);
+ ICEBERG_PRECHECK(data->offset == 0,
+ "Position delete data with a non-zero offset is not
supported");
ICEBERG_PRECHECK(buffered_paths_.empty(),
"Cannot write batch data when there are buffered
deletes.");
- // TODO(anyone): Extract file paths from ArrowArray to update
referenced_paths_.
- return writer_->Write(data);
+
+ ArrowSchema arrow_schema;
+ ICEBERG_RETURN_UNEXPECTED(ToArrowSchema(*delete_schema_, &arrow_schema));
+ internal::ArrowSchemaGuard schema_guard(&arrow_schema);
+
+ ArrowArrayView array_view;
+ ArrowError error;
+ ICEBERG_NANOARROW_RETURN_UNEXPECTED_WITH_ERROR(
+ ArrowArrayViewInitFromSchema(&array_view, &arrow_schema, &error),
error);
+ internal::ArrowArrayViewGuard view_guard(&array_view);
+ ICEBERG_NANOARROW_RETURN_UNEXPECTED_WITH_ERROR(
+ ArrowArrayViewSetArray(&array_view, data, &error), error);
+
+ const auto* path_view = array_view.children[0];
+ if (ArrowArrayViewComputeNullCount(path_view) != 0) {
+ return InvalidArrowData("Position delete file paths must not contain
null values");
+ }
+
+ std::set<std::string> pending_paths;
+ for (int64_t i = 0; i < data->length; ++i) {
+ auto path = ArrowArrayViewGetStringUnsafe(path_view, i);
+ if (path.size_bytes == 0) {
Review Comment:
Should we error out in this case?
##########
src/iceberg/test/data_writer_test.cc:
##########
@@ -458,6 +453,145 @@ TEST_F(PositionDeleteWriterTest, WriteBatchData) {
const auto& data_file = metadata_result.value().data_files[0];
EXPECT_EQ(data_file->content, DataFile::Content::kPositionDeletes);
EXPECT_GT(data_file->file_size_in_bytes, 0);
+ ASSERT_TRUE(data_file->referenced_data_file.has_value());
+ EXPECT_EQ(data_file->referenced_data_file.value(), "data_file_1.parquet");
+ // Bounds for delete metadata columns are kept when referencing a single
file.
+
EXPECT_TRUE(data_file->lower_bounds.contains(MetadataColumns::kDeleteFilePathColumnId));
+
EXPECT_TRUE(data_file->lower_bounds.contains(MetadataColumns::kDeleteFilePosColumnId));
+
EXPECT_TRUE(data_file->upper_bounds.contains(MetadataColumns::kDeleteFilePathColumnId));
+
EXPECT_TRUE(data_file->upper_bounds.contains(MetadataColumns::kDeleteFilePosColumnId));
+}
+
+TEST_F(PositionDeleteWriterTest, WriteBatchRejectsSlicedData) {
+ auto writer_result = PositionDeleteWriter::Make(MakeDeleteOptions());
+ ASSERT_THAT(writer_result, IsOk());
+ auto writer = std::move(writer_result.value());
+
+ auto test_data = CreatePositionDeleteData(
+ R"([["data_file_1.parquet", 0], ["data_file_1.parquet", 5]])");
+ auto sliced = test_data->Slice(1, 1);
+ ArrowArray arrow_array;
+ ASSERT_TRUE(::arrow::ExportArray(*sliced, &arrow_array).ok());
+
+ auto result = writer->Write(&arrow_array);
+ EXPECT_EQ(arrow_array.release, nullptr);
+ internal::ArrowArrayGuard array_guard(&arrow_array);
+ ASSERT_THAT(result, IsError(ErrorKind::kInvalidArgument));
+ EXPECT_THAT(
+ result,
+ HasErrorMessage("Position delete data with a non-zero offset is not
supported"));
+}
+
+TEST_F(PositionDeleteWriterTest, FailedBatchWriteDoesNotTrackReferencedFiles) {
+ auto writer_result = PositionDeleteWriter::Make(MakeDeleteOptions());
+ ASSERT_THAT(writer_result, IsOk());
+ auto writer = std::move(writer_result.value());
+
+ // A rejected batch must not contribute referenced paths.
+ auto bad_data =
+ CreatePositionDeleteData(R"([[null, 0], ["data_file_bad.parquet", 1]])");
+ ArrowArray bad_array;
+ ASSERT_TRUE(::arrow::ExportArray(*bad_data, &bad_array).ok());
+ internal::ArrowArrayGuard bad_array_guard(&bad_array);
+ ASSERT_THAT(writer->Write(&bad_array),
IsError(ErrorKind::kInvalidArrowData));
+
+ auto test_data = CreatePositionDeleteData(R"([["data_file_1.parquet", 0]])");
+ ArrowArray arrow_array;
+ ASSERT_TRUE(::arrow::ExportArray(*test_data, &arrow_array).ok());
+ ASSERT_THAT(writer->Write(&arrow_array), IsOk());
+ ASSERT_THAT(writer->Close(), IsOk());
+
+ auto metadata_result = writer->Metadata();
+ ASSERT_THAT(metadata_result, IsOk());
+
+ const auto& data_file = metadata_result.value().data_files[0];
+ ASSERT_TRUE(data_file->referenced_data_file.has_value());
+ EXPECT_EQ(data_file->referenced_data_file.value(), "data_file_1.parquet");
+}
+
+TEST_F(PositionDeleteWriterTest, WriteBatchDataForMultipleFiles) {
+ auto writer_result = PositionDeleteWriter::Make(MakeDeleteOptions());
+ ASSERT_THAT(writer_result, IsOk());
+ auto writer = std::move(writer_result.value());
+
+ auto test_data = CreatePositionDeleteData(
+ R"([["data_file_1.parquet", 0], ["data_file_2.parquet", 5]])");
+ ArrowArray arrow_array;
+ ASSERT_TRUE(::arrow::ExportArray(*test_data, &arrow_array).ok());
+ ASSERT_THAT(writer->Write(&arrow_array), IsOk());
+ ASSERT_THAT(writer->Close(), IsOk());
+
+ auto metadata_result = writer->Metadata();
+ ASSERT_THAT(metadata_result, IsOk());
+
+ const auto& data_file = metadata_result.value().data_files[0];
+ EXPECT_FALSE(data_file->referenced_data_file.has_value());
+ EXPECT_FALSE(
+
data_file->lower_bounds.contains(MetadataColumns::kDeleteFilePathColumnId));
+
EXPECT_FALSE(data_file->lower_bounds.contains(MetadataColumns::kDeleteFilePosColumnId));
+ EXPECT_FALSE(
+
data_file->upper_bounds.contains(MetadataColumns::kDeleteFilePathColumnId));
+
EXPECT_FALSE(data_file->upper_bounds.contains(MetadataColumns::kDeleteFilePosColumnId));
+}
+
+TEST_F(PositionDeleteWriterTest, WriteBatchThenDeleteTracksAllReferencedFiles)
{
+ auto writer_result = PositionDeleteWriter::Make(MakeDeleteOptions());
+ ASSERT_THAT(writer_result, IsOk());
+ auto writer = std::move(writer_result.value());
+
+ auto test_data = CreatePositionDeleteData(R"([["data_file_1.parquet", 0]])");
+ ArrowArray arrow_array;
+ ASSERT_TRUE(::arrow::ExportArray(*test_data, &arrow_array).ok());
+ ASSERT_THAT(writer->Write(&arrow_array), IsOk());
+ ASSERT_THAT(writer->WriteDelete("data_file_2.parquet", 5), IsOk());
+ ASSERT_THAT(writer->Close(), IsOk());
+
+ auto metadata_result = writer->Metadata();
+ ASSERT_THAT(metadata_result, IsOk());
+
EXPECT_FALSE(metadata_result.value().data_files[0]->referenced_data_file.has_value());
+}
+
+TEST_F(PositionDeleteWriterTest, WriteBatchRejectsNullFilePath) {
+ auto writer_result = PositionDeleteWriter::Make(MakeDeleteOptions());
+ ASSERT_THAT(writer_result, IsOk());
+ auto writer = std::move(writer_result.value());
+
+ auto test_data = CreatePositionDeleteData(R"([[null, 0]])");
+ ArrowArray arrow_array;
+ ASSERT_TRUE(::arrow::ExportArray(*test_data, &arrow_array).ok());
+
+ auto result = writer->Write(&arrow_array);
+ EXPECT_EQ(arrow_array.release, nullptr);
+ internal::ArrowArrayGuard array_guard(&arrow_array);
+ ASSERT_THAT(result, IsError(ErrorKind::kInvalidArrowData));
+ EXPECT_THAT(result,
+ HasErrorMessage("Position delete file paths must not contain
null values"));
+}
+
+TEST_F(PositionDeleteWriterTest, WriteBatchRejectsNullData) {
Review Comment:
This only exercises a one-line null precondition and does not touch batch
paths or metadata. It is low-value so please consider dropping it or folding it
into a broader invalid-input test.
##########
src/iceberg/test/data_writer_test.cc:
##########
@@ -458,6 +453,145 @@ TEST_F(PositionDeleteWriterTest, WriteBatchData) {
const auto& data_file = metadata_result.value().data_files[0];
EXPECT_EQ(data_file->content, DataFile::Content::kPositionDeletes);
EXPECT_GT(data_file->file_size_in_bytes, 0);
+ ASSERT_TRUE(data_file->referenced_data_file.has_value());
+ EXPECT_EQ(data_file->referenced_data_file.value(), "data_file_1.parquet");
+ // Bounds for delete metadata columns are kept when referencing a single
file.
+
EXPECT_TRUE(data_file->lower_bounds.contains(MetadataColumns::kDeleteFilePathColumnId));
+
EXPECT_TRUE(data_file->lower_bounds.contains(MetadataColumns::kDeleteFilePosColumnId));
+
EXPECT_TRUE(data_file->upper_bounds.contains(MetadataColumns::kDeleteFilePathColumnId));
+
EXPECT_TRUE(data_file->upper_bounds.contains(MetadataColumns::kDeleteFilePosColumnId));
+}
+
+TEST_F(PositionDeleteWriterTest, WriteBatchRejectsSlicedData) {
+ auto writer_result = PositionDeleteWriter::Make(MakeDeleteOptions());
+ ASSERT_THAT(writer_result, IsOk());
+ auto writer = std::move(writer_result.value());
+
+ auto test_data = CreatePositionDeleteData(
+ R"([["data_file_1.parquet", 0], ["data_file_1.parquet", 5]])");
+ auto sliced = test_data->Slice(1, 1);
+ ArrowArray arrow_array;
+ ASSERT_TRUE(::arrow::ExportArray(*sliced, &arrow_array).ok());
+
+ auto result = writer->Write(&arrow_array);
+ EXPECT_EQ(arrow_array.release, nullptr);
+ internal::ArrowArrayGuard array_guard(&arrow_array);
+ ASSERT_THAT(result, IsError(ErrorKind::kInvalidArgument));
+ EXPECT_THAT(
+ result,
+ HasErrorMessage("Position delete data with a non-zero offset is not
supported"));
+}
+
+TEST_F(PositionDeleteWriterTest, FailedBatchWriteDoesNotTrackReferencedFiles) {
+ auto writer_result = PositionDeleteWriter::Make(MakeDeleteOptions());
+ ASSERT_THAT(writer_result, IsOk());
+ auto writer = std::move(writer_result.value());
+
+ // A rejected batch must not contribute referenced paths.
+ auto bad_data =
+ CreatePositionDeleteData(R"([[null, 0], ["data_file_bad.parquet", 1]])");
+ ArrowArray bad_array;
+ ASSERT_TRUE(::arrow::ExportArray(*bad_data, &bad_array).ok());
+ internal::ArrowArrayGuard bad_array_guard(&bad_array);
+ ASSERT_THAT(writer->Write(&bad_array),
IsError(ErrorKind::kInvalidArrowData));
+
+ auto test_data = CreatePositionDeleteData(R"([["data_file_1.parquet", 0]])");
+ ArrowArray arrow_array;
+ ASSERT_TRUE(::arrow::ExportArray(*test_data, &arrow_array).ok());
+ ASSERT_THAT(writer->Write(&arrow_array), IsOk());
+ ASSERT_THAT(writer->Close(), IsOk());
+
+ auto metadata_result = writer->Metadata();
+ ASSERT_THAT(metadata_result, IsOk());
+
+ const auto& data_file = metadata_result.value().data_files[0];
+ ASSERT_TRUE(data_file->referenced_data_file.has_value());
+ EXPECT_EQ(data_file->referenced_data_file.value(), "data_file_1.parquet");
+}
+
+TEST_F(PositionDeleteWriterTest, WriteBatchDataForMultipleFiles) {
Review Comment:
This is one batch containing two paths, not multiple successful Write calls.
A bug that replaces referenced_paths_ instead of unioning across batches would
still pass. Add two successful batches with disjoint paths.
##########
src/iceberg/test/data_writer_test.cc:
##########
@@ -458,6 +453,145 @@ TEST_F(PositionDeleteWriterTest, WriteBatchData) {
const auto& data_file = metadata_result.value().data_files[0];
EXPECT_EQ(data_file->content, DataFile::Content::kPositionDeletes);
EXPECT_GT(data_file->file_size_in_bytes, 0);
+ ASSERT_TRUE(data_file->referenced_data_file.has_value());
+ EXPECT_EQ(data_file->referenced_data_file.value(), "data_file_1.parquet");
+ // Bounds for delete metadata columns are kept when referencing a single
file.
+
EXPECT_TRUE(data_file->lower_bounds.contains(MetadataColumns::kDeleteFilePathColumnId));
+
EXPECT_TRUE(data_file->lower_bounds.contains(MetadataColumns::kDeleteFilePosColumnId));
+
EXPECT_TRUE(data_file->upper_bounds.contains(MetadataColumns::kDeleteFilePathColumnId));
+
EXPECT_TRUE(data_file->upper_bounds.contains(MetadataColumns::kDeleteFilePosColumnId));
+}
+
+TEST_F(PositionDeleteWriterTest, WriteBatchRejectsSlicedData) {
+ auto writer_result = PositionDeleteWriter::Make(MakeDeleteOptions());
+ ASSERT_THAT(writer_result, IsOk());
+ auto writer = std::move(writer_result.value());
+
+ auto test_data = CreatePositionDeleteData(
+ R"([["data_file_1.parquet", 0], ["data_file_1.parquet", 5]])");
+ auto sliced = test_data->Slice(1, 1);
+ ArrowArray arrow_array;
+ ASSERT_TRUE(::arrow::ExportArray(*sliced, &arrow_array).ok());
+
+ auto result = writer->Write(&arrow_array);
+ EXPECT_EQ(arrow_array.release, nullptr);
+ internal::ArrowArrayGuard array_guard(&arrow_array);
+ ASSERT_THAT(result, IsError(ErrorKind::kInvalidArgument));
+ EXPECT_THAT(
+ result,
+ HasErrorMessage("Position delete data with a non-zero offset is not
supported"));
+}
+
+TEST_F(PositionDeleteWriterTest, FailedBatchWriteDoesNotTrackReferencedFiles) {
+ auto writer_result = PositionDeleteWriter::Make(MakeDeleteOptions());
+ ASSERT_THAT(writer_result, IsOk());
+ auto writer = std::move(writer_result.value());
+
+ // A rejected batch must not contribute referenced paths.
+ auto bad_data =
+ CreatePositionDeleteData(R"([[null, 0], ["data_file_bad.parquet", 1]])");
+ ArrowArray bad_array;
+ ASSERT_TRUE(::arrow::ExportArray(*bad_data, &bad_array).ok());
+ internal::ArrowArrayGuard bad_array_guard(&bad_array);
+ ASSERT_THAT(writer->Write(&bad_array),
IsError(ErrorKind::kInvalidArrowData));
Review Comment:
This looks odd to me because we continue to use a failed writer which should
not happen in production. And this does actually verify the case name
`FailedBatchWriteDoesNotTrackReferencedFiles`.
##########
src/iceberg/test/data_writer_test.cc:
##########
@@ -458,6 +453,145 @@ TEST_F(PositionDeleteWriterTest, WriteBatchData) {
const auto& data_file = metadata_result.value().data_files[0];
EXPECT_EQ(data_file->content, DataFile::Content::kPositionDeletes);
EXPECT_GT(data_file->file_size_in_bytes, 0);
+ ASSERT_TRUE(data_file->referenced_data_file.has_value());
+ EXPECT_EQ(data_file->referenced_data_file.value(), "data_file_1.parquet");
+ // Bounds for delete metadata columns are kept when referencing a single
file.
+
EXPECT_TRUE(data_file->lower_bounds.contains(MetadataColumns::kDeleteFilePathColumnId));
+
EXPECT_TRUE(data_file->lower_bounds.contains(MetadataColumns::kDeleteFilePosColumnId));
+
EXPECT_TRUE(data_file->upper_bounds.contains(MetadataColumns::kDeleteFilePathColumnId));
+
EXPECT_TRUE(data_file->upper_bounds.contains(MetadataColumns::kDeleteFilePosColumnId));
+}
+
+TEST_F(PositionDeleteWriterTest, WriteBatchRejectsSlicedData) {
+ auto writer_result = PositionDeleteWriter::Make(MakeDeleteOptions());
+ ASSERT_THAT(writer_result, IsOk());
+ auto writer = std::move(writer_result.value());
+
+ auto test_data = CreatePositionDeleteData(
+ R"([["data_file_1.parquet", 0], ["data_file_1.parquet", 5]])");
+ auto sliced = test_data->Slice(1, 1);
+ ArrowArray arrow_array;
+ ASSERT_TRUE(::arrow::ExportArray(*sliced, &arrow_array).ok());
+
+ auto result = writer->Write(&arrow_array);
+ EXPECT_EQ(arrow_array.release, nullptr);
+ internal::ArrowArrayGuard array_guard(&arrow_array);
+ ASSERT_THAT(result, IsError(ErrorKind::kInvalidArgument));
+ EXPECT_THAT(
+ result,
+ HasErrorMessage("Position delete data with a non-zero offset is not
supported"));
+}
+
+TEST_F(PositionDeleteWriterTest, FailedBatchWriteDoesNotTrackReferencedFiles) {
+ auto writer_result = PositionDeleteWriter::Make(MakeDeleteOptions());
+ ASSERT_THAT(writer_result, IsOk());
+ auto writer = std::move(writer_result.value());
+
+ // A rejected batch must not contribute referenced paths.
+ auto bad_data =
+ CreatePositionDeleteData(R"([[null, 0], ["data_file_bad.parquet", 1]])");
+ ArrowArray bad_array;
+ ASSERT_TRUE(::arrow::ExportArray(*bad_data, &bad_array).ok());
+ internal::ArrowArrayGuard bad_array_guard(&bad_array);
+ ASSERT_THAT(writer->Write(&bad_array),
IsError(ErrorKind::kInvalidArrowData));
+
+ auto test_data = CreatePositionDeleteData(R"([["data_file_1.parquet", 0]])");
+ ArrowArray arrow_array;
+ ASSERT_TRUE(::arrow::ExportArray(*test_data, &arrow_array).ok());
+ ASSERT_THAT(writer->Write(&arrow_array), IsOk());
+ ASSERT_THAT(writer->Close(), IsOk());
+
+ auto metadata_result = writer->Metadata();
+ ASSERT_THAT(metadata_result, IsOk());
+
+ const auto& data_file = metadata_result.value().data_files[0];
+ ASSERT_TRUE(data_file->referenced_data_file.has_value());
+ EXPECT_EQ(data_file->referenced_data_file.value(), "data_file_1.parquet");
+}
+
+TEST_F(PositionDeleteWriterTest, WriteBatchDataForMultipleFiles) {
+ auto writer_result = PositionDeleteWriter::Make(MakeDeleteOptions());
+ ASSERT_THAT(writer_result, IsOk());
+ auto writer = std::move(writer_result.value());
+
+ auto test_data = CreatePositionDeleteData(
+ R"([["data_file_1.parquet", 0], ["data_file_2.parquet", 5]])");
+ ArrowArray arrow_array;
+ ASSERT_TRUE(::arrow::ExportArray(*test_data, &arrow_array).ok());
+ ASSERT_THAT(writer->Write(&arrow_array), IsOk());
+ ASSERT_THAT(writer->Close(), IsOk());
+
+ auto metadata_result = writer->Metadata();
+ ASSERT_THAT(metadata_result, IsOk());
+
+ const auto& data_file = metadata_result.value().data_files[0];
+ EXPECT_FALSE(data_file->referenced_data_file.has_value());
+ EXPECT_FALSE(
+
data_file->lower_bounds.contains(MetadataColumns::kDeleteFilePathColumnId));
+
EXPECT_FALSE(data_file->lower_bounds.contains(MetadataColumns::kDeleteFilePosColumnId));
+ EXPECT_FALSE(
+
data_file->upper_bounds.contains(MetadataColumns::kDeleteFilePathColumnId));
+
EXPECT_FALSE(data_file->upper_bounds.contains(MetadataColumns::kDeleteFilePosColumnId));
+}
+
+TEST_F(PositionDeleteWriterTest, WriteBatchThenDeleteTracksAllReferencedFiles)
{
+ auto writer_result = PositionDeleteWriter::Make(MakeDeleteOptions());
+ ASSERT_THAT(writer_result, IsOk());
+ auto writer = std::move(writer_result.value());
+
+ auto test_data = CreatePositionDeleteData(R"([["data_file_1.parquet", 0]])");
+ ArrowArray arrow_array;
+ ASSERT_TRUE(::arrow::ExportArray(*test_data, &arrow_array).ok());
+ ASSERT_THAT(writer->Write(&arrow_array), IsOk());
+ ASSERT_THAT(writer->WriteDelete("data_file_2.parquet", 5), IsOk());
+ ASSERT_THAT(writer->Close(), IsOk());
+
+ auto metadata_result = writer->Metadata();
+ ASSERT_THAT(metadata_result, IsOk());
+
EXPECT_FALSE(metadata_result.value().data_files[0]->referenced_data_file.has_value());
Review Comment:
This only checks the per-file hint. WriteResult also exposes
referenced_data_files; assert that public result as well if this writer is
meant to satisfy the FileWriter contract.
--
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]
