-
Notifications
You must be signed in to change notification settings - Fork 415
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(python, rust): add column
operation
#2562
Conversation
add column
operationadd column
operation
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.
I think this is overall a good idea to add 👏
fields: fields.into_iter().collect_vec(), | ||
}; | ||
|
||
metadata.schema_string = serde_json::to_string(&new_table_schema)?; |
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.
Just thinking out loud here, not feedback on this pull request, but implementing TryInto for StructType would probably be handle for us
Hi, it's really a good idea to add such operation. Is there any update on this? |
Unfortunately I am busy for the next 4+ months, after that I'll probably pick the pace up again on delta-rs |
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.
Overall looks okay, I would like the starts_with addressed though
if self.min_writer_version >= 7 { | ||
let mut converted_writer_features = configuration | ||
.iter() | ||
.filter(|(_, value)| { |
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.
Just understanding here, so this unwraps the value of the key, if it's already none, return false. If it's Some, parse it into a bool which you check is both ok and actually true, but then collect the hashmap back into a string?
Why not at this point just keep it bools because you just drop the values below anyways. Or better yet collect::<HashMap<_, _>>>()
?
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.
So I need to first filter the hashmap for enabled configurations, to then try parse each key as a feature
0502ccd
to
ed9b513
Compare
ed9b513
to
acf1e35
Compare
Description
The schema evolution code is quite well abstracted from the writer, so it seemed straight forward to expose this through an
add column
api. This would make it easier for users to add new columns or fields in structs.At some point we can add type widening to the schema evolution as a separate path, which also than could be used to create an
alter column
operation.