moomindani commented on code in PR #3624:
URL: https://github.com/apache/iceberg-python/pull/3624#discussion_r3634771082
##########
pyiceberg/manifest.py:
##########
@@ -1284,18 +1295,96 @@ def prepare_entry(self, entry: ManifestEntry) ->
ManifestEntry:
return entry
+class ManifestWriterV3(ManifestWriterV2):
+ """Writes V3 manifest files.
+
+ The writer inherits the V2 sequence-number semantics; the V3 manifest entry
+ schema additionally carries `first_row_id`, `referenced_data_file`,
+ `content_offset` and `content_size_in_bytes` on the data file struct.
+
+ An optional `first_row_id` can be provided when rewriting a manifest whose
+ `first_row_id` is already known; it is carried into the produced manifest
file
+ so the manifest list writer preserves it instead of assigning a new one.
For
+ new manifests it is None and assigned when writing the manifest list.
+ """
+
+ _first_row_id: int | None
+
+ def __init__(
+ self,
+ spec: PartitionSpec,
+ schema: Schema,
+ output_file: OutputFile,
+ snapshot_id: int,
+ avro_compression: AvroCompressionCodec,
+ first_row_id: int | None = None,
+ ):
+ super().__init__(spec, schema, output_file, snapshot_id,
avro_compression)
+ self._first_row_id = first_row_id
+
+ @property
+ def version(self) -> TableVersion:
+ return 3
+
+ def to_manifest_file(self) -> ManifestFile:
+ """Return the manifest file, bound to the V3 layout and carrying
`first_row_id`."""
+ manifest_file = super().to_manifest_file()
+ args = {
+ field.name: value
+ for field, value in
zip(MANIFEST_LIST_FILE_SCHEMAS[DEFAULT_READ_VERSION].fields,
manifest_file._data, strict=True)
+ }
+ return ManifestFile.from_args(_table_format_version=3,
first_row_id=self._first_row_id, **args)
+
+ def new_writer(self) -> AvroOutputFile[ManifestEntry]:
+ # Use the V3 record layout so the V3-only data file fields are written
+ return AvroOutputFile[ManifestEntry](
+ output_file=self._output_file,
+ file_schema=self._with_partition(3),
+ record_schema=self._with_partition(3),
+ schema_name="manifest_entry",
+ metadata=self._meta,
+ )
+
+ def _wrap_data_file(self, data_file: DataFile) -> DataFile:
+ """Rebind a data file to the V3 record layout."""
+ if len(data_file._data) >= len(DATA_FILE_TYPE[3].fields):
+ return data_file
+ args = {
+ field.name: value for field, value in
zip(DATA_FILE_TYPE[DEFAULT_READ_VERSION].fields, data_file._data, strict=True)
Review Comment:
Fixed by adding `_layout_version_from_field_count`, which recovers the
source layout from the record's field count (each version's layout currently
has a distinct field count) instead of assuming V2.
I did think about how this holds up for a future V4: if V4 only adds fields
(the pattern so far), field-count inference keeps working automatically — no
code change needed beyond adding the V4 schema entry, same as how
`LATEST_MANIFEST_ENTRY_READ_VERSION` / `LATEST_MANIFEST_LIST_READ_VERSION` (see
the other thread) are derived via `max()` rather than hardcoded. But if V4 ever
lands with the same field count as an existing version, or changes a field
non-additively, field-count inference can no longer tell them apart — so I
added an explicit check that raises `Ambiguous layout` in that case instead of
silently binding to the wrong one. That failure mode felt important to guard
against explicitly rather than leave implicit.
One thing I'd like your take on: this is still inference, not the record
actually knowing its own version. The more principled fix would have
`Record`/`DataFile`/`ManifestFile` carry the format version they were bound
with, so rebinding wouldn't need to guess at all — that's how Java's manifest
records work, since they're self-describing rather than positional. I
considered that, but went with field-count inference for now since it's
consistent with the existing code's approach (the pre-existing early-return
already used a field-count threshold) and, after switching reads to the latest
schema, most records reaching this rebind path are already V3-shaped — it's now
mostly a defensive path for records constructed programmatically at an older
version, not a hot path. Changing `Record`'s core design to carry its own
version felt like the right call for a dedicated follow-up, not something to
fold into this PR's scope, but let me know if you'd weigh that differently.
--
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]
