-
Notifications
You must be signed in to change notification settings - Fork 3k
Core: Use scan API to read partition stats #14989
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Core: Use scan API to read partition stats #14989
Conversation
| && Objects.equals(stats1.lastUpdatedSnapshotId(), stats2.lastUpdatedSnapshotId()); | ||
| } | ||
|
|
||
| @SuppressWarnings("checkstyle:CyclomaticComplexity") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is temporarily needed because the tests write stats using PartitionStatistics, however the tests use still use PartitionStats on the read path. When comparing expectations with actual stats we need this function. Will be dropped with the follow-up PR
|
|
||
| private static final int STATS_COUNT = 13; | ||
|
|
||
| public BasePartitionStatistics(StructLike partition, int specId) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this be package private?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| } | ||
|
|
||
| /** Used by internal readers to instantiate this class with a projection schema. */ | ||
| public BasePartitionStatistics(Types.StructType projection) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe even this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| value, | ||
| (existingEntry, newEntry) -> { | ||
| existingEntry.appendStats(newEntry); | ||
| ((BasePartitionStatistics) existingEntry).appendStats(newEntry); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really like this. Why are we sure that this is an instance of BasePartitionStatistics?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactored the code to move the liveEntry(), appendStats() etc. functions into PartitionStatsHandler and populate the stats there using the setter inherited from StructLike. This eliminated the need for the casts.
c2b4a14 to
f9beb4a
Compare
f9beb4a to
584f943
Compare
584f943 to
85124af
Compare
|
Merged to main. |
#14640 introduced a new api to scan partition stats. The next step is to replace the usage of the old reader function to the new stat api. The work is divided into 2 steps:
PartitionStatsHandler.readPartitionStatsFile()in the prod code. See the tests still pass