CTTY commented on code in PR #1568:
URL: https://github.com/apache/iceberg-rust/pull/1568#discussion_r2244092168
##########
crates/catalog/glue/src/catalog.rs:
##########
@@ -624,15 +624,75 @@ impl Catalog for GlueCatalog {
}
}
+ /// Asynchronously registers an existing table into the Glue Catalog.
+ ///
+ /// Converts the provided table identifier and metadata location into a
+ /// Glue-compatible table representation, and attempts to create the
+ /// corresponding table in the Glue Catalog.
+ ///
+ /// # Returns
+ /// Returns `Ok(Table)` if the table is successfully registered and loaded.
+ /// If the registration fails due to validation issues, existing table
conflicts,
+ /// metadata problems, or errors during the registration or loading
process,
+ /// an `Err(...)` is returned.
async fn register_table(
&self,
- _table_ident: &TableIdent,
- _metadata_location: String,
+ table: &TableIdent,
+ metadata_location: String,
) -> Result<Table> {
- Err(Error::new(
- ErrorKind::FeatureUnsupported,
- "Registering a table is not supported yet",
- ))
+ let db_name = validate_namespace(table.namespace())?;
+ let table_name = table.name();
+
+ let metadata = TableMetadata::read_from(&self.file_io,
&metadata_location).await?;
+
+ let table_input = convert_to_glue_table(
+ table_name,
+ metadata_location.clone(),
+ &metadata,
+ metadata.properties(),
+ None,
+ )?;
+
+ let builder = self
+ .client
+ .0
+ .create_table()
+ .database_name(&db_name)
+ .table_input(table_input);
+ let builder = with_catalog_id!(builder, self.config);
+
+ let result = builder.send().await;
+
+ match result {
+ Ok(_) => {
+ self.load_table(table).await.map_err(|e| {
+ Error::new(
+ ErrorKind::Unexpected,
+ format!(
+ "Table {}.{} created but failed to load: {e}",
+ db_name, table_name
+ ),
+ )
+ })
+ }
+ Err(err) => {
+ let service_err = err.as_service_error();
+
+ if service_err.map(|e| e.is_entity_not_found_exception()) ==
Some(true) {
+ Err(Error::new(
+ ErrorKind::NamespaceNotFound,
+ format!("Database {} does not exist", db_name),
+ ))
+ } else if service_err.map(|e| e.is_already_exists_exception())
== Some(true) {
+ Err(Error::new(
+ ErrorKind::TableAlreadyExists,
+ format!("Table {}.{} already exists", db_name,
table_name),
+ ))
+ } else {
+ Err(from_aws_sdk_error(err))
+ }
+ }
Review Comment:
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
