You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "stevie9868 (via GitHub)" <gi...@apache.org> on 2025/04/21 17:11:10 UTC
[PR] refactor partition_summary_limit into SnapshotSummaryCollector constr… [iceberg-python]
stevie9868 opened a new pull request, #1940:
URL: https://github.com/apache/iceberg-python/pull/1940
<!--
Thanks for opening a pull request!
-->
<!-- In the case this PR will resolve an issue, please replace ${GITHUB_ISSUE_ID} below with the actual Github issue id. -->
Closes #1779
# Rationale for this change
See [issue](https://github.com/apache/iceberg-python/issues/1779#issuecomment-2816700603)
# Are these changes tested?
Tested locally
# Are there any user-facing changes?
<!-- In the case of user-facing changes, please add the changelog label. -->
--
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] refactor partition_summary_limit into SnapshotSummaryCollector constr… [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #1940:
URL: https://github.com/apache/iceberg-python/pull/1940#discussion_r2061602570
##########
pyiceberg/table/snapshots.py:
##########
@@ -272,10 +272,10 @@ class SnapshotSummaryCollector:
partition_metrics: DefaultDict[str, UpdateMetrics]
max_changed_partitions_for_summaries: int
- def __init__(self) -> None:
+ def __init__(self, partition_summary_limit: int = 0) -> None:
self.metrics = UpdateMetrics()
self.partition_metrics = defaultdict(UpdateMetrics)
- self.max_changed_partitions_for_summaries = 0
+ self.max_changed_partitions_for_summaries = partition_summary_limit
def set_partition_summary_limit(self, limit: int) -> None:
Review Comment:
Deprecation means that we first signal the user, see: https://py.iceberg.apache.org/contributing/#api-compatibility, and in the next release, we can remove it. I don't have strong feelings around removing this one.
--
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] refactor partition_summary_limit into SnapshotSummaryCollector constr… [iceberg-python]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu merged PR #1940:
URL: https://github.com/apache/iceberg-python/pull/1940
--
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] refactor partition_summary_limit into SnapshotSummaryCollector constr… [iceberg-python]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on PR #1940:
URL: https://github.com/apache/iceberg-python/pull/1940#issuecomment-2836053049
Thanks @stevie9868 for the PR and @Fokko 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] refactor partition_summary_limit into SnapshotSummaryCollector constr… [iceberg-python]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on code in PR #1940:
URL: https://github.com/apache/iceberg-python/pull/1940#discussion_r2056298150
##########
pyiceberg/table/snapshots.py:
##########
@@ -272,10 +272,10 @@ class SnapshotSummaryCollector:
partition_metrics: DefaultDict[str, UpdateMetrics]
max_changed_partitions_for_summaries: int
- def __init__(self) -> None:
+ def __init__(self, partition_summary_limit: int = 0) -> None:
self.metrics = UpdateMetrics()
self.partition_metrics = defaultdict(UpdateMetrics)
- self.max_changed_partitions_for_summaries = 0
+ self.max_changed_partitions_for_summaries = partition_summary_limit
def set_partition_summary_limit(self, limit: int) -> None:
Review Comment:
maybe we can deprecate this in favor of the init function
--
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] refactor partition_summary_limit into SnapshotSummaryCollector constr… [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #1940:
URL: https://github.com/apache/iceberg-python/pull/1940#discussion_r2053947917
##########
pyiceberg/table/snapshots.py:
##########
@@ -272,13 +272,10 @@ class SnapshotSummaryCollector:
partition_metrics: DefaultDict[str, UpdateMetrics]
max_changed_partitions_for_summaries: int
- def __init__(self) -> None:
+ def __init__(self, partition_summary_limit: int = 0) -> None:
self.metrics = UpdateMetrics()
self.partition_metrics = defaultdict(UpdateMetrics)
- self.max_changed_partitions_for_summaries = 0
-
- def set_partition_summary_limit(self, limit: int) -> None:
- self.max_changed_partitions_for_summaries = limit
+ self.max_changed_partitions_for_summaries = partition_summary_limit
Review Comment:
Let's also keep the method below so people that actually use this, won't get any errors:
```suggestion
self.max_changed_partitions_for_summaries = partition_summary_limit
def set_partition_summary_limit(self, limit: int) -> None:
self.max_changed_partitions_for_summaries = limit
```
--
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] refactor partition_summary_limit into SnapshotSummaryCollector constr… [iceberg-python]
Posted by "stevie9868 (via GitHub)" <gi...@apache.org>.
stevie9868 commented on PR #1940:
URL: https://github.com/apache/iceberg-python/pull/1940#issuecomment-2832299637
I have rebased and fixed the test, thanks!
--
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] refactor partition_summary_limit into SnapshotSummaryCollector constr… [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on PR #1940:
URL: https://github.com/apache/iceberg-python/pull/1940#issuecomment-2821076556
Thanks @stevie9868 for working on this 🙌 I left one comment 👍
--
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] refactor partition_summary_limit into SnapshotSummaryCollector constr… [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #1940:
URL: https://github.com/apache/iceberg-python/pull/1940#discussion_r2053947917
##########
pyiceberg/table/snapshots.py:
##########
@@ -272,13 +272,10 @@ class SnapshotSummaryCollector:
partition_metrics: DefaultDict[str, UpdateMetrics]
max_changed_partitions_for_summaries: int
- def __init__(self) -> None:
+ def __init__(self, partition_summary_limit: int = 0) -> None:
self.metrics = UpdateMetrics()
self.partition_metrics = defaultdict(UpdateMetrics)
- self.max_changed_partitions_for_summaries = 0
-
- def set_partition_summary_limit(self, limit: int) -> None:
- self.max_changed_partitions_for_summaries = limit
+ self.max_changed_partitions_for_summaries = partition_summary_limit
Review Comment:
```suggestion
self.max_changed_partitions_for_summaries = partition_summary_limit
def set_partition_summary_limit(self, limit: int) -> None:
self.max_changed_partitions_for_summaries = limit
```
--
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] refactor partition_summary_limit into SnapshotSummaryCollector constr… [iceberg-python]
Posted by "stevie9868 (via GitHub)" <gi...@apache.org>.
stevie9868 commented on PR #1940:
URL: https://github.com/apache/iceberg-python/pull/1940#issuecomment-2824642604
Thanks @Fokko !
I agree and have updated my changes.
--
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] refactor partition_summary_limit into SnapshotSummaryCollector constr… [iceberg-python]
Posted by "stevie9868 (via GitHub)" <gi...@apache.org>.
stevie9868 commented on code in PR #1940:
URL: https://github.com/apache/iceberg-python/pull/1940#discussion_r2061391541
##########
pyiceberg/table/snapshots.py:
##########
@@ -272,10 +272,10 @@ class SnapshotSummaryCollector:
partition_metrics: DefaultDict[str, UpdateMetrics]
max_changed_partitions_for_summaries: int
- def __init__(self) -> None:
+ def __init__(self, partition_summary_limit: int = 0) -> None:
self.metrics = UpdateMetrics()
self.partition_metrics = defaultdict(UpdateMetrics)
- self.max_changed_partitions_for_summaries = 0
+ self.max_changed_partitions_for_summaries = partition_summary_limit
def set_partition_summary_limit(self, limit: int) -> None:
Review Comment:
ah, I was thinking about this.
And I think @Fokko makes a good point that people might be using this function
--
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