singhpk234 commented on code in PR #273:
URL: https://github.com/apache/polaris/pull/273#discussion_r2021563710
##########
extension/persistence/eclipselink/src/main/java/org/apache/polaris/extension/persistence/impl/eclipselink/PolarisEclipseLinkStore.java:
##########
@@ -278,21 +281,40 @@ long countActiveChildEntities(
}
List<ModelEntity> lookupFullEntitiesActive(
- EntityManager session, long catalogId, long parentId, @Nonnull
PolarisEntityType entityType) {
+ EntityManager session,
+ long catalogId,
+ long parentId,
+ @Nonnull PolarisEntityType entityType,
+ PageToken pageToken) {
Review Comment:
should this be also marked as `@NonNull`
##########
polaris-core/src/main/java/org/apache/polaris/core/config/FeatureConfiguration.java:
##########
@@ -183,4 +183,12 @@ protected FeatureConfiguration(
"How many times to retry refreshing metadata when the previous
error was retryable")
.defaultValue(2)
.buildFeatureConfiguration();
+
+ public static final PolarisConfiguration<Boolean> LIST_PAGINATION_ENABLED =
Review Comment:
can this be configured at client level ?
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/PolarisMetaStoreManager.java:
##########
@@ -296,11 +298,12 @@ EntityResult loadEntity(
*
* @param callCtx call context
* @param executorId executor id
- * @param limit limit
+ * @param pageToken page token to start after
* @return list of tasks to be completed
*/
@Nonnull
- EntitiesResult loadTasks(@Nonnull PolarisCallContext callCtx, String
executorId, int limit);
+ EntitiesResult loadTasks(
+ @Nonnull PolarisCallContext callCtx, String executorId, PageToken
pageToken);
Review Comment:
[orthogonal] Are we ok in changing this interface ?
cc @dennishuo
##########
polaris-core/src/main/java/org/apache/polaris/core/config/FeatureConfiguration.java:
##########
@@ -183,4 +183,12 @@ protected FeatureConfiguration(
"How many times to retry refreshing metadata when the previous
error was retryable")
.defaultValue(2)
.buildFeatureConfiguration();
+
+ public static final PolarisConfiguration<Boolean> LIST_PAGINATION_ENABLED =
+ PolarisConfiguration.<Boolean>builder()
+ .key("LIST_PAGINATION_ENABLED")
+ .catalogConfig("list-pagination.enabled")
+ .description("If set to true, pagination for APIs like listTables is
enabled")
Review Comment:
should we be explicit on which of the following APIs we will support
pagination ?
##########
service/common/build.gradle.kts:
##########
@@ -29,6 +29,7 @@ dependencies {
implementation(project(":polaris-api-management-service"))
implementation(project(":polaris-api-iceberg-service"))
implementation(project(":polaris-api-catalog-service"))
+ implementation(project(":polaris-jpa-model"))
Review Comment:
[doubt] why do we need this here ?
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/pagination/ReadEverythingPageToken.java:
##########
@@ -0,0 +1,95 @@
+/*
+ * 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 for readers who want to read everything.
The behavior when
+ * using this token should be the same as when reading without a token.
+ */
+public class ReadEverythingPageToken extends PageToken {
+
+ private ReadEverythingPageToken() {
+ this.pageSize = Integer.MAX_VALUE;
Review Comment:
[doubt] Do we have checks further down in the persistence to not push down
LIMIT when we need INT_MAX ?
##########
quarkus/service/src/test/java/org/apache/polaris/service/quarkus/catalog/GenericTableCatalogTest.java:
##########
@@ -534,7 +534,6 @@ public void testDropIcebergTable() {
Assertions.assertThatCode(
() ->
genericTableCatalog.dropGenericTable(TableIdentifier.of("ns", "t1")))
.hasMessageContaining("Generic table does not exist: ns.t1");
-
Review Comment:
un-intentional ?
##########
extension/persistence/eclipselink/src/main/java/org/apache/polaris/extension/persistence/impl/eclipselink/PolarisEclipseLinkMetaStoreSessionImpl.java:
##########
@@ -441,27 +445,57 @@ 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> PolarisPage<T> listEntitiesInCurrentTxn(
@Nonnull PolarisCallContext callCtx,
long catalogId,
long parentId,
@Nonnull PolarisEntityType entityType,
- int limit,
@Nonnull Predicate<PolarisBaseEntity> entityFilter,
- @Nonnull Function<PolarisBaseEntity, T> transformer) {
- // 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());
+ @Nonnull Function<PolarisBaseEntity, T> transformer,
+ @Nonnull PageToken pageToken) {
+ List<T> data;
+ if (entityFilter.equals(Predicates.alwaysTrue())) {
+ // In this case, we can push the filter down into the query
+ data =
+ this.store
+ .lookupFullEntitiesActive(
+ localSession.get(), catalogId, parentId, entityType,
pageToken)
+ .stream()
+ .map(ModelEntity::toEntity)
+ .filter(entityFilter)
+ .map(transformer)
+ .collect(Collectors.toList());
+ } else {
+ // In this case, we cannot push the filter down into the query. We must
therefore remove
+ // the page size limit from the PageToken and filter on the client side.
+ // TODO Implement a generic predicate that can be pushed down into
different metastores
+ PageToken unlimitedPageSizeToken =
pageToken.withPageSize(Integer.MAX_VALUE);
+ List<ModelEntity> rawData =
+ this.store.lookupFullEntitiesActive(
+ localSession.get(), catalogId, parentId, entityType,
unlimitedPageSizeToken);
Review Comment:
Does listing requires now to make this transactionally consistent, consider
for ex: we need to list records, and some record got added while we were
listing ? whats the behaviour expected ?
for ex Snowflake id generator or the ID generator that i am writing just
makes sure entity id is unique, it doesn't guarantee incremental, thoughts ?
--
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]
