-
Notifications
You must be signed in to change notification settings - Fork 287
Rest: Implement register table #1521
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 |
---|---|---|
|
@@ -41,7 +41,7 @@ use crate::client::{ | |
use crate::types::{ | ||
CatalogConfig, CommitTableRequest, CommitTableResponse, CreateTableRequest, | ||
ListNamespaceResponse, ListTableResponse, LoadTableResponse, NamespaceSerde, | ||
RenameTableRequest, | ||
RegisterTableRequest, RenameTableRequest, | ||
}; | ||
|
||
const ICEBERG_REST_SPEC_VERSION: &str = "0.14.1"; | ||
|
@@ -100,6 +100,10 @@ impl RestCatalogConfig { | |
self.url_prefixed(&["tables", "rename"]) | ||
} | ||
|
||
fn register_table_endpoint(&self, ns: &NamespaceIdent) -> String { | ||
self.url_prefixed(&["namespaces", &ns.to_url_string(), "register"]) | ||
} | ||
|
||
fn table_endpoint(&self, table: &TableIdent) -> String { | ||
self.url_prefixed(&[ | ||
"namespaces", | ||
|
@@ -237,7 +241,7 @@ struct RestContext { | |
pub struct RestCatalog { | ||
/// User config is stored as-is and never be changed. | ||
/// | ||
/// It's could be different from the config fetched from the server and used at runtime. | ||
/// It could be different from the config fetched from the server and used at runtime. | ||
user_config: RestCatalogConfig, | ||
ctx: OnceCell<RestContext>, | ||
} | ||
|
@@ -742,13 +746,61 @@ impl Catalog for RestCatalog { | |
|
||
async fn register_table( | ||
&self, | ||
_table_ident: &TableIdent, | ||
_metadata_location: String, | ||
table_ident: &TableIdent, | ||
metadata_location: String, | ||
overwrite: Option<bool>, | ||
) -> Result<Table> { | ||
Err(Error::new( | ||
ErrorKind::FeatureUnsupported, | ||
"Registering a table is not supported yet", | ||
)) | ||
let context = self.context().await?; | ||
|
||
let request = context | ||
.client | ||
.request( | ||
Method::POST, | ||
context | ||
.config | ||
.register_table_endpoint(table_ident.namespace()), | ||
) | ||
.json(&RegisterTableRequest { | ||
name: table_ident.name.clone(), | ||
metadata_location: metadata_location.clone(), | ||
overwrite, | ||
}) | ||
.build()?; | ||
|
||
let http_response = context.client.query_catalog(request).await?; | ||
|
||
let response: LoadTableResponse = match http_response.status() { | ||
StatusCode::OK => { | ||
deserialize_catalog_response::<LoadTableResponse>(http_response).await? | ||
} | ||
StatusCode::NOT_FOUND => { | ||
return Err(Error::new( | ||
ErrorKind::NamespaceNotFound, | ||
"The namespace specified does not exist.", | ||
)); | ||
} | ||
StatusCode::CONFLICT => { | ||
return Err(Error::new( | ||
ErrorKind::TableAlreadyExists, | ||
"The given table already exists.", | ||
)); | ||
} | ||
_ => return Err(deserialize_unexpected_catalog_error(http_response).await), | ||
}; | ||
|
||
let metadata_location = response.metadata_location.as_ref().ok_or(Error::new( | ||
ErrorKind::DataInvalid, | ||
"Metadata location missing in `register_table` response!", | ||
))?; | ||
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. nit: this can be slightly simplified using some syntax sugar
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. I think for this we would need to change the signature of |
||
|
||
let file_io = self.load_file_io(Some(metadata_location), None).await?; | ||
|
||
Table::builder() | ||
.identifier(table_ident.clone()) | ||
.file_io(file_io) | ||
.metadata(response.metadata) | ||
.metadata_location(metadata_location.clone()) | ||
.build() | ||
} | ||
|
||
async fn update_table(&self, mut commit: TableCommit) -> Result<Table> { | ||
|
@@ -2457,4 +2509,87 @@ mod tests { | |
update_table_mock.assert_async().await; | ||
load_table_mock.assert_async().await; | ||
} | ||
|
||
#[tokio::test] | ||
async fn test_register_table() { | ||
let mut server = Server::new_async().await; | ||
|
||
let config_mock = create_config_mock(&mut server).await; | ||
|
||
let register_table_mock = server | ||
.mock("POST", "/v1/namespaces/ns1/register") | ||
.with_status(200) | ||
.with_body_from_file(format!( | ||
"{}/testdata/{}", | ||
env!("CARGO_MANIFEST_DIR"), | ||
"load_table_response.json" | ||
)) | ||
.create_async() | ||
.await; | ||
|
||
let catalog = RestCatalog::new(RestCatalogConfig::builder().uri(server.url()).build()); | ||
let table_ident = | ||
TableIdent::new(NamespaceIdent::new("ns1".to_string()), "test1".to_string()); | ||
let metadata_location = String::from( | ||
"s3://warehouse/database/table/metadata/00001-5f2f8166-244c-4eae-ac36-384ecdec81fc.gz.metadata.json", | ||
); | ||
|
||
let table = catalog | ||
.register_table(&table_ident, metadata_location, Some(false)) | ||
.await | ||
.unwrap(); | ||
|
||
assert_eq!( | ||
&TableIdent::from_strs(vec!["ns1", "test1"]).unwrap(), | ||
table.identifier() | ||
); | ||
assert_eq!( | ||
"s3://warehouse/database/table/metadata/00001-5f2f8166-244c-4eae-ac36-384ecdec81fc.gz.metadata.json", | ||
table.metadata_location().unwrap() | ||
); | ||
|
||
config_mock.assert_async().await; | ||
register_table_mock.assert_async().await; | ||
} | ||
|
||
#[tokio::test] | ||
async fn test_register_table_404() { | ||
let mut server = Server::new_async().await; | ||
|
||
let config_mock = create_config_mock(&mut server).await; | ||
|
||
let register_table_mock = server | ||
.mock("POST", "/v1/namespaces/ns1/register") | ||
.with_status(404) | ||
.with_body( | ||
r#" | ||
{ | ||
"error": { | ||
"message": "The namespace specified does not exist", | ||
"type": "NoSuchNamespaceErrorException", | ||
"code": 404 | ||
} | ||
} | ||
"#, | ||
) | ||
.create_async() | ||
.await; | ||
|
||
let catalog = RestCatalog::new(RestCatalogConfig::builder().uri(server.url()).build()); | ||
|
||
let table_ident = | ||
TableIdent::new(NamespaceIdent::new("ns1".to_string()), "test1".to_string()); | ||
let metadata_location = String::from( | ||
"s3://warehouse/database/table/metadata/00001-5f2f8166-244c-4eae-ac36-384ecdec81fc.gz.metadata.json", | ||
); | ||
let table = catalog | ||
.register_table(&table_ident, metadata_location, None) | ||
.await; | ||
|
||
assert!(table.is_err()); | ||
assert!(table.err().unwrap().message().contains("does not exist")); | ||
|
||
config_mock.assert_async().await; | ||
register_table_mock.assert_async().await; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -280,7 +280,11 @@ impl Catalog for MemoryCatalog { | |
&self, | ||
table_ident: &TableIdent, | ||
metadata_location: String, | ||
overwrite: Option<bool>, | ||
) -> Result<Table> { | ||
// TODO: Use overwrite in `insert_new_table` to overwrite metadata for an already existing table | ||
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. Thinking this could be a 'good first issue'? |
||
let _overwrite = overwrite.unwrap_or(false); | ||
|
||
let mut root_namespace_state = self.root_namespace_state.lock().await; | ||
root_namespace_state.insert_new_table(&table_ident.clone(), metadata_location.clone())?; | ||
|
||
|
@@ -1737,7 +1741,7 @@ mod tests { | |
let register_table_ident = | ||
TableIdent::new(namespace_ident.clone(), "register_table".into()); | ||
let registered_table = catalog | ||
.register_table(®ister_table_ident, metadata_location.clone()) | ||
.register_table(®ister_table_ident, metadata_location.clone(), None) | ||
.await | ||
.unwrap(); | ||
|
||
|
Uh oh!
There was an error while loading. Please reload this page.