general cleanup

Author: rambleraptor

pyiceberg/manifest.py

@@ -920,16 +920,18 @@ def fetch_manifest_entry(
             read_enums={0: ManifestEntryStatus, 101: FileFormat, 134: DataFileContent},
         ) as reader:
             result = []
-            next_row_id = self.first_row_id
+            next_row_id = self.first_row_id if self.content == ManifestContent.DATA else None
 
             for entry in reader:
                 if discard_deleted and entry.status == ManifestEntryStatus.DELETED:
                     continue
                 _inherit_from_manifest(entry, self)
 
-                # Data files without a first_row_id inherit one from the manifest, assigned in file order
+                # Inherit first row IDs, which are only valid when a data manifest has a first_row_id
                 data_file = entry.data_file
-                if next_row_id is not None and entry.status != ManifestEntryStatus.DELETED and data_file.first_row_id is None:
+                if next_row_id is None:
+                    data_file.first_row_id = None
+                elif entry.status != ManifestEntryStatus.DELETED and data_file.first_row_id is None:
                     data_file.first_row_id = next_row_id
                     next_row_id += data_file.record_count
 

tests/integration/test_reads.py

@@ -1257,7 +1257,6 @@ def test_initial_default(catalog: Catalog, spark: SparkSession) -> None:
 @pytest.mark.integration
 @pytest.mark.parametrize("catalog", [lf("session_catalog")])
 def test_read_first_row_ids_written_by_spark(catalog: Catalog, spark: SparkSession) -> None:
-    """Data files read by PyIceberg get the same first row IDs that Spark reports through `_row_id`."""
     identifier = "default.test_read_first_row_ids_written_by_spark"
     spark.sql(f"DROP TABLE IF EXISTS {identifier}")
     spark.sql(f"CREATE TABLE {identifier} (id int) USING ICEBERG TBLPROPERTIES ('format-version'='3')")

tests/table/test_init.py

@@ -1972,7 +1972,6 @@ def test_add_snapshot_update_fails_without_added_rows(table_v3: Table) -> None:
 
 
 def test_add_snapshot_update_keeps_next_row_id_unset_below_v3(table_v2: Table) -> None:
-    """`next-row-id` only exists in V3, so a V2 commit must not gain one."""
     new_snapshot = Snapshot(
         snapshot_id=25,
         parent_snapshot_id=3055729675574597004,

tests/utils/test_manifest.py

@@ -229,7 +229,7 @@ def test_fetch_manifest_entry_with_filter(generated_manifest_entry_file: str) ->
 
 
 def test_fetch_manifest_entry_inherits_first_row_id(tmp_path: Path) -> None:
-    """Data files without a first_row_id inherit one from the manifest in file order, skipping deleted entries."""
+    """Data files inherit first row IDs from data manifests in file order, and have them cleared otherwise."""
     io = PyArrowFileIO()
     manifest_path = str(tmp_path / "manifest.avro")
 
@@ -268,21 +268,25 @@ def entry(status: ManifestEntryStatus, record_count: int, first_row_id: int | No
             ]
         )
 
-    def first_row_ids(manifest_first_row_id: int | None, discard_deleted: bool) -> list[int | None]:
+    def first_row_ids(
+        manifest_first_row_id: int | None, discard_deleted: bool, content: ManifestContent = ManifestContent.DATA
+    ) -> list[int | None]:
         manifest = ManifestFile.from_args(
             manifest_path=manifest_path,
             manifest_length=0,
             partition_spec_id=0,
+            content=content,
             added_snapshot_id=25,
             sequence_number=1,
             min_sequence_number=1,
             first_row_id=manifest_first_row_id,
         )
         return [e.data_file.first_row_id for e in manifest.fetch_manifest_entry(io, discard_deleted=discard_deleted)]
 
-    assert first_row_ids(None, discard_deleted=False) == [None, 500, None, None]
     assert first_row_ids(1000, discard_deleted=False) == [1000, 500, None, 1010]
     assert first_row_ids(1000, discard_deleted=True) == [1000, 500, 1010]
+    assert first_row_ids(None, discard_deleted=False) == [None, None, None, None]
+    assert first_row_ids(1000, discard_deleted=False, content=ManifestContent.DELETES) == [None, None, None, None]
 
 
 def test_read_manifest_entry_v3_fields(tmp_path: Path) -> None:
@@ -314,6 +318,7 @@ def write_and_read(file_name: str, data_file: DataFile) -> DataFile:
             added_snapshot_id=25,
             sequence_number=1,
             min_sequence_number=1,
+            first_row_id=0,
         )
         return manifest.fetch_manifest_entry(io)[0].data_file