Repository navigation
Conversation
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
Review effort: Lite
Findings: 3
Open (4)
PositionDeleteIndexLoaderis tightly coupled toBasicDeleteFileLoaderjust to reusefile_io()… · NewPARQUET_FIELD_ID_META_KEYmetadata in… · Newbatches[0]if it’s ever called with an empty slice. Even though current… · New
What changed in this PR
Adds a dedicated loader/index for file-scoped V2 Parquet position-delete files, validating schema/metadata and returning canonical sorted/de-duplicated positions.
Changes:
- Introduces
PositionDeleteIndex+PositionDeleteIndexLoaderto read/validate Parquet position-delete files and build an in-memory index. - Adds extensive tests for schema validation, target verification, record_count checks, and ordering/dedup behavior.
- Updates public API surface listing to expose the new loader/index types.
| File | Description |
|---|---|
| crates/iceberg/src/arrow/delete_file_loader.rs | Adds position-delete index/loader implementation and related tests. |
| crates/iceberg/public-api.txt | Exposes the new public structs and methods in the API listing. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Just a note for anyone reviewing this: I went through all four Copilot comments as well. I added the small guard in the test helper. The encrypted delete-file path is already covered by the existing encrypted positional-delete test, and I kept the projected field-id lookup as-is since Arrow/Parquet preserves those IDs in the projected schema today. I also kept the loader/metrics wiring scoped for now rather than widening this PR. |
JeonDaehong
left a comment
JeonDaehong
left a comment
There was a problem hiding this comment.
Question on the public type: PositionDeleteIndex wraps a DeleteVector but only exposes iter() as i64. Since #3287 is about rewriting file-scoped position deletes, and #3232 adds DeleteVector::serialize for the deletion-vector-v1 blob, the rewrite path would probably want the DeleteVector itself. With only iter(), it would have to rebuild one position by position.
Would it be simpler to return DeleteVector directly, or add something like into_delete_vector()? That would also avoid having a second public type for a set of deleted positions (u64 in DeleteVector, i64 here).
Yeah, avoiding a position-by-position rebuild for the rewrite path is a fair point. I’m less sure that exposing "DeleteVector" is the right API though, especially with the DV work still in progress in #3232 / #2580. I’d keep this PR scoped to the loader and decide the bulk transfer/merge API alongside that work or the rewrite integration, once the right shape is clearer. |
|
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 [email protected] list. Thank you for your contributions. |
|
This pull request has been closed due to lack of activity. This is not a judgement on the merit of the PR in any way. It is just a way of keeping the PR queue manageable. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time. |
Part of #3287.
Adds a loader for file-scoped V2 position delete files.
It reads and validates the "file_path" / "pos" fields, checks that the delete file targets the expected data file, and returns the positions in a canonical sorted form.
Also covers malformed schemas, mismatched paths, negative positions, duplicate/out-of-order positions, encrypted files, and record-count validation.
Keeping this PR limited to the loader. The rewrite/sorting writer will come separately.