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(rust): advance state in post commit #2396

Merged
merged 3 commits into from
Apr 27, 2024

Conversation

ion-elgreco
Copy link
Collaborator

Description

We advance the state in the post commit now, so it's done in a single location as per suggestion from @Blajda here: #2391 (comment)

This PR also supersedes this one: #2280

Related Issue(s)

@Blajda
Copy link
Collaborator

Blajda commented Apr 12, 2024

@ion-elgreco I've looked a this quick a bit and feel like a deep refactor is required for this. IMO if the commit logic is going to be responsible for providing the committed then it must return return a DeltaTableState / Snapshot. What are the expected scenarios where that struct will be None?
I also think it will be worth having the commit code take ownership of the passed Snapshot so a clone can be avoided.
Let me know what you think.

@ion-elgreco
Copy link
Collaborator Author

@Blajda Yeah it felt a bit strange to not be able to take ownership of the snapshot.

While working on it I didn't understand why the incoming snapshot could be None until I hit the create path, which obviously has no table snapshot.

We should just return always a snapshot. If the incoming snapshot is None, we can just create a snapshot after committing and return that. Do you agree?

@ion-elgreco ion-elgreco marked this pull request as draft April 14, 2024 12:23
@ion-elgreco ion-elgreco force-pushed the feat/advance_state_in_post_commit branch from 52be0c2 to 8024cf8 Compare April 21, 2024 21:28
@ion-elgreco ion-elgreco marked this pull request as ready for review April 21, 2024 21:28
@ion-elgreco ion-elgreco force-pushed the feat/advance_state_in_post_commit branch 2 times, most recently from 7b4950e to be2a388 Compare April 24, 2024 20:10
@ion-elgreco ion-elgreco force-pushed the feat/advance_state_in_post_commit branch from be2a388 to 4544772 Compare April 26, 2024 12:40
@ion-elgreco ion-elgreco force-pushed the feat/advance_state_in_post_commit branch from 4544772 to 32a62c2 Compare April 26, 2024 21:08
Copy link
Collaborator

@Blajda Blajda left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for making the changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
binding/rust Issues for the Rust crate
Projects
None yet
3 participants