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 alter columns for names and nullability #1903
Conversation
72ea448
to
09f58fb
Compare
09f58fb
to
a3d0795
Compare
a3d0795
to
fa9fc76
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.
Looks good!
python/src/dataset.rs
Outdated
let alterations = alterations | ||
.iter() | ||
.map(|obj| { | ||
let obj = PyAny::downcast::<PyDict>(obj)?; |
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.
Can you not just say obj.downcast::<PyDict>()
?
rust/lance/src/dataset.rs
Outdated
/// If a column has an index, it's index will be preserved or transformed to | ||
/// the new type as part of the 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.
Index transformation is only future right? Once we support data type changes?
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.
Yeah I should remove that for now.
- "name": str, optional | ||
The new name of the column. If not specified, the column name is | ||
not changed. | ||
- "nullable": bool, optional | ||
Whether the column should be nullable. If not specified, the column | ||
nullability is not changed. Only non-nullable columns can be changed | ||
to nullable. Currently, you cannot change a nullable column to | ||
non-nullable. |
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.
If the column is already nullable, or already has the desired name, is it a no-op or a failure? From the code it looks like a no-op correct?
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.
Yes, it should be a no-op.
Initial pass on
alter_columns()
API. This allows renaming columns and making them nullable. A future PR will allow casting the type of column.