moomindani commented on code in PR #3624:
URL: https://github.com/apache/iceberg-python/pull/3624#discussion_r3634769899
##########
pyiceberg/manifest.py:
##########
Review Comment:
Fixed by adding `LATEST_MANIFEST_ENTRY_READ_VERSION` /
`LATEST_MANIFEST_LIST_READ_VERSION` (derived from `max()` of the respective
schema dicts, so they stay in sync as new format versions are added) and
switching `fetch_manifest_entry` / `read_manifest_list` to read with these
instead of `DEFAULT_READ_VERSION`. Reading an older file with the latest schema
is safe since the added fields are all optional, so Avro schema resolution
fills them with `None`. `DEFAULT_READ_VERSION` itself is unchanged and still
used for constructing/writing records, so this only touches the two read call
sites. Also added the corresponding V3 properties on `DataFile`
(`first_row_id`, `referenced_data_file`, `content_offset`,
`content_size_in_bytes`) so the values are actually exposed to callers, and
extended the round-trip test to assert on their actual values rather than just
presence.
##########
pyiceberg/manifest.py:
##########
@@ -1419,19 +1508,101 @@ def prepare_manifest(self, manifest_file:
ManifestFile) -> ManifestFile:
return wrapped_manifest_file
+class ManifestListWriterV3(ManifestListWriterV2):
+ """Writes V3 manifest lists, assigning `first_row_id` to data manifests.
+
+ Follows the spec's First Row ID Assignment: existing `first_row_id` values
are
+ preserved, delete manifests are never assigned one, and data manifests
without a
+ `first_row_id` are assigned a running value starting at the snapshot's
+ `first-row-id`, advanced by each assigned manifest's existing and added
row counts.
+ """
+
+ _next_row_id: int
+
+ def __init__(
+ self,
+ output_file: OutputFile,
+ snapshot_id: int,
+ parent_snapshot_id: int | None,
+ sequence_number: int,
+ compression: AvroCompressionCodec,
+ first_row_id: int,
+ ):
+ super().__init__(output_file, snapshot_id, parent_snapshot_id,
sequence_number, compression)
+ self._format_version = 3
+ self._meta = {
+ **self._meta,
+ "first-row-id": str(first_row_id),
+ "format-version": "3",
+ }
+ self._next_row_id = first_row_id
+
+ def __enter__(self) -> ManifestListWriter:
+ """Open the writer for writing, using the V3 record layout so
`first_row_id` is written."""
+ self._writer = AvroOutputFile[ManifestFile](
+ output_file=self._output_file,
+ record_schema=MANIFEST_LIST_FILE_SCHEMAS[3],
+ file_schema=MANIFEST_LIST_FILE_SCHEMAS[3],
+ schema_name="manifest_file",
+ metadata=self._meta,
+ )
+ self._writer.__enter__()
+ return self
+
+ @property
+ def next_row_id(self) -> int:
+ """The row ID after the last assigned one; the table's `next-row-id`
after this snapshot."""
+ return self._next_row_id
+
+ def _wrap(self, manifest_file: ManifestFile) -> ManifestFile:
+ """Rebind a manifest file to the V3 record layout.
+
+ Records not created with an explicit layout are bound to
+ MANIFEST_LIST_FILE_SCHEMAS[DEFAULT_READ_VERSION] (the from_args
default),
+ so that is the layout to zip the positional data against.
+ """
+ if len(manifest_file._data) >=
len(MANIFEST_LIST_FILE_SCHEMAS[3].fields):
+ return copy(manifest_file)
Review Comment:
Fixed — `ManifestListWriterV3._wrap` now always rebinds through `from_args`
instead of `copy()`, so mutating the returned manifest file's `first_row_id`
never leaks back into the caller's record.
One thing to flag separately: `ManifestListWriterV2.prepare_manifest` has
the same shallow-copy sharing bug (`copy(manifest_file)` there also shares
`_data`). I didn't touch it since it's pre-existing and out of scope for this
PR, but wanted to make sure it's on your radar — happy to file a follow-up
issue if useful.
--
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]
