Skip to content

Conversation

@jonathanc-n
Copy link
Contributor

Which issue does this PR close?

Part of #1102

What changes are included in this PR?

Added partition stats outline

Are these changes tested?

@jonathanc-n
Copy link
Contributor Author

@Xuanwo @Fokko @sdd @liurenjie1024 This should be ready for review

Ok(())
}

fn update_snapshot_info(&mut self, snapshot_id: i64, updated_at: i64) {
Copy link
Member

Choose a reason for hiding this comment

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

If we only use update_snapshot_info internally, is it better to have seperate APIs like update_with_data_file and refresh_with_snapshot?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe its safer to just have update_snapshot_info kept like this because if users are going to update the statistics for the latest snapshot this should always be called alongside it. I think it is quite dangerous to not update to latest snapshot when updating because it is incorrect if the user forgets to call the API to refresh. It will also be used for other partition stat updates as seen in the java implementation here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants