CTTY commented on code in PR #2920:
URL: https://github.com/apache/iceberg-rust/pull/2920#discussion_r3780509428
##########
crates/catalog/rest/src/catalog.rs:
##########
@@ -4120,4 +4363,115 @@ mod tests {
assert_eq!(err.message(), "Catalog uri is required");
}
}
+
+ #[tokio::test]
+ async fn test_create_session_catalog() {
+ let builder = RestSessionCatalogBuilder::default();
+
+ let result = builder
+ .load(
+ "test",
+ HashMap::from([
+ (
+ REST_CATALOG_PROP_URI.to_string(),
+ "http://localhost:8080".to_string(),
+ ),
+ ("a".to_string(), "b".to_string()),
+ ]),
+ )
+ .await;
+
+ assert!(result.is_ok());
+
+ let catalog = result.unwrap();
+
+ let catalog_config = catalog.user_config;
+ assert_eq!(catalog_config.name.as_deref(), Some("test"));
+ assert_eq!(catalog_config.uri, "http://localhost:8080";);
+ assert_eq!(catalog_config.warehouse, None);
+ // The default builder sets no client (only `with_client` does).
+ assert!(catalog_config.client.is_none());
+
+ // `uri` is consumed into its own field; other props are retained.
+ assert_eq!(catalog_config.props.get("a"), Some(&"b".to_string()));
+ assert!(!catalog_config.props.contains_key(REST_CATALOG_PROP_URI));
+ }
+
+ #[tokio::test]
+ async fn test_create_rest_catalog_with_session() {
+ let context = SessionContext::builder()
+ .session_id("test-id".to_string())
+ .build();
+
+ let result = RestCatalogBuilder::default()
+ .with_session_context(context)
+ .load(
+ "test",
+ HashMap::from([(
+ REST_CATALOG_PROP_URI.to_string(),
+ "http://localhost:8080".to_string(),
+ )]),
+ )
+ .await;
+
+ assert!(result.is_ok());
+
+ // The context passed to `with_session_context` is the one the catalog
is bound to.
+ let catalog = result.unwrap();
+ assert_eq!(catalog.session_context.session_id(), "test-id");
+ }
+
+ #[tokio::test]
+ async fn test_create_rest_catalog_default_session() {
+ let result = RestCatalogBuilder::default()
+ .load(
+ "test",
+ HashMap::from([(
+ REST_CATALOG_PROP_URI.to_string(),
+ "http://localhost:8080".to_string(),
+ )]),
+ )
+ .await;
+
+ assert!(result.is_ok());
+
+ // Without `with_session`, the catalog falls back to
`SessionContext::empty()`,
Review Comment:
```suggestion
// Without `with_session_context`, the catalog falls back to
`SessionContext::empty()`,
```
##########
crates/catalog/rest/src/catalog.rs:
##########
@@ -1083,7 +1219,11 @@ impl Catalog for RestCatalog {
/// If there are any config properties that are present in both the
response from the REST
/// server and the config provided when creating this `RestCatalog`
instance, then the value
Review Comment:
The doc needs update here
##########
crates/catalog/rest/src/catalog.rs:
##########
@@ -419,6 +419,51 @@ struct RestClient {
}
impl RestClient {
+ async fn init(
+ user_config: &RestCatalogConfig,
+ auth_manager: Arc<dyn AuthManager>,
+ ) -> Result<Self> {
+ let http_client = HttpClient::new(user_config)?;
+ // The init session lives only for the config handshake, so a
+ // manager whose session guards a one-shot resource can release
+ // it before deriving the catalog session.
+ let catalog_config = {
+ let init_session = auth_manager
+ .init_session(
+ &http_client.without_auth_session(),
+ &RestSessionCatalog::auth_props(user_config),
+ )
+ .await?;
+ RestSessionCatalog::load_config(
Review Comment:
makes sense to me, let's do it!
##########
crates/catalog/rest/src/catalog.rs:
##########
@@ -998,6 +1133,7 @@ impl Catalog for RestCatalog {
/// the value provided locally to the `RestCatalog` will take precedence.
Review Comment:
The doc here needs update
##########
crates/catalog/rest/src/catalog.rs:
##########
@@ -890,13 +1012,17 @@ impl Catalog for RestCatalog {
}
}
- async fn namespace_exists(&self, ns: &NamespaceIdent) -> Result<bool> {
+ async fn namespace_exists(
+ &self,
+ context: &SessionContext,
+ ns: &NamespaceIdent,
+ ) -> Result<bool> {
// Prefer a cheap HEAD when the server advertises it; otherwise fall
back
// to loading the namespace (GET) and treating a missing namespace as
// `false`, so this still works against servers that don't advertise
the
// HEAD route.
if !self.supports_endpoint(&V1_NAMESPACE_EXISTS).await? {
Review Comment:
There are some documentation need to be updated since we are moving
functions into RestSessionCatalog:
https://github.com/apache/iceberg-rust/blob/main/crates/catalog/rest/src/endpoint.rs#L27
--
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]
