moomindani commented on PR #3551:
URL: https://github.com/apache/iceberg-python/pull/3551#issuecomment-5368298670
Picking up the thread here, since #3620 / #3621 / #3622 / #3623 are all
waiting on this one. @rambleraptor no pressure intended — mostly answering the
open review questions so this is easier to land, plus two gaps I think block it
as it stands.
**@abnobdoss on whether this enables upgrades:** it does not.
`SUPPORTED_TABLE_FORMAT_VERSION` stays at 2 and
`Transaction.upgrade_table_version` still rejects `format_version=3`, so this
PR only makes *serialization* possible: creating a new v3 table via
`create_table(properties={"format-version": "3"})` starts working, which is
exactly what the integration test change here shows. Upgrading an existing
v1/v2 table is #3622 / #3623.
**@abnobdoss on row lineage being a prerequisite:** for data it is not
reachable. `write_manifest` and `write_manifest_list`
(`pyiceberg/manifest.py:1289` and `:1424`) only handle v1 and v2 and otherwise
raise `ValueError: Cannot write manifest for table version: 3`, so no rows can
be written to a v3 table with or without this PR. The row lineage *metadata*
field is a different matter — that is gap 1 below.
**Gap 1: `next-row-id` is dropped for newly created v3 tables.** The spec
marks it required in v3 table metadata, and specifies 0 as the initial value.
On current `main`:
```python
md = new_table_metadata(schema=..., partition_spec=..., sort_order=...,
location=..., properties={"format-version": "3"})
md.next_row_id # None
"next-row-id" in md.model_dump() # False
```
`TableMetadataV3.next_row_id` defaults to `None` and `IcebergBaseModel`
serializes with `exclude_none=True`, so once the gate is removed the metadata
file we write omits a required field. Initializing it to 0 in
`new_table_metadata`'s v3 branch is a two-line fix.
**Gap 2: `encryption-keys` does not round-trip.** As mentioned in July, the
field does not exist anywhere in `pyiceberg/` and the pydantic models ignore
unknown keys, so a v3 table carrying `encryption-keys` loses them on the next
commit. While reads are the only path this is invisible; with serialization
enabled it becomes silent data loss on an encrypted table. A passthrough field
on `TableMetadataV3` would be enough, well before encryption itself is
supported.
**How would you like to proceed?** Any of these works for me: enable
maintainer edits and I will push both fixes onto this branch, or I open a PR
against your branch so authorship stays with you, or if you would rather hand
it off I can carry it forward with credit to you — I have both changes written
and tested locally. @ebyhr any preference on the `encryption-keys` passthrough,
since it reaches a bit beyond v3 metadata?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
