-
Notifications
You must be signed in to change notification settings - Fork 295
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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, | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
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| { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not just use ref:
|
||||||
Error::new( | ||||||
ErrorKind::Unexpected, | ||||||
format!( | ||||||
"Table {}.{} created but failed to load: {e}", | ||||||
db_name, table_name | ||||||
), | ||||||
) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should use |
||||||
}) | ||||||
} | ||||||
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)) | ||||||
} | ||||||
} | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 ideally we want to handle every error listed here for the error handling in this PR looks good to me, and we should update the existing error handling in |
||||||
} | ||||||
} | ||||||
|
||||||
async fn update_table(&self, _commit: TableCommit) -> Result<Table> { | ||||||
|
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.