Skip to content

Commit d606bc8

Browse files
committed
address pr comments
- removed unnecessary wording in function documentation comments - renamed `table_ident` parameter - replaced `load_table()` with `Table::builder()` - attached source error using `.with_source(e)` - restructured error handling to follow `update_table()` pattern
1 parent a9994d4 commit d606bc8

File tree

2 files changed

+39
-37
lines changed

2 files changed

+39
-37
lines changed

crates/catalog/glue/src/catalog.rs

Lines changed: 31 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ use std::fmt::Debug;
2020

2121
use anyhow::anyhow;
2222
use async_trait::async_trait;
23+
use aws_sdk_glue::operation::create_table::CreateTableError;
2324
use aws_sdk_glue::operation::update_table::UpdateTableError;
2425
use aws_sdk_glue::types::TableInput;
2526
use iceberg::io::{
@@ -700,7 +701,7 @@ impl Catalog for GlueCatalog {
700701
}
701702
}
702703

703-
/// Asynchronously registers an existing table into the Glue Catalog.
704+
/// registers an existing table into the Glue Catalog.
704705
///
705706
/// Converts the provided table identifier and metadata location into a
706707
/// Glue-compatible table representation, and attempts to create the
@@ -713,12 +714,11 @@ impl Catalog for GlueCatalog {
713714
/// an `Err(...)` is returned.
714715
async fn register_table(
715716
&self,
716-
table: &TableIdent,
717+
table_ident: &TableIdent,
717718
metadata_location: String,
718719
) -> Result<Table> {
719-
let db_name = validate_namespace(table.namespace())?;
720-
let table_name = table.name();
721-
720+
let db_name = validate_namespace(table_ident.namespace())?;
721+
let table_name = table_ident.name();
722722
let metadata = TableMetadata::read_from(&self.file_io, &metadata_location).await?;
723723

724724
let table_input = convert_to_glue_table(
@@ -737,38 +737,34 @@ impl Catalog for GlueCatalog {
737737
.table_input(table_input);
738738
let builder = with_catalog_id!(builder, self.config);
739739

740-
let result = builder.send().await;
741-
742-
match result {
743-
Ok(_) => {
744-
self.load_table(table).await.map_err(|e| {
745-
Error::new(
746-
ErrorKind::Unexpected,
747-
format!(
748-
"Table {}.{} created but failed to load: {e}",
749-
db_name, table_name
750-
),
751-
)
752-
})
740+
builder.send().await.map_err(|e| {
741+
let error = e.into_service_error();
742+
match error {
743+
CreateTableError::EntityNotFoundException(_) => Error::new(
744+
ErrorKind::NamespaceNotFound,
745+
format!("Database {} does not exist", db_name),
746+
),
747+
CreateTableError::AlreadyExistsException(_) => Error::new(
748+
ErrorKind::TableAlreadyExists,
749+
format!("Table {}.{} already exists", db_name, table_name),
750+
),
751+
_ => Error::new(
752+
ErrorKind::Unexpected,
753+
format!(
754+
"Failed to register table {}.{} due to AWS SDK error",
755+
db_name, table_name
756+
),
757+
),
753758
}
754-
Err(err) => {
755-
let service_err = err.as_service_error();
759+
.with_source(anyhow!("aws sdk error: {:?}", error))
760+
})?;
756761

757-
if service_err.map(|e| e.is_entity_not_found_exception()) == Some(true) {
758-
Err(Error::new(
759-
ErrorKind::NamespaceNotFound,
760-
format!("Database {} does not exist", db_name),
761-
))
762-
} else if service_err.map(|e| e.is_already_exists_exception()) == Some(true) {
763-
Err(Error::new(
764-
ErrorKind::TableAlreadyExists,
765-
format!("Table {}.{} already exists", db_name, table_name),
766-
))
767-
} else {
768-
Err(from_aws_sdk_error(err))
769-
}
770-
}
771-
}
762+
Ok(Table::builder()
763+
.identifier(table_ident.clone())
764+
.metadata_location(metadata_location)
765+
.metadata(metadata)
766+
.file_io(self.file_io())
767+
.build()?)
772768
}
773769

774770
async fn update_table(&self, commit: TableCommit) -> Result<Table> {

crates/catalog/glue/tests/glue_catalog_test.rs

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -410,7 +410,10 @@ async fn test_register_table() -> Result<()> {
410410
let namespace = NamespaceIdent::new("test_register_table".into());
411411
set_test_namespace(&catalog, &namespace).await?;
412412

413-
let creation = set_table_creation(Some("s3a://warehouse/hive/test_register_table".into()), "my_table")?;
413+
let creation = set_table_creation(
414+
Some("s3a://warehouse/hive/test_register_table".into()),
415+
"my_table",
416+
)?;
414417
let table = catalog.create_table(&namespace, creation).await?;
415418
let metadata_location = table
416419
.metadata_location()
@@ -505,7 +508,10 @@ async fn test_register_table() -> Result<()> {
505508
let namespace = NamespaceIdent::new("test_register_table".into());
506509
set_test_namespace(&catalog, &namespace).await?;
507510

508-
let creation = set_table_creation(Some("s3a://warehouse/hive/test_register_table".into()), "my_table")?;
511+
let creation = set_table_creation(
512+
Some("s3a://warehouse/hive/test_register_table".into()),
513+
"my_table",
514+
)?;
509515
let table = catalog.create_table(&namespace, creation).await?;
510516
let metadata_location = table
511517
.metadata_location()

0 commit comments

Comments
 (0)