Skip to content

Core: Refactor and use InternalData for partition stats - #12946

Merged
pvary merged 5 commits into
apache:mainfrom
ajantha-bhat:internaldata
May 8, 2025
Merged

Core: Refactor and use InternalData for partition stats#12946
pvary merged 5 commits into
apache:mainfrom
ajantha-bhat:internaldata

Conversation

@ajantha-bhat

@ajantha-bhatajantha-bhat commented May 1, 2025•
edited
Loading

Copy link
Copy Markdown
Member

a) Deprecate PartitionStatsHandler in iceberg-data
b) Copy PartitionStatsHandler from iceberg-data to iceberg-core and Use InternalData( also package name had to change to use PartitionStatsUtill logic as private code). So, users can add dependency on core and parquet instead of whole data module to use partition stats feature.
c) Deprecate PartitionStatsUtil as no need of too many public interface and we can use that logic as private inside PartitionStatsHandler (comment during #12629)
d) Deprecate TestPartitionStatsUtil as the functionality test is covered from TestPartitionStatsHandler
e) Move benchmarks to iceberg-data as InternalData needs parquet dependency.

@ajantha-bhat
ajantha-bhat requested a review from pvaryMay 1, 2025 13:22
ajantha-bhat
ajantha-bhat commented May 1, 2025
View reviewed changes
Comment threadcore/src/main/java/org/apache/iceberg/PartitionStatsHandler.java
* Computes, writes and reads the {@link PartitionStatisticsFile}. Uses generic readers and writers
* to support writing and reading of the stats in table default format.
*/
public class PartitionStatsHandler {

@ajantha-bhatajantha-bhatMay 1, 2025

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This class was just copied from iceberg-data module (deprecated now) and replace datawriter and reader function with InternalData

ajantha-bhat
ajantha-bhat commented May 1, 2025
View reviewed changes
Comment threadcore/src/main/java/org/apache/iceberg/PartitionStatsHandler.java
return stats;
}

private static Collection<PartitionStats> computeStats(Table table, Snapshot snapshot) {

@ajantha-bhatajantha-bhatMay 1, 2025

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copied the methods from partitionStatsUtil (perviously public, now deprecated) as got a comment that no need to keep too many public interfaces and compute logic can be internal to compute and write.

@ajantha-bhatajantha-bhat mentioned this pull request May 1, 2025
Merged
@deniskuzZ

deniskuzZ commented May 1, 2025

Copy link
Copy Markdown
Member

hi @ajantha-bhat, do you think it would make sense to rename PartitionStatsHandler to PartitionStatsUtil since it's a util class with only static methods?

deniskuzZ
deniskuzZ reviewed May 1, 2025
View reviewed changes
Comment threadcore/src/main/java/org/apache/iceberg/PartitionStatsHandler.java Outdated
deniskuzZ
deniskuzZ reviewed May 1, 2025
View reviewed changes
Comment threadcore/src/main/java/org/apache/iceberg/PartitionStatsHandler.java Outdated
@ajantha-bhat

ajantha-bhat commented May 1, 2025•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

hi @ajantha-bhat, do you think it would make sense to rename PartitionStatsHandler to PartitionStatsUtil since it's a util class with only static methods?

@deniskuzZ: Thanks for the suggestion. Since we already released the public interface as PartitionStatsHandler, I would like to continue this naming. Plus in the talks and blogs we are already using this name.

deniskuzZ
deniskuzZ approved these changes May 1, 2025
View reviewed changes

@deniskuzZdeniskuzZ left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM +1
thanks @pvary for the proposal and @ajantha-bhat for refactoring and merging the partition stats functionality. Looking forward for the increment partition stats compute support.

@ajantha-bhatajantha-bhat mentioned this pull request May 5, 2025
Merged
gaborkaszab
gaborkaszab reviewed May 5, 2025
View reviewed changes
Comment threaddata/src/test/java/org/apache/iceberg/TestPartitionStatsHandler.java Outdated
@ajantha-bhat

ajantha-bhat commented May 6, 2025

Copy link
Copy Markdown
MemberAuthor

@pvary: PR is ready for review.

advancedxy
advancedxy approved these changes May 6, 2025
View reviewed changes

@advancedxyadvancedxy left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM on my side, looking forward to the incremental support.

pvary
pvary reviewed May 6, 2025
View reviewed changes
Comment threadcore/src/test/java/org/apache/iceberg/PartitionStatsHandlerBase.java Outdated
pvary
pvary reviewed May 6, 2025
View reviewed changes
Comment threadcore/src/test/java/org/apache/iceberg/PartitionStatsHandlerBase.java Outdated
pvary
pvary reviewed May 6, 2025
View reviewed changes
Comment threadcore/src/main/java/org/apache/iceberg/PartitionStatsHandler.java
pvary
pvary reviewed May 6, 2025
View reviewed changes
Comment threadcore/src/test/java/org/apache/iceberg/PartitionStatsHandlerTestBase.java Outdated
@ajantha-bhat

ajantha-bhat commented May 7, 2025

Copy link
Copy Markdown
MemberAuthor

Rebased the PR due to spark flaky test.

@ajantha-bhat

ajantha-bhat commented May 8, 2025

Copy link
Copy Markdown
MemberAuthor

@pvary and @gaborkaszab: Anymore comments for this? Thanks for the review.

gaborkaszab
gaborkaszab reviewed May 8, 2025
View reviewed changes

@gaborkaszabgaborkaszab left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the changes @ajantha-bhat !
I left one minor comment, but other than that the PR LGTM!

Comment threadcore/src/test/java/org/apache/iceberg/TestPartitionStatsUtil.java Outdated
@pvary
pvary merged commit bed88a3 into apache:mainMay 8, 2025
@pvary

pvary commented May 8, 2025

Copy link
Copy Markdown
Contributor

Merged to main.
Thanks for the PR @ajantha-bhat and @deniskuzZ, @advancedxy, @gaborkaszab for the reviews!

anuragmantri added a commit to anuragmantri/iceberg that referenced this pull request Jul 25, 2025
@anuragmantri@ajantha-bhatGitHub Enterprise
Core: Refactor and use InternalData for partition stats (apache#12946) (
ed069a2
apache#1581)
Co-authored-by: Ajantha Bhat <ajanthabhat@gmail.com>
devendra-nr pushed a commit to devendra-nr/iceberg that referenced this pull request Dec 8, 2025
@ajantha-bhat@devendra-nr
Core: Refactor and use InternalData for partition stats (apache#12946)
7e56a40
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pvarypvarypvary approved these changes
+3 more reviewers
@advancedxyadvancedxyadvancedxy approved these changes
@deniskuzZdeniskuzZdeniskuzZ approved these changes
@gaborkaszabgaborkaszabgaborkaszab approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

coredataORCparquet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@ajantha-bhat@deniskuzZ@pvary@advancedxy@gaborkaszab