fix: track batch position delete references - #842
Conversation
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.
|
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 |
|
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. |
| ArrowArrayViewSetArray(&array_view, data, &error), error); | ||
|
|
||
| const auto* path_view = array_view.children[0]; | ||
| if (ArrowArrayViewComputeNullCount(path_view) != 0) { |
There was a problem hiding this 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.
| // TODO(anyone): Extract file paths from ArrowArray to update referenced_paths_. | ||
| return writer_->Write(data); | ||
|
|
||
| ArrowSchema arrow_schema; |
There was a problem hiding this 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.
| return InvalidArrowData("Position delete file paths must not contain null values"); | ||
| } | ||
|
|
||
| std::set<std::string> pending_paths; |
There was a problem hiding this 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.
| EXPECT_EQ(data_file->referenced_data_file.value(), "data_file_1.parquet"); | ||
| } | ||
|
|
||
| TEST_F(PositionDeleteWriterTest, WriteBatchDataForMultipleFiles) { |
There was a problem hiding this 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.
| 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)); |
There was a problem hiding this 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.
|
|
||
| auto metadata_result = writer->Metadata(); | ||
| ASSERT_THAT(metadata_result, IsOk()); | ||
| EXPECT_FALSE(metadata_result.value().data_files[0]->referenced_data_file.has_value()); |
There was a problem hiding this 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.
| HasErrorMessage("Position delete file paths must not contain null values")); | ||
| } | ||
|
|
||
| TEST_F(PositionDeleteWriterTest, WriteBatchRejectsNullData) { |
There was a problem hiding this 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.
| 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) { |
There was a problem hiding this comment.
Should we error out in this case?
2 participants
Summary
Testing
pre-commit run --files src/iceberg/data/position_delete_writer.cc src/iceberg/test/data_writer_test.ccbuild-gcc14/src/iceberg/test/data_test(159 tests passed)git diff --check