fix: track batch position delete references by u70b3 · Pull Request #842 · apache/iceberg-cpp

u70b3 · 2026-07-24T05:04:05Z

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, null file paths, and sliced (non-zero offset) batches
cover single-file, multi-file, mixed, empty, invalid, and failed-then-successful 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 (159 tests passed)
git diff --check

u70b3 · 2026-08-11T05:54:45Z

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!

github-actions · 2026-09-11T00:45:20Z

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.

wgtmac

Thanks @u70b3 for fixing this and sorry for the delay! I have left some comments.

wgtmac · 2026-09-11T03:26:44Z

+        ArrowArrayViewSetArray(&array_view, data, &error), error);
+
+    const auto* path_view = array_view.children[0];
+    if (ArrowArrayViewComputeNullCount(path_view) != 0) {


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.

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.

wgtmac · 2026-09-11T03:27:27Z

-    // TODO(anyone): Extract file paths from ArrowArray to update referenced_paths_.
-    return writer_->Write(data);
+
+    ArrowSchema arrow_schema;


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.

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.

wgtmac · 2026-09-11T03:27:28Z

+      return InvalidArrowData("Position delete file paths must not contain null values");
+    }
+
+    std::set<std::string> pending_paths;


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.

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.

wgtmac · 2026-09-11T03:27:29Z

+  EXPECT_EQ(data_file->referenced_data_file.value(), "data_file_1.parquet");
+}
+
+TEST_F(PositionDeleteWriterTest, WriteBatchDataForMultipleFiles) {


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.

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.

wgtmac · 2026-09-11T03:27:30Z

+  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));


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.

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.

wgtmac · 2026-09-11T03:27:30Z

+
+  auto metadata_result = writer->Metadata();
+  ASSERT_THAT(metadata_result, IsOk());
+  EXPECT_FALSE(metadata_result.value().data_files[0]->referenced_data_file.has_value());


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.

Done in ce7184a. Metadata() now populates WriteResult::referenced_data_files, and both WriteBatchThenDeleteTracksAllReferencedFiles and WriteBatchDataForMultipleFiles assert it in addition to the per-file hint.

wgtmac · 2026-09-11T03:27:30Z

+              HasErrorMessage("Position delete file paths must not contain null values"));
+}
+
+TEST_F(PositionDeleteWriterTest, WriteBatchRejectsNullData) {


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.

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.

wgtmac · 2026-09-11T03:36:40Z

+    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) {


Should we error out in this case?

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.

Take ownership before validation so early-return paths honor the FileWriter contract and do not leak Arrow buffers. Assert that rejected sliced and null-path batches are released.

Build the Arrow delete schema and its array view once in the writer instead of rebuilding them for every batch, and validate the batch per row: null file paths and null positions are rejected, and an empty file path is now an error rather than an empty referenced path. Record referenced paths as they are seen using a transparent lookup and roll back the entries added by a batch that is rejected. A batch that keeps referencing the same file now costs a lookup instead of a scratch allocation per unique path, while a rejected batch still leaves no trace in the metadata. WriteResult also exposes the public referenced_data_files list now. Tests: union disjoint paths across two successful batches, assert referenced_data_files instead of only the per-file hint, and fold the null input cases into one invalid-input test that covers empty paths too.

github-actions Bot added the stale label Sep 11, 2026

wgtmac requested changes Sep 11, 2026

View reviewed changes

u70b3 added 4 commits September 12, 2026 17:32

fix: track batch position delete references

f7857c5

fix: validate batch offset and harden array view lifetime

08ae1d9

fix: release rejected position delete batches

7a0e768

Take ownership before validation so early-return paths honor the FileWriter contract and do not leak Arrow buffers. Assert that rejected sliced and null-path batches are released.

u70b3 force-pushed the fix-position-delete-batch-metadata branch from 39b9b78 to ce7184a Compare September 12, 2026 10:07

Uh oh!

Conversation

u70b3 commented Jul 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Testing

Uh oh!

u70b3 commented Aug 11, 2026

Uh oh!

github-actions Bot commented Sep 11, 2026

Uh oh!

wgtmac left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

u70b3 commented Jul 24, 2026 •

edited

Loading