feat(catalog): Implement update_table for GlueCatalog #1584
Conversation
| @@ -367,3 +368,69 @@ async fn test_list_namespace() -> Result<()> { | |||
|
|
|||
| Ok(()) | |||
| } | |||
|
|
|||
| #[tokio::test] | |||
| async fn test_update_table() -> Result<()> { | |||
There was a problem hiding this comment.
should there be tests for non-happy paths?
There was a problem hiding this comment.
I think this is a good point, but I didn't test conflicts, because currently it's tricky to forge conflicts that are non-retryable without more complicated transaction actions like overwrite or rewrite
There was a problem hiding this comment.
there isn't a way to mock the response?
crates/catalog/glue/src/catalog.rs
Outdated
| .with_retryable(true), | ||
| _ => Error::new( | ||
| ErrorKind::Unexpected, | ||
| "Operation failed for hitting aws sdk error", |
There was a problem hiding this comment.
include table name here for completeness?
crates/catalog/glue/src/catalog.rs
Outdated
| .with_retryable(true), | ||
| _ => Error::new( | ||
| ErrorKind::Unexpected, | ||
| "Operation failed for hitting aws sdk error", |
There was a problem hiding this comment.
| "Operation failed for hitting aws sdk error", | |
| "Operation failed for hitting AWS SDK error", |
nit
There was a problem hiding this comment.
I think the current convention is to use lower case for aws sdk-related messaged across glue and s3table's implementations, and this should be fixed in a separate PR if needed
| "Operation failed for hitting aws sdk error", | ||
| ), | ||
| } | ||
| .with_source(anyhow!("aws sdk error: {:?}", error)) |
There was a problem hiding this comment.
| .with_source(anyhow!("aws sdk error: {:?}", error)) | |
| .with_source(anyhow!("AWS SDK error: {:?}", error)) |
nit
## Which issue does this PR close? - Addresses the Glue part of apache#1389 ## What changes are included in this PR? - Implemented `update_table` for `GlueCatalog` - Added test for `GlueCatalog::update_table` - Fixed exception type in `RestCatalog` - Fixed a typo in `ErrorKind` ## Are these changes tested? added a test
Which issue does this PR close?
What changes are included in this PR?
update_tableforGlueCatalogGlueCatalog::update_tableRestCatalogErrorKindAre these changes tested?
added a test