mkroll-db commented on code in PR #3295:
URL: https://github.com/apache/iceberg-rust/pull/3295#discussion_r4140788193
##########
crates/catalog/rest/src/catalog.rs:
##########
@@ -1718,6 +1780,276 @@ mod tests {
)
}
+ #[tokio::test]
+ async fn test_page_size_precedence() {
+ // Server default < builder < load properties < server override.
+ for (default, builder, property, override_value, expected) in [
+ (Some("10"), None, None, None, 10),
+ (Some("10"), Some(20), None, None, 20),
+ (None, Some(20), None, None, 20),
+ (Some("10"), None, Some("30"), None, 30),
+ (Some("10"), Some(20), Some("30"), None, 30),
+ (Some("10"), Some(20), Some("30"), Some("40"), 40),
+ (None, None, None, Some("40"), 40),
+ (None, Some(1), None, None, 1),
+ (None, Some(u32::MAX), None, None, u32::MAX),
+ // Invalid lower-priority values must not reject a valid effective
value.
+ (Some("invalid"), Some(20), None, None, 20),
+ (None, Some(0), Some("invalid"), Some("40"), 40),
+ ] {
+ let mut server = Server::new_async().await;
+ let page_size_props = |value: Option<&str>| -> HashMap<String,
String> {
+ value
+ .map(|value| (REST_CATALOG_PROP_PAGE_SIZE.to_string(),
value.to_string()))
+ .into_iter()
+ .collect()
+ };
+ let config_mock = server
+ .mock("GET", "/v1/config")
+ .with_status(200)
+ .with_body(
+ json!({
+ "defaults": page_size_props(default),
+ "overrides": page_size_props(override_value),
+ })
+ .to_string(),
+ )
+ .create_async()
+ .await;
+ let list_mock = server
+ .mock("GET", "/v1/namespaces")
+ .match_query(mockito::Matcher::UrlEncoded(
+ "pageSize".into(),
+ expected.to_string(),
+ ))
+ .with_status(200)
+ .with_body(r#"{"namespaces": []}"#)
+ .expect(1)
+ .create_async()
+ .await;
+
+ let mut builder_config = RestCatalogBuilder::default();
+ if let Some(page_size) = builder {
+ builder_config = builder_config.with_page_size(page_size);
+ }
+ let mut props = page_size_props(property);
+ props.insert(REST_CATALOG_PROP_URI.to_string(), server.url());
+ let catalog = builder_config.load("test", props).await.unwrap();
+
+ catalog.list_namespaces(None).await.unwrap_or_else(|err| {
+ panic!(
+ "default={default:?}, builder={builder:?},
property={property:?}, \
+ override={override_value:?}, expected={expected}: {err}"
+ )
+ });
+ config_mock.assert_async().await;
+ list_mock.assert_async().await;
+ }
+ }
+
+ #[tokio::test]
+ async fn test_page_size_pagination() {
+ let mut server = Server::new_async().await;
+ let config_mock = create_config_mock(&mut server).await;
+ let catalog = RestCatalogBuilder::default()
+ .with_page_size(1)
+ .load(
+ "test",
+ HashMap::from([(REST_CATALOG_PROP_URI.to_string(),
server.url())]),
+ )
+ .await
+ .unwrap();
+
+ let mut mocks = Vec::new();
+ for (endpoint, parent, first_body, empty_body, last_body) in [
+ (
+ "/v1/namespaces",
+ Some("parent\u{1f}child"),
+ json!({"namespaces": [["ns1"]], "next-page-token": "next/+"}),
+ json!({"namespaces": [], "next-page-token": "last"}),
+ json!({"namespaces": [["ns2"]], "next-page-token": null}),
+ ),
+ (
+ "/v1/namespaces/ns1/tables",
+ None,
+ json!({"identifiers": [{"namespace": ["ns1"], "name": "t1"}],
"next-page-token": "next/+"}),
+ json!({"identifiers": [], "next-page-token": "last"}),
+ json!({"identifiers": [{"namespace": ["ns1"], "name": "t2"}]}),
+ ),
+ ] {
+ // Page 1 opts in with an empty token. Later tokens and the
multipart
+ // parent must survive URL encoding, independent of query
parameter order.
+ // The empty middle page must not terminate pagination.
+ for (token, body) in [
+ ("", first_body),
+ ("next/+", empty_body),
+ ("last", last_body),
+ ] {
+ let mut query = vec![
+ mockito::Matcher::UrlEncoded("pageSize".into(),
"1".into()),
+ mockito::Matcher::UrlEncoded("pageToken".into(),
token.into()),
+ ];
+ if let Some(parent) = parent {
+ query.push(mockito::Matcher::UrlEncoded("parent".into(),
parent.into()));
+ }
+ mocks.push(
+ server
+ .mock("GET", endpoint)
+ .match_query(mockito::Matcher::AllOf(query))
+ .with_status(200)
+ .with_body(body.to_string())
+ .expect(1)
+ .create_async()
+ .await,
+ );
+ }
+ }
+
+ let parent = NamespaceIdent::from_vec(vec!["parent".into(),
"child".into()]).unwrap();
+ assert_eq!(catalog.list_namespaces(Some(&parent)).await.unwrap(), vec![
+ NamespaceIdent::new("ns1".into()),
+ NamespaceIdent::new("ns2".into())
+ ]);
+ let namespace = NamespaceIdent::new("ns1".into());
+ assert_eq!(catalog.list_tables(&namespace).await.unwrap(), vec![
+ TableIdent::new(namespace.clone(), "t1".into()),
+ TableIdent::new(namespace, "t2".into())
+ ]);
+ config_mock.assert_async().await;
+ for mock in mocks {
+ mock.assert_async().await;
+ }
+ }
+
+ #[tokio::test]
+ async fn test_page_size_is_not_a_result_limit() {
+ let mut server = Server::new_async().await;
+ let config_mock = create_config_mock(&mut server).await;
+ let catalog = RestCatalogBuilder::default()
+ .with_page_size(1)
+ .load(
+ "test",
+ HashMap::from([(REST_CATALOG_PROP_URI.to_string(),
server.url())]),
+ )
+ .await
+ .unwrap();
+
+ // A server may ignore pageSize and return all results without a next
token.
+ let mut mocks = Vec::new();
+ for (endpoint, body) in [
+ ("/v1/namespaces", json!({"namespaces": [["ns1"], ["ns2"]]})),
+ (
+ "/v1/namespaces/ns1/tables",
+ json!({"identifiers": [
+ {"namespace": ["ns1"], "name": "t1"},
+ {"namespace": ["ns1"], "name": "t2"},
+ ]}),
+ ),
+ ] {
+ mocks.push(
+ server
+ .mock("GET", endpoint)
+ .match_query(mockito::Matcher::AllOf(vec![
+ mockito::Matcher::UrlEncoded("pageSize".into(),
"1".into()),
+ mockito::Matcher::UrlEncoded("pageToken".into(),
"".into()),
+ ]))
+ .with_status(200)
+ .with_body(body.to_string())
+ .expect(1)
+ .create_async()
+ .await,
+ );
+ }
+
+ assert_eq!(catalog.list_namespaces(None).await.unwrap(), vec![
+ NamespaceIdent::new("ns1".into()),
+ NamespaceIdent::new("ns2".into())
+ ]);
+ let namespace = NamespaceIdent::new("ns1".into());
+ assert_eq!(catalog.list_tables(&namespace).await.unwrap(), vec![
+ TableIdent::new(namespace.clone(), "t1".into()),
+ TableIdent::new(namespace, "t2".into())
+ ]);
+ config_mock.assert_async().await;
+ for mock in mocks {
+ mock.assert_async().await;
+ }
+ }
+
+ #[tokio::test]
+ async fn test_unset_page_size() {
Review Comment:
NIT: Alternative name `test_page_size_absent`.
##########
crates/catalog/rest/src/catalog.rs:
##########
@@ -1625,10 +1686,11 @@ impl RestSessionCatalogBuilder {
}
// Collect other remaining properties
- self.config.props = props
- .into_iter()
- .filter(|(k, _)| k != REST_CATALOG_PROP_URI && k !=
REST_CATALOG_PROP_WAREHOUSE)
- .collect();
+ self.config.props.extend(
+ props
+ .into_iter()
+ .filter(|(k, _)| k != REST_CATALOG_PROP_URI && k !=
REST_CATALOG_PROP_WAREHOUSE),
+ );
Review Comment:
NIT: sql has a very [similar
pattern](https://github.com/apache/iceberg-rust/blob/8cb2adeddb4f8da1ca7bd86ca303337f011c12e2/crates/catalog/sql/src/catalog.rs#L198-L205).
Applied to this snippet the code would become:
```suggestion
self.config.props.extend(props);
self.config.props.remove(REST_CATALOG_PROP_URI);
self.config.props.remove(REST_CATALOG_PROP_WAREHOUSE);
```
I think this looks a bit cleaner and also aligns the code.
--
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]
