CTTY commented on code in PR #1568:
URL: https://github.com/apache/iceberg-rust/pull/1568#discussion_r2244078648
##########
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,
Review Comment:
```suggestion
table_ident: &TableIdent,
```
##########
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
+ ),
+ )
Review Comment:
We should use `.with_source(e)` to expose the cause
##########
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| {
Review Comment:
Why not just use `Table::builder()....build()` so we don't need to load the
table here?
ref:
https://github.com/apache/iceberg-rust/blob/195fb739da1a01e028a537e8a8b9e4e07ee144fa/crates/catalog/glue/src/catalog.rs#L419
##########
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
I think the 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
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]
