-
Notifications
You must be signed in to change notification settings - Fork 215
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
Conversation
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. |
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.
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?
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} | ||
""" |
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 think function implementations are meant to be put into .pyi
files, which are type stubs.
CompactionMetrics
is implemented in Rust here:
lance/python/src/dataset/optimize.rs
Line 78 in 1b8adb5
pub struct PyCompactionMetrics { |
Could you implement the method there in Rust? Here is an example Rust implementation of a __repr__
method:
lance/python/src/dataset/optimize.rs
Lines 113 to 119 in 1b8adb5
pub fn __repr__(&self) -> PyResult<String> { | |
Ok(format!( | |
"CompactionPlan(read_version={}, tasks=<{} compaction tasks>)", | |
self.0.read_version(), | |
self.num_tasks() | |
)) | |
} |
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.
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.
Hi @avr2002, I'm cleaning up our PRs and it's been a while since any progress is made here. If you end up getting back to this, feel free to re-open the PR. Your contribution would be most welcome. |
Issue Link: #1373