dimas-b commented on code in PR #1528: URL: https://github.com/apache/polaris/pull/1528#discussion_r2079934994
########## polaris-core/src/main/java/org/apache/polaris/core/persistence/pagination/PageToken.java: ########## @@ -0,0 +1,99 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.polaris.core.persistence.pagination; + +import java.util.List; + +/** + * Represents a page token that can be used by operations like `listTables`. Clients that specify a + * `pageSize` (or a `pageToken`) may receive a `next-page-token` in the response, the content of + * which is a serialized PageToken. + * + * <p>By providing that in the next query's `pageToken`, the client can resume listing where they + * left off. If the client provides a `pageToken` or `pageSize` but `next-page-token` is null in the + * response, that means there is no more data to read. + */ +public abstract class PageToken { + + /** Build a new PageToken that reads everything */ + public static PageToken readEverything() { + return build(null, null); + } + + /** Build a new PageToken from an input String, without a specified page size */ + public static PageToken fromString(String token) { + return build(token, null); + } + + /** Build a new PageToken from a limit */ + public static PageToken fromLimit(Integer pageSize) { + return build(null, pageSize); + } + + /** Build a {@link PageToken} from the input string and page size */ + public static PageToken build(String token, Integer pageSize) { + if (token == null || token.isEmpty()) { + if (pageSize != null) { + return new ReadFromStartPageToken(pageSize); + } else { + return new ReadEverythingPageToken(); + } + } else { + // TODO implement, split out by the token's prefix + return new ReadEverythingPageToken(); + } + } + + /** Serialize a {@link PageToken} into a string */ + @Override + public abstract String toString(); Review Comment: Maybe nitpicky, but I'd prefer a method like `toStringExternal()` for clarity. `toString()` may return the same value, of course, but I think it is usually used "too lightly" so a dedicated serialization method should add some safety. ########## polaris-core/src/main/java/org/apache/polaris/core/persistence/pagination/DonePageToken.java: ########## @@ -0,0 +1,36 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.polaris.core.persistence.pagination; + +import java.util.List; + +public class DonePageToken extends PageToken { + + public DonePageToken() {} + + @Override + public String toString() { + return null; + } + + @Override + protected PageToken updated(List<?> newData) { + return new DonePageToken(); Review Comment: It is an error for a "done" token to have a next page, right? Should we throw an exception here? ########## polaris-core/src/main/java/org/apache/polaris/core/persistence/pagination/ReadFromStartPageToken.java: ########## @@ -0,0 +1,51 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.polaris.core.persistence.pagination; + +import java.util.List; + +/** + * A {@link PageToken} implementation that has a page size, but no start offset. This can be used to + * represent a `limit`. When updated, it returns a token with the same semantics. + */ +public class ReadFromStartPageToken extends PageToken implements HasPageSize { Review Comment: nit: rename to `InitialPageToken`... not sure the name is readable :sweat_smile: but I'm trying to avoid confusion with a token to "Read From Start Page", which is what I tent to read it as :sweat_smile: ########## service/common/build.gradle.kts: ########## @@ -104,6 +104,7 @@ dependencies { testFixturesImplementation(project(":polaris-api-management-model")) testFixturesImplementation(project(":polaris-api-management-service")) testFixturesImplementation(project(":polaris-api-iceberg-service")) + testFixturesImplementation(project(":polaris-jpa-model")) Review Comment: Is this still needed? why? ########## polaris-core/src/main/java/org/apache/polaris/core/persistence/pagination/PageToken.java: ########## @@ -0,0 +1,99 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.polaris.core.persistence.pagination; + +import java.util.List; + +/** + * Represents a page token that can be used by operations like `listTables`. Clients that specify a + * `pageSize` (or a `pageToken`) may receive a `next-page-token` in the response, the content of + * which is a serialized PageToken. + * + * <p>By providing that in the next query's `pageToken`, the client can resume listing where they + * left off. If the client provides a `pageToken` or `pageSize` but `next-page-token` is null in the + * response, that means there is no more data to read. + */ +public abstract class PageToken { + + /** Build a new PageToken that reads everything */ + public static PageToken readEverything() { + return build(null, null); + } + + /** Build a new PageToken from an input String, without a specified page size */ + public static PageToken fromString(String token) { + return build(token, null); + } + + /** Build a new PageToken from a limit */ + public static PageToken fromLimit(Integer pageSize) { + return build(null, pageSize); + } + + /** Build a {@link PageToken} from the input string and page size */ + public static PageToken build(String token, Integer pageSize) { + if (token == null || token.isEmpty()) { + if (pageSize != null) { + return new ReadFromStartPageToken(pageSize); + } else { + return new ReadEverythingPageToken(); + } + } else { + // TODO implement, split out by the token's prefix + return new ReadEverythingPageToken(); Review Comment: Maybe throw an `IllegalArgumentException` for now? ########## service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java: ########## @@ -169,6 +170,23 @@ public static boolean isCreate(UpdateTableRequest request) { return isCreate; } + public ListNamespacesResponse listNamespaces( + Namespace parent, String pageToken, Integer pageSize) { + PolarisAuthorizableOperation op = PolarisAuthorizableOperation.LIST_NAMESPACES; + authorizeBasicNamespaceOperationOrThrow(op, parent); + + if (baseCatalog instanceof IcebergCatalog polarisCatalog) { + Page<Namespace> results = polarisCatalog.listNamespaces(parent, pageToken, pageSize); + return ListNamespacesResponse.builder() + .addAll(results.items) + .nextPageToken(results.pageToken.toString()) + .build(); + } else { + return CatalogHandlers.listNamespaces( + namespaceCatalog, parent, pageToken, String.valueOf(pageSize)); Review Comment: I believe we can do better in Polaris by using the REST API of `baseCatalog` directly. This can be deferred. ########## polaris-core/src/main/java/org/apache/polaris/core/persistence/pagination/ReadFromStartPageToken.java: ########## @@ -0,0 +1,51 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.polaris.core.persistence.pagination; + +import java.util.List; + +/** + * A {@link PageToken} implementation that has a page size, but no start offset. This can be used to + * represent a `limit`. When updated, it returns a token with the same semantics. + */ +public class ReadFromStartPageToken extends PageToken implements HasPageSize { + + public static final String PREFIX = "read-from-start"; + + private final int pageSize; + + public ReadFromStartPageToken(int pageSize) { + this.pageSize = pageSize; + } + + @Override + public int getPageSize() { + return pageSize; + } + + @Override + public String toString() { + return String.format("%s/%d", PREFIX, pageSize); + } + + @Override + protected PageToken updated(List<?> newData) { + return new ReadFromStartPageToken(pageSize); Review Comment: This looks incorrect to me. It will likely induce infinite loops in clients that try to do pagination. ########## polaris-core/src/main/java/org/apache/polaris/core/persistence/pagination/DonePageToken.java: ########## @@ -0,0 +1,36 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.polaris.core.persistence.pagination; + +import java.util.List; + +public class DonePageToken extends PageToken { + + public DonePageToken() {} + + @Override + public String toString() { + return null; Review Comment: nit: This is probably a case where we may want to have `toString() == "DONE"`, but `toStringExternal() == null`. ########## extension/persistence/eclipselink/src/main/java/org/apache/polaris/extension/persistence/impl/eclipselink/PolarisEclipseLinkMetaStoreSessionImpl.java: ########## @@ -450,27 +455,33 @@ public List<EntityNameLookupRecord> lookupEntityActiveBatchInCurrentTxn( entity.getParentId(), entity.getName(), entity.getTypeCode(), - entity.getSubTypeCode())); + entity.getSubTypeCode()), + pageToken); } @Override - public @Nonnull <T> List<T> listEntitiesInCurrentTxn( + public @Nonnull <T> Page<T> listEntitiesInCurrentTxn( @Nonnull PolarisCallContext callCtx, long catalogId, long parentId, @Nonnull PolarisEntityType entityType, - int limit, @Nonnull Predicate<PolarisBaseEntity> entityFilter, - @Nonnull Function<PolarisBaseEntity, T> transformer) { + @Nonnull Function<PolarisBaseEntity, T> transformer, + @Nonnull PageToken pageToken) { // full range scan under the parent for that type - return this.store - .lookupFullEntitiesActive(localSession.get(), catalogId, parentId, entityType) - .stream() - .map(ModelEntity::toEntity) - .filter(entityFilter) - .limit(limit) - .map(transformer) - .collect(Collectors.toList()); + Stream<PolarisBaseEntity> data = + this.store + .lookupFullEntitiesActive( + localSession.get(), catalogId, parentId, entityType, pageToken) + .stream() + .map(ModelEntity::toEntity) + .filter(entityFilter); + + if (pageToken instanceof HasPageSize hasPageSize) { Review Comment: I'm not sure this if functionally correct. If the token is a sub-class of `HasPageSize` but has other information that the client provided, but EclipseLink does not recognise, I do not think it would be correct to just use the page size from the token. I believe we should check for a specific type here and fail on all types, whose details cannot be fully processed. -- 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]
