-
Notifications
You must be signed in to change notification settings - Fork 139
fix(catalog/glue): create table with required fields #410
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
zeroshade
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.
@lliangyu-lin can you take a look and confirm if this fixes the issue you filed?
128ae7c to
55c2b45
Compare
|
@zeroshade Thank you for working on this! The implementation looks good to me. But somehow when I test locally with actual AWS Credentials, I'm getting 400 from Glue |
33f0375 to
4c8d78a
Compare
lliangyu-lin
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.
LGTM! Thanks @zeroshade
Fokko
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.
This looks great @zeroshade Thanks!
|
|
||
| icebergFieldIDKey = "iceberg.field.id" | ||
| icebergFieldOptionalKey = "iceberg.field.optional" | ||
| icebergFieldCurrentKey = "iceberg.field.current" |
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 forgot about this one. Will it ever be false? 🤔 The current schema should be stored in Glue.
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.
Interesting: apache/iceberg#3888
|
Thanks for working on this @zeroshade and thanks for the review @lliangyu-lin 🙌 |
fixes #407
Update and improve the Glue catalog CreateTable to properly set the parameters on the table itself and the columns (such as iceberg.field.id and iceberg.field.optional).
Also, write out the metadata first and then pass the location to Glue as part of creating the table.