amogh-jahagirdar commented on code in PR #247:
URL: https://github.com/apache/iceberg-python/pull/247#discussion_r1439114924
##########
pyiceberg/table/__init__.py:
##########
@@ -942,15 +942,16 @@ def snapshot(self) -> Optional[Snapshot]:
return self.table.current_snapshot()
def projection(self) -> Schema:
- snapshot_schema = self.table.schema()
- if snapshot := self.snapshot():
- if snapshot.schema_id is not None:
- snapshot_schema = self.table.schemas()[snapshot.schema_id]
+ current_schema = self.table.schema()
+ if self.snapshot_id is not None:
+ snapshot = self.table.snapshot_by_id(self.snapshot_id)
Review Comment:
I think it would be an invalid state if snapshot is `None` but a
`snapshot_id` is set, should we throw?
Maybe we could consider a `schema_for(snapshot_id)` API similar to
https://github.com/apache/iceberg/blob/main/core/src/main/java/org/apache/iceberg/util/SnapshotUtil.java#L368
.
I think there's a difference in the Java implementation and Python
implementation on the case where there is a schema ID on the snapshot but for
whatever reason the schema with that ID cannot be found. In the `schemaFor`
Java API implementation we throw, but here we fall back to the latest. I think
we should probably throw rather than assume the latest in that case because
that implies there is some bad metadata and it's safer to fail than coerce to
the latest schema. I think latest should only be used when there is no schema
ID on the snapshot and the original case when there is no `snapshot_id` set.
What do you think?
--
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-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
