feat(python): support cleanup_with_policy#5458
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
ddupg
force-pushed
the
feat/py-cleanup-policy
branch
from
f73bd53 to
2f20e78
Compare
majin1102
left a comment
majin1102
left a comment
There was a problem hiding this comment.
Left a comment, PTAL when you have time
ddupg
force-pushed
the
feat/py-cleanup-policy
branch
from
6320c16 to
5447cfe
Compare
wjones127
left a comment
wjones127
left a comment
There was a problem hiding this comment.
Got a question, as this seems like it might be overcomplicating things. There's only one new parameter, right?
| td_to_micros(older_than), delete_unverified, error_if_tagged_old_versions | ||
| ) | ||
|
|
||
| def cleanup_with_policy(self, policy: CleanupPolicy) -> CleanupStats: |
There was a problem hiding this comment.
Why not just add the new retain_versions parameter to cleanup_old_versions, why a whole new method?
There was a problem hiding this comment.
Thank @wjones127 for your attention.
Why not just add the new retain_versions parameter to cleanup_old_versions, why a whole new method?
Just want to align with rust cleanup_with_policy, I can add new parameter retain_versions to cleanup_old_versions, but is it okay if they differ from the Rust method?
Or should we first merge rust cleanup_old_versions and cleanup_with_policy into one?
There was a problem hiding this comment.
I think it’s fine if the API design differs between Python and Rust. Python has named arguments, which is often a natural API design for Python users. Rust doesn’t, and instead needs to use option structs and builder patterns to make configuration readable.
It seems like this should just be another named argument in Python.
There was a problem hiding this comment.
It seems like this should just be another named argument in Python.
Thank @wjones127 for your suggestion. I added retain_versions named argument in python cleanup_old_versions
Change-Id: Ic0170a9d7cbc8842e6ea7a4a5cb3981407883d3f
Change-Id: I2f1abae0a422b5f0b156ef343983777efad5b3d3
ddupg
force-pushed
the
feat/py-cleanup-policy
branch
2 times, most recently
from
166a788 to
f17cb57
Compare
Change-Id: Id4627874a1f2aa85ca2eed9323cbc2d0c8b9fbfe
ddupg
force-pushed
the
feat/py-cleanup-policy
branch
from
f17cb57 to
446cf8c
Compare
| time.sleep(0.05) | ||
| lance.write_dataset(table, base_dir, mode="overwrite") | ||
| moment = datetime.now() | ||
| time.sleep(0.05) |
There was a problem hiding this comment.
Why are the sleep calls necessary?
There was a problem hiding this comment.
This test is to verify the combined effect of older_than and retain_versions, the expected outcome should satisfy both constraints simultaneously. To avoid timing skew affecting the older_than evaluation and causing flakiness, I add sleep calls to stabilize the boundary conditions.
Co-authored-by: Will Jones <willjones127@gmail.com>
Co-authored-by: Will Jones <willjones127@gmail.com>
3 participants
No description provided.