Reject negative positional delete positions

[fallintoplace]

Summary

• Reject negative row positions while parsing positional delete files.
• Return ErrorKind::DataInvalid instead of casting negative i64 values to huge u64 positions.
• Add a focused regression test for pos = -1.

Root cause

The positional-delete parser reads positions from an Int64Array, but inserted each value with pos as u64. That allowed malformed negative positions to wrap into very large row offsets instead of failing validation.

Tests

• cargo fmt --check
• CARGO_REGISTRIES_CRATES_IO_PROTOCOL=sparse cargo test -p iceberg test_parse_positional_deletes_rejects_negative_positions --locked
• CARGO_REGISTRIES_CRATES_IO_PROTOCOL=sparse cargo test -p iceberg arrow::caching_delete_file_loader::tests --locked

[kevinjqliu]

although it makes sense to reject negative position values, we might want to be more liberal on the read side. For example, we might want to be able to read negative values in order to repair and fix them

[viirya]

Confirmed the bug: parse_positional_deletes_record_batch_stream reads positions from an Int64Array and inserts each with pos as u64, so a negative -1 wraps to u64::MAX and silently poisons the DeleteVector's roaring bitmap rather than being rejected. The fix rejects pos < 0 with DataInvalid before the cast, which is the right call — the Iceberg spec defines pos as a non-negative row position, so a negative value means the delete file is corrupt and failing fast is correct.

The check is placed correctly (before pos as u64) and the regression test exercises the exact -1 case. LGTM.

[kevinjqliu]

I checked the Java implementation for comparison. It looks like Java is liberal at the raw positional-delete row layer: PositionDelete stores pos as a signed long, and the setter just assigns the value without validating that it is non-negative.

The strict validation happens later when the rows are converted into a PositionDeleteIndex. Deletes.toPositionIndexes reads the position as long and calls index.delete(position), which eventually reaches RoaringPositionBitmap.set, where Java validates that the position is >= 0 and within the supported max range.

So I think Rust should follow a similar split:

• raw read / repair path: preserve the original signed i64 value so malformed delete files can be inspected or repaired
• scan/apply path: reject pos < 0 before inserting into DeleteVector
• avoid pos as u64; use u64::try_from(pos) or equivalent checked conversion

The current PR fixes the wraparound bug, but because the validation is inside the parser that directly builds DeleteVector, it may make malformed delete files impossible to inspect through this path.

[viirya]

Thanks @kevinjqliu — I looked into whether the Rust side can mirror Java's split here, and I think this PR is actually validating at the same layer Java does, just with less ceremony.

The key difference: Rust has no equivalent of Java's raw PositionDelete row type. parse_positional_deletes_record_batch_stream has a single caller and its output type is HashMap<String, DeleteVector> — i.e. it goes straight from Arrow rows to the roaring bitmap. That makes this function the analogue of Java's Deletes.toPositionIndexes → RoaringPositionBitmap.set, which is exactly where Java does validate pos >= 0. There's no "raw read" path in Rust today that this validation would shadow — the only path that exists is the apply path.

So a malformed delete file isn't being made "impossible to inspect" by a path that previously could; no such inspect/repair path exists yet. Building one would mean adding a separate reader that yields raw signed i64 rows instead of a DeleteVector, which I'd argue is a distinct feature rather than something this wraparound fix should carry.

That said, two of your suggestions are worth folding in regardless:

• Use u64::try_from(pos) instead of pos as u64 so the conversion itself is checked, rather than relying on the separate pos < 0 guard. (Same end result, but the cast can't silently wrap if the guard is ever moved/removed.)
• When a raw read/repair path is eventually added, that's the place to preserve the signed value — worth a tracking issue if there's appetite for it.

[kevinjqliu]

LGTM

@viirya is right, this takes the file stream and directly creates DeleteVector; adding a check here makes sense