You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "u70b3 (via GitHub)" <gi...@apache.org> on 2026/07/24 05:04:08 UTC
[PR] fix: track batch position delete references [iceberg-cpp]
u70b3 opened a new pull request, #842:
URL: https://github.com/apache/iceberg-cpp/pull/842
## Summary
- collect referenced data-file paths when writing position deletes as an Arrow batch
- update referenced-path metadata only after the underlying write succeeds
- reject null batch inputs and null file paths
- cover single-file, multi-file, mixed, empty, and invalid batches
## Testing
- `pre-commit run --files src/iceberg/data/position_delete_writer.cc src/iceberg/test/data_writer_test.cc`
- `build-gcc14/src/iceberg/test/data_test` (157 tests passed)
- `git diff --check`
This contribution was developed with assistance from OpenAI Codex. The implementation and tests were reviewed and verified by the author.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix: track batch position delete references [iceberg-cpp]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #842:
URL: https://github.com/apache/iceberg-cpp/pull/842#issuecomment-5627629469
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix: track batch position delete references [iceberg-cpp]
Posted by "u70b3 (via GitHub)" <gi...@apache.org>.
u70b3 commented on PR #842:
URL: https://github.com/apache/iceberg-cpp/pull/842#issuecomment-5249503713
Hi @wgtmac, gentle ping for a review when you have a moment. This has been sitting for about two weeks; all CI checks are green and it merges cleanly against the latest main. The change tracks referenced data-file paths for batch position-delete writes so `referenced_data_file` and bounds metadata match the `WriteDelete` path, and hardens ownership/validation for rejected batches. Thanks!
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix: track batch position delete references [iceberg-cpp]
Posted by "u70b3 (via GitHub)" <gi...@apache.org>.
u70b3 commented on code in PR #842:
URL: https://github.com/apache/iceberg-cpp/pull/842#discussion_r3996102217
##########
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:
Done in ce7184a. `WriteBatchRejectsNullData` and `WriteBatchRejectsNullFilePath` are folded into a single `WriteBatchRejectsInvalidInput` covering a null array, a null file path, a null position, and an empty file path.
##########
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:
Good catch, it is now an error in ce7184a: `InvalidArrowData("Position delete file paths must not be empty")`. Silently tracking an empty path would have been wrong, and it is covered by `WriteBatchRejectsInvalidInput`.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix: track batch position delete references [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac merged PR #842:
URL: https://github.com/apache/iceberg-cpp/pull/842
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix: track batch position delete references [iceberg-cpp]
Posted by "u70b3 (via GitHub)" <gi...@apache.org>.
u70b3 commented on code in PR #842:
URL: https://github.com/apache/iceberg-cpp/pull/842#discussion_r3996101704
##########
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:
Done in ce7184a. The null check is now per row over `i < data->length` using `ArrowArrayViewIsNull`, applied to both the `file_path` and the `pos` child views, so it no longer depends on the child view length.
##########
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:
Done in ce7184a. The delete schema is immutable, so it is converted to Arrow once in `Impl::InitSchema()` and the `ArrowArrayView` is initialized from it there. `Write` now only rebinds the existing view with `ArrowArrayViewSetArray`, and `FlushBuffer` reuses the same Arrow schema instead of rebuilding it. The view and the schema are released in `Impl`'s destructor.
##########
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:
Done in ce7184a, with a slightly different approach. A reusable scratch set does not actually avoid the churn, since `clear()` frees every node, so the allocations would keep coming for paths that are already tracked.
Instead the paths are recorded optimistically: `referenced_paths_` is now `std::set<std::string, std::less<>>`, so a `string_view` that is already tracked costs only a lookup and no allocation at all. The iterators of the entries inserted by the current batch are kept in a small reused vector and erased again if the batch is rejected, which keeps the failure-safe delayed merge: a rejected batch still leaves no trace in the metadata.
##########
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:
Done in ce7184a. `WriteBatchDataForMultipleFiles` now performs two successful writes with disjoint paths, so a bug that replaced `referenced_paths_` instead of unioning across batches would fail the test. It also asserts the public `WriteResult::referenced_data_files`.
##########
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:
Agreed, reworked in ce7184a. The test now writes a successful batch first and a rejected batch last, so the writer is never used after a failure. The rejected batch references a valid path and is then rejected by the null path, and after `Close()` the test verifies that only `data_file_1.parquet` is tracked. Because the bad path is inserted before the batch fails, this also covers the rollback path.
##########
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:
Done in ce7184a. `Metadata()` now populates `WriteResult::referenced_data_files`, and both `WriteBatchThenDeleteTracksAllReferencedFiles` and `WriteBatchDataForMultipleFiles` assert it in addition to the per-file hint.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix: track batch position delete references [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org