roryqi commented on code in PR #10671:
URL: https://github.com/apache/gravitino/pull/10671#discussion_r3295846938
##########
iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/rest/IcebergNamespaceOperations.java:
##########
@@ -114,7 +116,8 @@ public Response listNamespaces(
IcebergRequestContext context =
new IcebergRequestContext(httpServletRequest(), catalogName);
ListNamespacesResponse response =
- namespaceOperationDispatcher.listNamespaces(context,
parentNamespace);
+ namespaceOperationDispatcher.listNamespaces(
+ context, parentNamespace, pageToken, pageSize);
Review Comment:
**Pagination happens before authorization filtering.**
`filterListNamespacesResponse` runs below (after this call), so when
authorization is enabled the response is sliced to `pageSize` *first* and then
filtered, meaning the caller can get fewer than `pageSize` items (possibly
zero) while authorized items still exist at later offsets — and `nextPageToken`
here encodes a pre-filter offset, so the page-walk is inconsistent with what
the client sees. The same pattern applies to the table and view list endpoints.
Consider paginating after filtering, or document the limitation. Either way, a
test covering pagination + authz would be valuable.
##########
iceberg/iceberg-common/src/main/java/org/apache/gravitino/iceberg/common/ops/IcebergCatalogWrapper.java:
##########
@@ -451,6 +471,61 @@ public IcebergTableChange(TableIdentifier tableIdentifier,
Transaction transacti
}
}
+ private int parsePageToken(@Nullable String pageToken) {
+ if (pageToken == null || pageToken.isEmpty()) {
+ return 0;
+ }
+ try {
+ int offset = Integer.parseInt(pageToken);
+ Preconditions.checkArgument(offset >= 0, "pageToken must be
non-negative, got: %s", offset);
+ return offset;
+ } catch (NumberFormatException e) {
+ throw new IllegalArgumentException("Invalid pageToken: " + pageToken, e);
+ }
+ }
+
+ private ListNamespacesResponse paginateNamespaces(
+ ListNamespacesResponse response, @Nullable String pageToken, @Nullable
Integer pageSize) {
+ if (pageSize == null && (pageToken == null || pageToken.isEmpty())) {
+ return response;
+ }
+ List<Namespace> all = response.namespaces();
Review Comment:
`response` here already came from `listNamespace(parent)`, which
materializes the **entire** namespace list from the underlying catalog. Since
this happens on every page request, server-side memory is unchanged and paging
through N items is O(N²) total work. That's a reasonable first step, but the
PR's motivation implies true pagination — worth documenting this as a known
limitation.
Also note offset-based tokens assume a stable, deterministic ordering across
calls; concurrent create/drop between pages can skip or duplicate items. And
minor: the `offset >= all.size()` early-return (just below) runs before
`checkArgument(limit > 0)`, so an invalid `pageSize` combined with a large
offset silently returns empty instead of erroring.
--
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]
