[GitHub] [iceberg] rdblue commented on a diff in pull request #6933: Python: Add SnapshotUtils

["rdblue (via GitHub)" <gi...@apache.org>]

rdblue commented on code in PR #6933:
URL: https://github.com/apache/iceberg/pull/6933#discussion_r1117727207


##########
python/pyiceberg/table/__init__.py:
##########
@@ -145,12 +152,22 @@ def current_snapshot(self) -> Optional[Snapshot]:
             return self.snapshot_by_id(snapshot_id)
         return None
 
-    def snapshot_by_id(self, snapshot_id: int) -> Optional[Snapshot]:
-        """Get the snapshot of this table with the given id, or None if there is no matching snapshot."""
+    def snapshot_by_id(self, snapshot_id: int) -> Snapshot:
+        """Get the snapshot of this table with the given id.
+
+        Args:
+            snapshot_id: The id of the snapshot to lookup in the table
+
+        Returns:
+            The snapshot that corresponds to snapshot_id
+
+        Raises:
+            ValueError: If the snapshot cannot be found
+        """
         try:
             return next(snapshot for snapshot in self.metadata.snapshots if snapshot.snapshot_id == snapshot_id)
-        except StopIteration:
-            return None
+        except StopIteration as e:
+            raise ValueError(f"Snapshot id not found in table: {snapshot_id}") from e

Review Comment:
   Should we just index these in a dict and let it raise KeyError?



-- 
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: issues-unsubscribe@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org