feat: drop column for schema evolution#1582
Conversation
dentiny
force-pushed
the
hjiang/delete-column-implementation
branch
from
832e327 to
cbdb100
Compare
dentiny
force-pushed
the
hjiang/delete-column-implementation
branch
from
cbdb100 to
477519e
Compare
| /// # Returns | ||
| /// | ||
| /// Returns the `UpdateSchema` with the delete operation staged. | ||
| pub fn delete_column(&mut self, column_name: Vec<String>) -> Result<&mut Self> { |
There was a problem hiding this comment.
One thing I'm hesitant about is, is it better to provide a transaction action SetSchemaAction.
liurenjie1024
left a comment
liurenjie1024
left a comment
There was a problem hiding this comment.
Thanks @dentiny for adding this pr! The implementation is incorrect, please take other actions as reference. In general, all actions methods are used as builder method to store changes, and actual validation and change happens in commit method.
| /// # Returns | ||
| /// | ||
| /// Returns the `UpdateSchema` with the delete operation staged. | ||
| pub fn delete_column(&mut self, column_name: Vec<String>) -> Result<&mut Self> { |
There was a problem hiding this comment.
| pub fn delete_column(&mut self, column_name: Vec<String>) -> Result<&mut Self> { | |
| pub fn drop_column(&mut self, column_name: Vec<String>) -> Result<&mut Self> { |
There was a problem hiding this comment.
I think the column_name to be String would be enough.
There was a problem hiding this comment.
The implementation is incorrect. All changes should happen in the commit method.
There was a problem hiding this comment.
Gotcha, updated. Thank you!
dentiny
changed the title
dentiny
force-pushed
the
hjiang/delete-column-implementation
branch
from
72bb406 to
57531f6
Compare
| /// Current schema before update. | ||
| schema: SchemaRef, |
There was a problem hiding this comment.
This is incorrect, we should not store this, it should be fetched from table's current schema when applying.
|
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions. |
|
This pull request has been closed due to lack of activity. This is not a judgement on the merit of the PR in any way. It is just a way of keeping the PR queue manageable. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time. |
2 participants
What changes are included in this PR?
I want to take over @jonathanc-n 's previous work on schema evolution: already contacted him offline.
Previous PR for reference: #1172
In this PR, I implemented the column deletion logic, which is the simplest part; and integrate with the new transaction API.
Are these changes tested?
Yes, unit tests added.