Add table statistics by ndrluis · Pull Request #1285 · apache/iceberg-python

ndrluis · 2024-11-04T01:52:14Z

The Java expire snapshot process expires table statistics and partition statistics. I am implementing a statistics table to make our expire snapshot compatible with the Java implementation.

ndrluis · 2024-11-04T02:10:45Z

I plan to move the set/remove statistics methods from the Transaction class to another class, such as ManageSnapshot. In the meantime, I’d like to confirm with everyone if I’m heading in the right direction with the current implementation.

@Fokko @sungwy @kevinjqliu

kevinjqliu

Thanks for the PR! Added a few comments. I think it would also be helpful to include integration tests

pyiceberg/table/metadata.py

pyiceberg/table/statistics.py

pyiceberg/table/__init__.py

pyiceberg/table/update/__init__.py

tests/conftest.py

ndrluis · 2024-11-10T23:44:04Z

@kevinjqliu could you please review it once more?

kevinjqliu

Added a few comments.

Do you know which engine currently can generate puffin files? would be great to add an integration with a spark generated puffin file

mkdocs/docs/api.md

pyiceberg/table/statistics.py

ndrluis · 2024-11-12T12:41:50Z

Do you know which engine currently can generate puffin files? would be great to add an integration with a spark generated puffin file

@kevinjqliu As far as I know, only Trino can generate them. What kind of test would you like to have? I believe we are covering all relevant cases for this PR. If PyIceberg could generate or read puffin files, then I agree it would be useful to add tests to check compatibility between engines. However, I think it only makes sense to test puffin files during reading, as testing generation would mean verifying the implementation of something that isn’t our responsibility. In this case, it’s just a metadata update.

What do you think?

kevinjqliu

LGTM! Thanks for working on this!

Regarding the integration tests, since we're manipulating table metadata to add/remove table stats, it would be great to verify that another source can interact with these stats. Not a hard blocker

kevinjqliu · 2025-01-08T04:08:05Z

@ndrluis do you mind resolving the merge conflict here?

ndrluis · 2025-01-08T21:08:16Z

@kevinjqliu Done!

Fokko

Hey @ndrluis Thanks for working on this, and sorry for leaving this hanging for so long. I have some small comments, but it looks good to me 👍

dev/provision.py

mkdocs/docs/api.md

pyiceberg/table/statistics.py

Fokko · 2025-01-15T15:45:21Z

pyiceberg/table/update/__init__.py

+    if update.snapshot_id != update.statistics.snapshot_id:
+        raise ValueError("Snapshot id in statistics does not match the snapshot id in the update")


It's a bit of an awkward check, but something that we have to live with I guess.

Co-authored-by: Fokko Driesprong <fokko@apache.org>

Fokko

Looking good, thanks @ndrluis 🙌

ndrluis force-pushed the add-statistics branch from 6f0bee0 to a70edb2 Compare November 4, 2024 01:54

ndrluis changed the title ~~Add table statistics update~~ Add table statistics Nov 4, 2024

ndrluis force-pushed the add-statistics branch from a70edb2 to 384e229 Compare November 4, 2024 02:00

ndrluis changed the title ~~Add table statistics~~ WIP: Add table statistics Nov 4, 2024

ndrluis requested review from Fokko, kevinjqliu and sungwy November 4, 2024 02:21

kevinjqliu reviewed Nov 4, 2024

View reviewed changes

pyiceberg/table/metadata.py Show resolved Hide resolved

pyiceberg/table/statistics.py Show resolved Hide resolved

pyiceberg/table/__init__.py Outdated Show resolved Hide resolved

pyiceberg/table/update/__init__.py Outdated Show resolved Hide resolved

tests/conftest.py Show resolved Hide resolved

ndrluis force-pushed the add-statistics branch 2 times, most recently from 9b15c86 to d16ef47 Compare November 10, 2024 23:30

ndrluis requested a review from kevinjqliu November 10, 2024 23:42

ndrluis changed the title ~~WIP: Add table statistics~~ Add table statistics Nov 10, 2024

ndrluis marked this pull request as ready for review November 10, 2024 23:43

kevinjqliu reviewed Nov 12, 2024

View reviewed changes

mkdocs/docs/api.md Show resolved Hide resolved

mkdocs/docs/api.md Outdated Show resolved Hide resolved

pyiceberg/table/statistics.py Outdated Show resolved Hide resolved

ndrluis force-pushed the add-statistics branch from d16ef47 to 11120bf Compare November 12, 2024 12:47

ndrluis requested a review from kevinjqliu November 12, 2024 12:49

kevinjqliu approved these changes Nov 12, 2024

View reviewed changes

ndrluis mentioned this pull request Nov 24, 2024

Delete orphan files #1200

Open

kevinjqliu added this to the PyIceberg 0.9.0 release milestone Jan 8, 2025

ndrluis force-pushed the add-statistics branch from 11120bf to adfc971 Compare January 8, 2025 20:50

kevinjqliu mentioned this pull request Jan 10, 2025

table.update_schema() continues to commit when exist with an exception #1505

Closed

3 tasks

Fokko reviewed Jan 15, 2025

View reviewed changes

ndrluis force-pushed the add-statistics branch 3 times, most recently from 2034836 to ba64764 Compare January 15, 2025 21:19

ndrluis force-pushed the add-statistics branch 2 times, most recently from 8fe9992 to 217bb95 Compare January 15, 2025 21:43

ndrluis and others added 5 commits January 16, 2025 10:13

Add table statistics update

b6d7509

Update pyiceberg/table/statistics.py

011f647

Co-authored-by: Fokko Driesprong <fokko@apache.org>

Update mkdocs/docs/api.md

0f7190b

Co-authored-by: Fokko Driesprong <fokko@apache.org>

Update mkdocs/docs/api.md

0ad4aa5

Co-authored-by: Fokko Driesprong <fokko@apache.org>

Add Literal import

e328524

ndrluis force-pushed the add-statistics branch from 217bb95 to 45c60cb Compare January 16, 2025 13:13

Rewrite tests

fb9b2a2

ndrluis force-pushed the add-statistics branch from 45c60cb to fb9b2a2 Compare January 16, 2025 13:48

Fokko approved these changes Jan 16, 2025

View reviewed changes

Fokko merged commit 0a3a886 into apache:main Jan 16, 2025
8 checks passed

ndrluis deleted the add-statistics branch January 16, 2025 14:14

kevinjqliu mentioned this pull request Jan 21, 2025

Deprecate snapshot-id of SetStatisticsUpdate #1556

Closed

ndrluis mentioned this pull request Jan 22, 2025

Support Snapshot Expiration Operation #516

Closed

		if update.snapshot_id != update.statistics.snapshot_id:
		raise ValueError("Snapshot id in statistics does not match the snapshot id in the update")

Conversation

ndrluis commented Nov 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ndrluis commented Nov 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kevinjqliu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ndrluis commented Nov 10, 2024

Uh oh!

kevinjqliu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ndrluis commented Nov 12, 2024

Uh oh!

kevinjqliu left a comment

Choose a reason for hiding this comment

Uh oh!

kevinjqliu commented Jan 8, 2025

Uh oh!

ndrluis commented Jan 8, 2025

Uh oh!

Fokko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Fokko Jan 15, 2025

Choose a reason for hiding this comment

Uh oh!

Fokko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Comments

ndrluis commented Nov 4, 2024 •

edited

Loading

ndrluis commented Nov 4, 2024 •

edited

Loading