-
Notifications
You must be signed in to change notification settings - Fork 532
feat: add alter column nullable to non-nullable support #5589
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
Conversation
Code ReviewP1: Bug - Nested column validation will failIn When you call Suggestion: Consider using the batch's first (and only) column directly since the projection contains exactly one column: let col = batch.column(0);Or alternatively, extract the leaf field name from the path: let leaf_name = path.rsplit('.').next().unwrap_or(path);
let col = batch.column_by_name(leaf_name)...Recommendation: Add a test for nested columnsThe current tests only cover top-level columns. Given that Otherwise the implementation looks correct - the validation approach of scanning all data to check for nulls before allowing the schema change is sound. |
|
+1 on the review comment. Would like to have this supported in nested columns. |
wjones127
left a comment
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.
Add a test for nested columns for this feature.
wjones127
left a comment
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!
| // TODO: in the future, we could check the values of the column to see if | ||
| // they are all non-null and thus the column could be made 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.
Might want to remove this TODO before merging though, as it looks to be complete.
42ea514 to
b3f4d7a
Compare
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
This PR will add alter column nullable to non-nullable support
Parts of this PR were drafted with assistance from Codex (with
gpt-5.2) and fully reviewed and edited by me. I take full responsibility for all changes.