-
Notifications
You must be signed in to change notification settings - Fork 421
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: add convert_to_delta #1686
Conversation
ACTION NEEDED delta-rs 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. |
781ea5b
to
d2e70f9
Compare
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 the pull request! This is a great start and I've left some comments. I can help bring this further if you need it, but also feel free to ping me on Slack if you need any further help preparing this for merge!
d2e70f9
to
b6500d8
Compare
b6500d8
to
df20b56
Compare
df20b56
to
5a4fe14
Compare
I've replied to comments from @rtyler. This MR is ready for review. I also created four follow-up issues.
|
5a3f56e
to
99dbd50
Compare
80d113f
to
09dc423
Compare
Follow-up issue #1718 is done. The API now takes a vector of user-defined
The changes are pushed to this MR. |
e4cae43
to
aa514b0
Compare
@rtyler @wjones127 @roeap This MR is ready for final review. |
@rtyler @wjones127 @roeap Is there anything I can do to help get this merged? |
@rtyler @wjones127 @roeap Just a gentle ping for review. |
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 this great wortk @junjunjd! Left some comments in the code.
More generally speaking, I believe we should model CONVET TO DELTA
as an operation (i.e. config builder / IntoFuture
Trait ...) and also place it in the operations module rather then next to the table. We are slowly but surely working towards also modelling things as logical plans and exposing sql APIs so convert to delta should be compatible whith the other operations we expose.
4e9671e
to
d655f53
Compare
d655f53
to
a8e9413
Compare
@roeap Thanks for the comments. I updated the MR to model CONVET TO DELTA as an operation. This MR is ready for another review. |
4265e4f
to
eb4ae67
Compare
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.
Great work @junjunjd and thanks for sticking with it.
Just some minor nits to look at, but overall I think we are good to go!
9bb866f
to
c32157f
Compare
It seems we do have some compiler errors going on. I would disregard the ones related to a future not being |
c32157f
to
7f3c1d0
Compare
Add a convert_to_delta operation for converting a Parquet table to a Delta Table in place.
7f3c1d0
to
5b2951a
Compare
@roeap Thanks for the review! CI is fixed. MR is ready for final review/merge. |
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.
Looking great @junjunjd, thanks for sticking with it!
Description
Add a convert_to_delta operation for converting a Parquet table to a Delta Table in place.
Related Issue(s)
Documentation