You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "Fokko (via GitHub)" <gi...@apache.org> on 2024/01/01 21:08:40 UTC
[PR] Correct schema behavior [iceberg-python]
Fokko opened a new pull request, #247:
URL: https://github.com/apache/iceberg-python/pull/247
When we alter the schema, we want to use the latest schema by default, except when you select a specific snapshot that has a schema-id.
--
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
Re: [PR] Correct schema behavior [iceberg-python]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #247:
URL: https://github.com/apache/iceberg-python/pull/247#discussion_r1441260336
##########
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 the warning makes sense for the missing schema ID case but what about the case where the snapshot_id is set but cannot be found (if line 948 returns `None`)? I think the only option there would be to throw because that means there was some established snapshot_id but we can't find it anymore.
--
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
Re: [PR] Correct schema behavior [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko merged PR #247:
URL: https://github.com/apache/iceberg-python/pull/247
--
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
Re: [PR] Correct schema behavior [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #247:
URL: https://github.com/apache/iceberg-python/pull/247#discussion_r1440202999
##########
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've updated the code with a warning, let me know what 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-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
Re: [PR] Correct schema behavior [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #247:
URL: https://github.com/apache/iceberg-python/pull/247#discussion_r1442296384
##########
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:
Oof, that's a good one. I think we should check if the snapshot-id is valid earlier in the process. I've added a check now, but I'll follow up with another PR to make this more strict.
--
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
Re: [PR] Correct schema behavior [iceberg-python]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
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-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
Re: [PR] Correct schema behavior [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #247:
URL: https://github.com/apache/iceberg-python/pull/247#discussion_r1440199047
##########
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:
Great catch @amogh-jahagirdar I'm not super strong on this one. Typically, I would not fail in these situations, but I agree that [raising a warning](https://docs.python.org/3/library/warnings.html#module-warnings) might be appropriate here.
I know there are thoughts of pruning old schemas, which might lead to this situation, but I would expect this to happen regularly.
--
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