feat(table): integrate v3 deletion-vector write, encryption, and geo bounds end-to-end#24
Open
abnobdoss wants to merge 9 commits into
Open
feat(table): integrate v3 deletion-vector write, encryption, and geo bounds end-to-end#24abnobdoss wants to merge 9 commits into
abnobdoss wants to merge 9 commits into
Conversation
added 9 commits
June 23, 2026 09:45
Enables writing v3 table metadata and implements spec-conformant row lineage (next-row-id / first-row-id / added-rows) end to end. - metadata.py: bump SUPPORTED_TABLE_FORMAT_VERSION to 3; remove the TableMetadataV3.model_dump_json NotImplementedError so v3 serializes; initialize next_row_id=0 for new v3 tables. - manifest.py: DEFAULT_READ_VERSION=3 so manifest-list first_row_id (field 520) round-trips; add ManifestWriterV3, ManifestListWriterV3 (assigns first_row_id only to DATA manifests lacking one, advancing by existing+added rows), and the ManifestFile.first_row_id accessor. - update/__init__.py: upgrade-to-v3 seeds next_row_id=0; AddSnapshotUpdate computes next_row_id = next_row_id + added_rows with NO silent fallback to None (raises instead), fixing the bug where v3 commits collapsed next_row_id to None. - update/snapshot.py: _commit derives added_rows from the manifest-list writer's row-id advance and sets first_row_id; merge manager inherits min(first_row_id) for merged data manifests and refuses to merge v3 manifests whose row-id ranges are non-contiguous/out-of-order (correctness over compaction). - tests: acceptance suite (create v3, append twice, merge-append, delete+merge gap, json round-trip) plus negative guards.
…erent upgrade gate
Remediate four defects found by the t1-foundation skeptic audit:
1. [HIGH] Copy-on-write delete no longer re-numbers surviving rows.
- Whole-file deletes (incl. shared-manifest case) materialize each survivor's
absolute _row_id into DataFile field 142 and inherit the source manifest's
first_row_id, so next_row_id and survivors' row ids are preserved.
- Partial deletes that need a physical data-file rewrite now FAIL LOUDLY on v3
(NotImplementedError) instead of silently re-numbering survivors, because
PyIceberg has no read-side _row_id materialization yet.
2. [HIGH] v3 manifest merge was silently disabled by a descending-vs-ascending
ordering check in _v3_row_ids_are_contiguous. The check now sorts ranges and
verifies gapless/non-overlapping regardless of input order; _create_manifest
writes merged entries in ascending row-id order so min(first_row_id) inheritance
is correct. The apache#3070 double-count fix is now actually exercised.
3. [MED] upgrade_table_version gate hardcoded {1,2}; now uses
SUPPORTED_TABLE_FORMAT_VERSION so v2->v3 upgrade works end to end (seeds
next_row_id=0), consistent with create-allows-v3.
4. [LOW] The two mislabeled "merge" tests now instrument _create_manifest and
assert a merge actually runs; added whole-file delete-lineage tests asserting
field-142 _row_id values, a loud-fail test, a v2 regression test, and an
upgrade test.
DataFile gains a first_row_id (field 142) accessor and a __copy__ that isolates
its _data list so lineage materialization does not leak into source DataFiles.
Address skeptic re-audit LOW nit: from_args always allocates the full v3-width record, so the length-based "non-v3" guard never fires for from_args-built files. Clarify the comment; the guard is retained only for directly-constructed short records.
Revives the deletion-vector writer from PR apache#3474 (which itself revives the stale-bot-closed apache#2822) and hardens it with adversarial round-trip tests proving the writer and the existing PuffinFile reader are true inverses. - PuffinWriter writes a spec-compliant deletion-vector-v1 Puffin blob: length(4B BE) | DV magic D1D33964 | roaring portable vector | CRC-32(4B BE). - Vector body = roaring portable 64-bit (8B LE count + per-key 4B LE key + standard 32-bit roaring bitmap from pyroaring). - Reader fix: _bitmaps_to_chunked_array pins type=pa.int64() so high-key-only DVs (leading empty chunks) round-trip instead of failing pyarrow inference. - Tests cover: empty file, empty-high-key, large/sparse (~50k), 32-bit boundary positions, duplicate dedup, independent byte-level CRC/length framing verification, and independent croaring-portability of the vector body (parsed without pyiceberg's own deserializer; cookie pinned). Integration into the commit path depends on the v3 write gate (track T1); this prototype is the writer + round-trip, decoupled from manifest/commit.
…#2118) Adds metadata-only encryption fields (no crypto/KMS): EncryptedKey model (key-id, encrypted-key-metadata, encrypted-by-id, properties) matching the Java EncryptedKeyParser; optional encryption-keys list on TableMetadataV3 and optional key-id on Snapshot. Read/deserialize + alias round-trip only; V3 full-write still raises NotImplementedError upstream (apache#1551).
… prototype Revives the standalone core of stale-closed PR apache#3067 and extends it with REAL manifest-level bbox pruning (the original PR left pruning as no-op ROWS_MIGHT_MATCH stubs). - pyiceberg/utils/geospatial.py: WKB/EWKB (ISO + EWKB SRID/Z/M flags, both byte orders) envelope extraction, GeospatialBound serde (XY/XYZ/XYM/XYZM), antimeridian-aware geography longitude intervals, and a new GeospatialStatsAggregator that accumulates min/max bounds over a column on write. serialize_geospatial_bound now rejects a genuinely-NaN z with a present m (removes XYM sentinel ambiguity). - pyiceberg/utils/geospatial_pruning.py: bbox_might_match(predicate, query_wkb, lower_bound, upper_bound, is_geography) -> bool. Returns False ONLY when it is provably impossible for any row in the file to match (no false negatives = no silent data loss). Geometry uses planar x; geography uses circular longitude-interval overlap correct at the +-180 seam. Conservative epsilon on boundary comparisons absorbs geography circle round-trip drift. Adversarial review caught: (HIGH) +-180 antimeridian boundary wrongly pruned; (HIGH, found by fuzz) geography circle round-trip drift pruned a file's own edge points (~21% false-negative rate); (MED) weak negative tests; (MED) XYM NaN-z sentinel collision. All fixed with regression + property/fuzz tests asserting zero false negatives over 4000+ random files x 4 predicates. Standalone: stats/bound/predicate-pruning logic is independent of the T1 write gate (needed only to persist bounds into a real geo-table commit).
…ation Compose three v3 prototypes onto the working v3 write path (foundation branch v3/t1-foundation-v2) and demonstrate real end-to-end v3 commits. Deletion vectors (end-to-end): - PuffinWriter.finish() now exposes written blob metadata (offset/length). - New write_deletion_vector() helper builds a DV DataFile (content= POSITION_DELETES, file_format=PUFFIN) carrying referenced_data_file (143), content_offset (144), content_size_in_bytes (145). No separate DataFileContent enum is added: the spec models a v3 DV as a position-delete file whose physical format is Puffin, which the existing reader already routes via _read_deletes. - ManifestWriter/write_manifest now accept ManifestContent so DV entries land in a real DELETES manifest (V1 still rejects delete manifests). - New UpdateSnapshot.append_deletion_vectors() producer commits DV files into a DELETES manifest as a DELETE snapshot (v3-only). - DeleteFileIndex now matches a delete file to its data file via field 143 (referenced_data_file), then falls back to path bounds. - DataFile gains referenced_data_file/content_offset/content_size_in_bytes accessors. Geospatial bounds (write path integrated; append blocked on geoarrow interop): - write_file() now runs GeospatialStatsAggregator over geometry/geography WKB columns and merges spec-serialized lower/upper bounds into the data file. Encryption keys: round-trips through a real on-disk v3 metadata file (write gate already lifted in foundation). Tests (all green): test_v3_deletion_vector_write.py (3), test_v3_encryption_metadata_write.py (2), test_v3_geo_bounds_write.py (4).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.