Skip to content
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

feat: adding __repr__ method to CompactionMetrics #2236

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

avr2002
Copy link

@avr2002 avr2002 commented Apr 21, 2024

Issue Link: #1373

Copy link

ACTION NEEDED

Lance follows the Conventional Commits specification for release automation.

The PR title and description are used as the merge commit message. Please update your PR title and description to match the specification.

For details on the error please inspect the "PR Title Check" action.

@avr2002 avr2002 changed the title adding __repr__ method to CompactionMetrics feat: adding __repr__ method to CompactionMetrics Apr 21, 2024
Copy link
Contributor

@wjones127 wjones127 left a comment

Choose a reason for hiding this comment

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

Thanks for making a PR! In order to make this ready to merge, could you make sure to add a unit test to validate the repr method works?

Comment on lines +36 to +42
def __repr__(self):
return f"""
Fragments Removed: {self.fragments_removed}
Fragments Added: {self.fragments_added}
Files Removed: {self.files_removed}
Files Added: {self.files_added}
"""
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think function implementations are meant to be put into .pyi files, which are type stubs.

CompactionMetrics is implemented in Rust here:

pub struct PyCompactionMetrics {

Could you implement the method there in Rust? Here is an example Rust implementation of a __repr__ method:

pub fn __repr__(&self) -> PyResult<String> {
Ok(format!(
"CompactionPlan(read_version={}, tasks=<{} compaction tasks>)",
self.0.read_version(),
self.num_tasks()
))
}

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for getting back, I really appreciate the support. This is my first time contributing to open-source project. It'll try to implement this and get back.

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.

None yet

2 participants