Skip to content

feat(python): combine load_version/load_with_datetime into load_as_version#1968

Merged
roeap merged 12 commits intodelta-io:mainfrom
ion-elgreco:feat/consolidate_load_apis
Dec 19, 2023
Merged

feat(python): combine load_version/load_with_datetime into load_as_version#1968
roeap merged 12 commits intodelta-io:mainfrom
ion-elgreco:feat/consolidate_load_apis

Conversation

@ion-elgreco
Copy link
Copy Markdown
Collaborator

@ion-elgreco ion-elgreco commented Dec 14, 2023

@github-actions github-actions bot added the binding/python Issues for the Python package label Dec 14, 2023
Copy link
Copy Markdown
Collaborator

@roeap roeap 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 this refactor.

somehow I am not a fan of the name load_to, but also don't have a better one ready.

Comment thread python/deltalake/table.py
@roeap
Copy link
Copy Markdown
Collaborator

roeap commented Dec 14, 2023

maybe an opportunity to fix #1967 as well?

@ion-elgreco
Copy link
Copy Markdown
Collaborator Author

ion-elgreco commented Dec 14, 2023

thanks for this refactor.

somehow I am not a fan of the name load_to, but also don't have a better one ready.

@roeap maybe we can just name it load or load_state instead?

@ion-elgreco ion-elgreco requested a review from rtyler as a code owner December 14, 2023 10:35
@github-actions github-actions bot added binding/rust Issues for the Rust crate crate/core labels Dec 14, 2023
@ion-elgreco ion-elgreco requested a review from roeap December 14, 2023 10:38
@roeap
Copy link
Copy Markdown
Collaborator

roeap commented Dec 14, 2023

I try to always see if it works as a short sentence ... "load [table] to version" just somehow hit my ear wrong 😆.

Would "load [table] as version" work? load alone may be confusion as well, as this may be misconsrued as the table still needs to be loaded?

@ion-elgreco
Copy link
Copy Markdown
Collaborator Author

@roeap Yeah when you read it like that it doesn't sound right haha. Hmm, that would translate into load_as_version or load_asof?

@roeap
Copy link
Copy Markdown
Collaborator

roeap commented Dec 14, 2023

Not sure myself, but what would you think about at ...

table.at(version)

@ion-elgreco
Copy link
Copy Markdown
Collaborator Author

@roeap I think at is to unclear. Would the previously mentioned load_state not be more clear? Since it's doing that?

@ion-elgreco
Copy link
Copy Markdown
Collaborator Author

ion-elgreco commented Dec 14, 2023

maybe an opportunity to fix #1967 as well?

@roeap fixing this, causes now one of the restore test in Rusts to fail, not sure what to do here because all python tests pass fine. Maybe the timing of the rust written table is to close to each other?

@ion-elgreco ion-elgreco changed the title feat(python): combine load_version/load_with_datetime into load_to feat(python): combine load_version/load_with_datetime into load_as_version Dec 14, 2023
@ion-elgreco ion-elgreco enabled auto-merge (squash) December 14, 2023 17:41
Copy link
Copy Markdown
Collaborator

@roeap roeap left a comment

Choose a reason for hiding this comment

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

👍

@roeap roeap disabled auto-merge December 19, 2023 19:52
@roeap roeap enabled auto-merge (squash) December 19, 2023 19:52
@roeap roeap merged commit a5a4e69 into delta-io:main Dec 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

binding/python Issues for the Python package binding/rust Issues for the Rust crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

.load_with_datetime() is incorrectly rounding to nearest second Rethink the load_version and load_with_datetime interfaces

2 participants