You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "syun64 (via GitHub)" <gi...@apache.org> on 2024/04/12 14:27:45 UTC
[PR] Support Time Travel in InspectTable.entries [iceberg-python]
syun64 opened a new pull request, #599:
URL: https://github.com/apache/iceberg-python/pull/599
Time travellng on Metadata Tables allows for comparisons between two snapshots of Iceberg tables in many different and meaningful ways (files, partitions, etc)
Spark Iceberg API supports timetravel on all metadata tables except for snapshots
--
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] Support Time Travel in InspectTable.entries [iceberg-python]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on code in PR #599:
URL: https://github.com/apache/iceberg-python/pull/599#discussion_r1562924781
##########
pyiceberg/table/__init__.py:
##########
@@ -3253,6 +3253,15 @@ def __init__(self, tbl: Table) -> None:
except ModuleNotFoundError as e:
raise ModuleNotFoundError("For metadata operations PyArrow needs to be installed") from e
+ def _snapshot(self, snapshot_id: Optional[int] = None) -> Optional[Snapshot]:
+ if snapshot_id:
+ if snapshot := self.tbl.metadata.snapshot_by_id(snapshot_id):
+ return snapshot
+ else:
+ raise ValueError(f"Cannot find snapshot with ID {snapshot_id}")
Review Comment:
👍 make sense, thanks for the explanation
--
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] Support Time Travel in InspectTable.entries [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #599:
URL: https://github.com/apache/iceberg-python/pull/599#discussion_r1563100274
##########
pyiceberg/table/__init__.py:
##########
@@ -3253,6 +3253,15 @@ def __init__(self, tbl: Table) -> None:
except ModuleNotFoundError as e:
raise ModuleNotFoundError("For metadata operations PyArrow needs to be installed") from e
+ def _snapshot(self, snapshot_id: Optional[int] = None) -> Optional[Snapshot]:
+ if snapshot_id:
Review Comment:
Nit. Otherwise, it will fail on Snapshot ID 0.
```suggestion
if snapshot_id is not None:
```
--
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] Support Time Travel in InspectTable.entries [iceberg-python]
Posted by "syun64 (via GitHub)" <gi...@apache.org>.
syun64 commented on code in PR #599:
URL: https://github.com/apache/iceberg-python/pull/599#discussion_r1562919602
##########
pyiceberg/table/__init__.py:
##########
@@ -3253,6 +3253,15 @@ def __init__(self, tbl: Table) -> None:
except ModuleNotFoundError as e:
raise ModuleNotFoundError("For metadata operations PyArrow needs to be installed") from e
+ def _snapshot(self, snapshot_id: Optional[int] = None) -> Optional[Snapshot]:
+ if snapshot_id:
+ if snapshot := self.tbl.metadata.snapshot_by_id(snapshot_id):
+ return snapshot
+ else:
+ raise ValueError(f"Cannot find snapshot with ID {snapshot_id}")
Review Comment:
And I believe the above reason is because a newly created table isn't required to have a snapshot
##########
pyiceberg/table/__init__.py:
##########
@@ -3253,6 +3253,15 @@ def __init__(self, tbl: Table) -> None:
except ModuleNotFoundError as e:
raise ModuleNotFoundError("For metadata operations PyArrow needs to be installed") from e
+ def _snapshot(self, snapshot_id: Optional[int] = None) -> Optional[Snapshot]:
+ if snapshot_id:
+ if snapshot := self.tbl.metadata.snapshot_by_id(snapshot_id):
+ return snapshot
+ else:
+ raise ValueError(f"Cannot find snapshot with ID {snapshot_id}")
Review Comment:
And I believe the above is because a newly created table isn't required to have a snapshot
--
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] Support Time Travel in InspectTable.entries [iceberg-python]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on code in PR #599:
URL: https://github.com/apache/iceberg-python/pull/599#discussion_r1562740546
##########
pyiceberg/table/__init__.py:
##########
@@ -3253,6 +3253,15 @@ def __init__(self, tbl: Table) -> None:
except ModuleNotFoundError as e:
raise ModuleNotFoundError("For metadata operations PyArrow needs to be installed") from e
+ def _snapshot(self, snapshot_id: Optional[int] = None) -> Optional[Snapshot]:
+ if snapshot_id:
+ if snapshot := self.tbl.metadata.snapshot_by_id(snapshot_id):
+ return snapshot
+ else:
+ raise ValueError(f"Cannot find snapshot with ID {snapshot_id}")
Review Comment:
nit: if the return value is `Optional[Snapshot]`, maybe the function should not raise `ValueError`
--
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] Support Time Travel in InspectTable.entries [iceberg-python]
Posted by "syun64 (via GitHub)" <gi...@apache.org>.
syun64 commented on code in PR #599:
URL: https://github.com/apache/iceberg-python/pull/599#discussion_r1563120200
##########
pyiceberg/table/__init__.py:
##########
@@ -3253,6 +3253,15 @@ def __init__(self, tbl: Table) -> None:
except ModuleNotFoundError as e:
raise ModuleNotFoundError("For metadata operations PyArrow needs to be installed") from e
+ def _snapshot(self, snapshot_id: Optional[int] = None) -> Optional[Snapshot]:
Review Comment:
Couldn't think of a better one... is this a bit better than _snapshot? ^^;
##########
pyiceberg/table/__init__.py:
##########
@@ -3253,6 +3253,15 @@ def __init__(self, tbl: Table) -> None:
except ModuleNotFoundError as e:
raise ModuleNotFoundError("For metadata operations PyArrow needs to be installed") from e
+ def _snapshot(self, snapshot_id: Optional[int] = None) -> Optional[Snapshot]:
+ if snapshot_id:
+ if snapshot := self.tbl.metadata.snapshot_by_id(snapshot_id):
+ return snapshot
+ else:
+ raise ValueError(f"Cannot find snapshot with ID {snapshot_id}")
Review Comment:
@Fokko Sure - I thought it would be more correct to return an empty metadata table (entries, partitions, etc) if there's no snapshot in the table than raising an Exception, but this way I think we avoid extra `if` statements in each of the metadata table generating methods.
--
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] Support Time Travel in InspectTable.entries [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko merged PR #599:
URL: https://github.com/apache/iceberg-python/pull/599
--
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] Support Time Travel in InspectTable.entries [iceberg-python]
Posted by "syun64 (via GitHub)" <gi...@apache.org>.
syun64 commented on code in PR #599:
URL: https://github.com/apache/iceberg-python/pull/599#discussion_r1562951244
##########
pyiceberg/table/__init__.py:
##########
@@ -3253,6 +3253,15 @@ def __init__(self, tbl: Table) -> None:
except ModuleNotFoundError as e:
raise ModuleNotFoundError("For metadata operations PyArrow needs to be installed") from e
+ def _snapshot(self, snapshot_id: Optional[int] = None) -> Optional[Snapshot]:
+ if snapshot_id:
+ if snapshot := self.tbl.metadata.snapshot_by_id(snapshot_id):
+ return snapshot
+ else:
+ raise ValueError(f"Cannot find snapshot with ID {snapshot_id}")
Review Comment:
No problem, and thank you again for the review!
--
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] Support Time Travel in InspectTable.entries [iceberg-python]
Posted by "syun64 (via GitHub)" <gi...@apache.org>.
syun64 commented on code in PR #599:
URL: https://github.com/apache/iceberg-python/pull/599#discussion_r1562918080
##########
pyiceberg/table/__init__.py:
##########
@@ -3253,6 +3253,15 @@ def __init__(self, tbl: Table) -> None:
except ModuleNotFoundError as e:
raise ModuleNotFoundError("For metadata operations PyArrow needs to be installed") from e
+ def _snapshot(self, snapshot_id: Optional[int] = None) -> Optional[Snapshot]:
+ if snapshot_id:
+ if snapshot := self.tbl.metadata.snapshot_by_id(snapshot_id):
+ return snapshot
+ else:
+ raise ValueError(f"Cannot find snapshot with ID {snapshot_id}")
Review Comment:
Hi @kevinjqliu thanks for the review.
I thought about this, and I stand by this behavior / type annotation. This is my rationale:
1. If the user passes in `snapshot_id` - that's an indication that the user wants to look up a specific snapshot_id (instead of using the current one). Then we should look up snapshot_by_id and see if we can find the corresponding Snapshot and if we can't find the Snapshot, we should raise.
2. The output arg type is still `Optional[Snapshot]` even if we raise in the above behavior, because tbl.metadata.current_snapshot() returns `Optional[Snapshot]`. https://github.com/apache/iceberg-python/blob/5039b5d70644bc06c98349090912c6e9066d3ea1/pyiceberg/table/metadata.py#L295
--
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] Support Time Travel in InspectTable.entries [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #599:
URL: https://github.com/apache/iceberg-python/pull/599#discussion_r1563107139
##########
pyiceberg/table/__init__.py:
##########
@@ -3253,6 +3253,15 @@ def __init__(self, tbl: Table) -> None:
except ModuleNotFoundError as e:
raise ModuleNotFoundError("For metadata operations PyArrow needs to be installed") from e
+ def _snapshot(self, snapshot_id: Optional[int] = None) -> Optional[Snapshot]:
+ if snapshot_id:
+ if snapshot := self.tbl.metadata.snapshot_by_id(snapshot_id):
+ return snapshot
+ else:
+ raise ValueError(f"Cannot find snapshot with ID {snapshot_id}")
Review Comment:
I agree that we should raise an error when the snapshot cannot be found. What do you tink of updating the signature to `Snapshot`, and also raise an exception when there is no current snapshot?
--
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] Support Time Travel in InspectTable.entries [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #599:
URL: https://github.com/apache/iceberg-python/pull/599#discussion_r1563107624
##########
pyiceberg/table/__init__.py:
##########
@@ -3253,6 +3253,15 @@ def __init__(self, tbl: Table) -> None:
except ModuleNotFoundError as e:
raise ModuleNotFoundError("For metadata operations PyArrow needs to be installed") from e
+ def _snapshot(self, snapshot_id: Optional[int] = None) -> Optional[Snapshot]:
Review Comment:
Can we think of a better name that describes the function's behavior? :)
--
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