HIVE-29878: Support pagination for list endpoints in HMS REST Catalog and remove redundant type casting in REST Catalog - #6750
Aggarwal-Raghav wants to merge 4 commits into
Conversation
2b9d4af to
26cf388
Compare
26cf388 to
69ab1e2
Compare
|
CC @deniskuzZ , can you help with review? |
|
gentle ping for review @deniskuzZ , have handled your review comments. |
| Class<T> responseType, | ||
| Supplier<Object> unpaginatedTask, | ||
| BiFunction<String, String, Object> paginatedTask) { | ||
| String pageToken = PropertyUtil.propertyAsString(properties, PAGE_TOKEN, null); |
There was a problem hiding this comment.
Only listNamespaces, listTables, and listViews and listFunctions supports pagination based on iceberg spec.
6c443e8 to
89d0354
Compare
84f21ed to
8874536
Compare
|
This is ready from my side! |
| namespace = Namespace.empty(); | ||
| } | ||
| return castResponse(ListNamespacesResponse.class, CatalogHandlers.listNamespaces(asNamespaceCatalog, namespace)); | ||
| return paginateIfRequested( |
There was a problem hiding this comment.
could we simplify it further?
/**
* Paging parameters of a list request. Without pageSize the listing is unpaged, expressed as a
* single unbounded first page. This relies on CatalogHandlers.paginate treating a null token as
* the first page and returning a null next-page-token once the list is exhausted.
*/
private record PageRequest(String token, String size) {
private static final PageRequest UNPAGED =
new PageRequest(null, String.valueOf(Integer.MAX_VALUE));
static PageRequest from(Map<String, String> vars) {
String size = vars.get(PAGE_SIZE);
if (size == null) {
return UNPAGED; // token ignored, as upstream; keeping it would overflow token + MAX_VALUE
}
Preconditions.checkArgument(NumberUtils.toInt(size, 0) > 0,
"Invalid %s: %s, must be a positive integer", PAGE_SIZE, size);
return new PageRequest(vars.get(PAGE_TOKEN), size);
}
}
private ListNamespacesResponse listNamespaces(Map<String, String> vars) {
Namespace parent = vars.containsKey(PARENT)
? RESTUtil.namespaceFromQueryParam(vars.get(PARENT))
: Namespace.empty();
PageRequest page = PageRequest.from(vars);
return CatalogHandlers.listNamespaces(asNamespaceCatalog, parent, page.token(), page.size());
}
private ListTablesResponse listTables(Map<String, String> vars) {
Namespace namespace = namespaceFromPathVars(vars);
PageRequest page = PageRequest.from(vars);
return CatalogHandlers.listTables(catalog, namespace, page.token(), page.size());
}
private ListTablesResponse listViews(Map<String, String> vars) {
Namespace namespace = namespaceFromPathVars(vars);
PageRequest page = PageRequest.from(vars);
return CatalogHandlers.listViews(asViewCatalog, namespace, page.token(), page.size());
}
There was a problem hiding this comment.
Done. Thanks for your repeated guidance on this
8874536 to
b841b98
Compare
| Assertions.assertNotNull(res1, "Response should not be null"); | ||
| Assertions.assertEquals(5, res1.namespaces().size(), "Should return all 5 unpaginated"); | ||
|
|
||
| // 2. Both pageToken and pageSize (should call paginated and slice without errors) |
There was a problem hiding this comment.
as in prev PRs, please split into individual test-cases
deniskuzZ
left a comment
There was a problem hiding this comment.
+1, but please address the test refactor comment
…atalog - Removed redundant castResponse method and BadResponseType exception - Replaced deprecated NAMESPACE_SPLITTER with RESTUtil.namespaceFromQueryParam - Extracted 'parent' string literal into PARENT constant
b841b98 to
bca0fda
Compare
|
bca0fda to
de216bf
Compare
|
@deniskuzZ |



What changes were proposed in this pull request?
This PR updates HMSCatalogAdapter to correctly extract pageToken and pageSize query parameters for the
listNamespaces,listTables, andlistViewsREST routes.Additionally, this PR performs several architectural cleanups to modernize the adapter:
castResponsemethod, leveraging Java generics.NAMESPACE_SPLITTERanddecodeNamespace(String)with modern Iceberg 1.11.0 equivalents (namespaceFromQueryParamanddecodeNamespace(..., "%1F")).Why are the changes needed?
Iceberg Rest Catalog Spec supports it as follows and this helps in catalog contains large number of tables.
Does this PR introduce any user-facing change?
NO
How was this patch tested?
Added
TestHMSCatalogAdapterPaginationJUnit test