refactor partition_summary_limit into SnapshotSummaryCollector constr… #1940
Conversation
|
|
||
| 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 |
There was a problem hiding this comment.
Let's also keep the method below so people that actually use this, won't get any errors:
| self.max_changed_partitions_for_summaries = partition_summary_limit | |
| 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 |
|
Thanks @stevie9868 for working on this 🙌 I left one comment 👍 |
|
Thanks @Fokko ! |
| 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: |
There was a problem hiding this comment.
maybe we can deprecate this in favor of the init function
There was a problem hiding this comment.
ah, I was thinking about this.
And I think @Fokko makes a good point that people might be using this function
There was a problem hiding this 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.
yingjianwu98
force-pushed
the
yingjianw/addPartitionSummaryLimitIntoSCCInit
branch
from
2542c5c to
e321bc4
Compare
|
I have rebased and fixed the test, thanks! |
|
Thanks @stevie9868 for the PR and @Fokko for the review :) |
apache#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 apache#1779 # Rationale for this change See [issue](apache#1779 (comment)) # Are these changes tested? Tested locally # Are there any user-facing changes? No <!-- In the case of user-facing changes, please add the changelog label. --> --------- Co-authored-by: Yingjian Wu <yingjianw@netflix.com>
3 participants
Closes #1779
Rationale for this change
See issue
Are these changes tested?
Tested locally
Are there any user-facing changes?
No