Add RemovePartitionStatisticsUpdate and SetPartitionStatisticsUpdate #2192
Conversation
events This allows us to add, update, and remove partition statistics files.
rambleraptor
force-pushed
the
partition_statistics_events
branch
from
42a82ca to
8d31af0
Compare
kevinjqliu
left a comment
kevinjqliu
left a comment
There was a problem hiding this comment.
LGTM, added 1 nit comment about function reuse.
Could you also add the rest spec reference to the PR description? https://github.com/apache/iceberg/blob/09140e52836048b112c42c9cfe721295bd57048b/open-api/rest-catalog-open-api.yaml#L2981-L3004
pyiceberg/table/statistics.py
Outdated
| return [stat for stat in statistics if stat.snapshot_id != reject_snapshot_id] | ||
|
|
||
|
|
||
| def filter_partition_statistics_by_snapshot_id( |
There was a problem hiding this comment.
nit: wdyt about combing this with the function above?
There was a problem hiding this comment.
I gave this a try and couldn't get it to work without very strange looking type errors / wonky annotations (this could also be my Python-foo failing me). I'm going to keep it as-is if that's alright.
There was a problem hiding this comment.
FWIW don't think we've released #2146 yet so can maybe rename StatisticsCommonFields to BaseStatisticsFile but unsure.
There was a problem hiding this comment.
Then or otherwise I'd have thought S = TypeVar("S", bound=BaseStatisticsFile) (or StatisticsCommonFields without rename) may work? But not tested it 😄
|
@kevinjqliu please merge whenever you're ready! |
kevinjqliu
left a comment
kevinjqliu
left a comment
There was a problem hiding this comment.
LGTM! I managed to get type hint working with Union. The | operator only works for python 3.10+ and we still supports 3.9 currently
|
Thanks for the PR @rambleraptor and @smaheshwar-pltr for the review! :) |
…pache#2192) <!-- 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#2191 # Rationale for this change apache/iceberg supports these two event types. We should do the same to match the Java implementation and allow users to alter their partition statistics files. [REST Spec Reference]([apache/iceberg@09140e5/open-api/rest-catalog-open-api.yaml#L2981-L3004](https://github.com/apache/iceberg/blob/09140e52836048b112c42c9cfe721295bd57048b/open-api/rest-catalog-open-api.yaml#L2981-L3004)) # Are these changes tested? Unit test included. # Are there any user-facing changes? - Adds `RemovePartitionStatisticsUpdate` and `SetPartitionStatisticsUpdate` actions. <!-- In the case of user-facing changes, please add the changelog label. --> --------- Co-authored-by: Kevin Liu <kevin.jq.liu@gmail.com>
3 participants
Closes #2191
Rationale for this change
apache/iceberg supports these two event types. We should do the same to match the Java implementation and allow users to alter their partition statistics files.
REST Spec Reference
Are these changes tested?
Unit test included.
Are there any user-facing changes?
RemovePartitionStatisticsUpdateandSetPartitionStatisticsUpdateactions.