-
Notifications
You must be signed in to change notification settings - Fork 294
feat(catalog): Implement register_table for glue catalog #1568
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
base: main
Are you sure you want to change the base?
Conversation
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.
Thanks for the work Yiyang ! Left some comments
async fn register_table( | ||
&self, | ||
_table_ident: &TableIdent, | ||
_metadata_location: String, | ||
table: &TableIdent, |
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.
table: &TableIdent, | |
table_ident: &TableIdent, |
"Table {}.{} created but failed to load: {e}", | ||
db_name, table_name | ||
), | ||
) |
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.
We should use .with_source(e)
to expose the cause
} else { | ||
Err(from_aws_sdk_error(err)) | ||
} | ||
} |
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.
Hmm it seems that iceberg-rs doesn't handle aws error very explicitly, the existing way of exposing error is just attaching AWS SDK error as a source and return ErrorKind::Unexpected
: https://github.com/apache/iceberg-rust/blob/2bc03c28268f15472353b828602bb5efd3bf9513/crates/catalog/glue/src/error.rs#L24-L23
ideally we want to handle every error listed here for CreateTable
: https://docs.rs/aws-sdk-glue/latest/aws_sdk_glue/operation/create_table/enum.CreateTableError.html
the error handling in this PR looks good to me, and we should update the existing error handling in create_table
. this should be completed with a follow-up
|
||
match result { | ||
Ok(_) => { | ||
self.load_table(table).await.map_err(|e| { |
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.
Why not just use Table::builder()....build()
so we don't need to load the table here?
ref:
Table::builder() |
@@ -624,15 +624,75 @@ impl Catalog for GlueCatalog { | |||
} | |||
} | |||
|
|||
/// Asynchronously registers an existing table into the Glue Catalog. |
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.
Why adding Asynchronously
? Do you mean the async function? If so, we typically don't add this word.
Which issue does this PR close?
What changes are included in this PR?
implemented the register_table method in the Glue catalog.
added an integration test to cover table registration via metadata location.
Are these changes tested?
Yes
I'm new to Rust and to Iceberg, any feedback or suggestions for improvement are very welcome! 😊