Implement pagination for list APIs #273
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -52,11 +52,14 @@ | |
| import org.apache.polaris.core.persistence.BaseMetaStoreManager; | ||
| import org.apache.polaris.core.persistence.PrincipalSecretsGenerator; | ||
| import org.apache.polaris.core.persistence.RetryOnConcurrencyException; | ||
| import org.apache.polaris.core.persistence.pagination.PageToken; | ||
| import org.apache.polaris.core.persistence.pagination.PolarisPage; | ||
| import org.apache.polaris.core.persistence.transactional.AbstractTransactionalPersistence; | ||
| import org.apache.polaris.core.policy.PolarisPolicyMappingRecord; | ||
| import org.apache.polaris.core.storage.PolarisStorageConfigurationInfo; | ||
| import org.apache.polaris.core.storage.PolarisStorageIntegration; | ||
| import org.apache.polaris.core.storage.PolarisStorageIntegrationProvider; | ||
| import org.apache.polaris.jpa.models.EntityIdPageToken; | ||
| import org.apache.polaris.jpa.models.ModelEntity; | ||
| import org.apache.polaris.jpa.models.ModelEntityActive; | ||
| import org.apache.polaris.jpa.models.ModelEntityChangeTracking; | ||
|
|
@@ -419,29 +422,30 @@ public List<EntityNameLookupRecord> lookupEntityActiveBatchInCurrentTxn( | |
|
|
||
| /** {@inheritDoc} */ | ||
| @Override | ||
| public @Nonnull PolarisPage<EntityNameLookupRecord> listEntitiesInCurrentTxn( | ||
| @Nonnull PolarisCallContext callCtx, | ||
| long catalogId, | ||
| long parentId, | ||
| @Nonnull PolarisEntityType entityType, | ||
| @Nonnull PageToken pageToken) { | ||
| return this.listEntitiesInCurrentTxn( | ||
| callCtx, catalogId, parentId, entityType, Predicates.alwaysTrue(), pageToken); | ||
| } | ||
|
|
||
| @Override | ||
| public @Nonnull PolarisPage<EntityNameLookupRecord> listEntitiesInCurrentTxn( | ||
| @Nonnull PolarisCallContext callCtx, | ||
| long catalogId, | ||
| long parentId, | ||
| @Nonnull PolarisEntityType entityType, | ||
| @Nonnull Predicate<PolarisBaseEntity> entityFilter, | ||
| @Nonnull PageToken pageToken) { | ||
| // full range scan under the parent for that type | ||
| return this.listEntitiesInCurrentTxn( | ||
| callCtx, | ||
| catalogId, | ||
| parentId, | ||
| entityType, | ||
| entityFilter, | ||
| entity -> | ||
| new EntityNameLookupRecord( | ||
|
|
@@ -450,27 +454,57 @@ public List<EntityNameLookupRecord> lookupEntityActiveBatchInCurrentTxn( | |
| entity.getParentId(), | ||
| entity.getName(), | ||
| entity.getTypeCode(), | ||
| entity.getSubTypeCode()), | ||
| pageToken); | ||
| } | ||
|
|
||
| @Override | ||
| public @Nonnull <T> PolarisPage<T> listEntitiesInCurrentTxn( | ||
| @Nonnull PolarisCallContext callCtx, | ||
| long catalogId, | ||
| long parentId, | ||
| @Nonnull PolarisEntityType entityType, | ||
| @Nonnull Predicate<PolarisBaseEntity> entityFilter, | ||
| @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); | ||
|
Comment on lines
+487
to
+489
Contributor
There was a problem hiding this comment. [doubt] Let say we want to 10 records and table had 10k records and we have filter currently since this is a client side filter. And client requested page size of 2 we will get 5 time 10 K records. I was seeing similar issue had 2 options.
I am not sure which is lesser poison pill to swallow.
Contributor
Author
There was a problem hiding this comment. Luckily, we do not use this filter very often at all... but yes, this situation is less than ideal. If the metastore itself provided a lazily-fetched There is actually a third option as well, which is that we just return more results than the client asked for. This is what we currently do actually, since we don't respect the page token.
Contributor
Author
There was a problem hiding this comment. Looking into this a little more, EclipseLink does support edit: 😔
Contributor
There was a problem hiding this 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 which NoSQL impl generates or the ID generator that i am writing just makes sure entity id is unique, it doesn't guarantee incremental, am I missing something ?
Contributor
Author
There was a problem hiding this comment. I don't think this needs to be transactionally consistent. You can miss an entity, but you will never get the same entity twice. Entity ID is guaranteed to always be unique. I suppose there is a case where you transactionally add some table and drop some other table that was already returned in an existing page -- the list results in aggregate will be inconsistent with that transaction. But I think this is outside the realm of what we need to support for the time being. I would probably not want to fail either the list operation or the table update operation to ensure consistency when clients who care about transactional consistency have the option to list without a page token.
Member
There was a problem hiding this comment. I disagree that an inconsistency (duplicate and omitted) results is acceptable.
Member
There was a problem hiding this comment. I also have a strong concern that there's more data processed here than absolutely necessary.
Contributor
Author
There was a problem hiding this comment. There are no duplicate or omitted results here if we define omitted as "not present in the results of a list when the entity was in fact present through the duration of the list". Results are only omitted when they are deleted in the middle of a list operation; that is, the entity is gone by the time we reach the page that should contain it. |
||
| if (pageToken.pageSize < Integer.MAX_VALUE && rawData.size() > pageToken.pageSize) { | ||
| LOGGER.info( | ||
| "A page token could not be respected due to a predicate. " | ||
| + "{} records were read but the client was asked to return {}.", | ||
| rawData.size(), | ||
| pageToken.pageSize); | ||
| } | ||
|
|
||
| data = | ||
| rawData.stream() | ||
| .map(ModelEntity::toEntity) | ||
| .filter(entityFilter) | ||
| .limit(pageToken.pageSize) | ||
| .map(transformer) | ||
| .collect(Collectors.toList()); | ||
| } | ||
|
|
||
| return pageToken.buildNextPage(data); | ||
| } | ||
|
|
||
| /** {@inheritDoc} */ | ||
|
|
@@ -762,4 +796,9 @@ public void rollback() { | |
| session.getTransaction().rollback(); | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
| public @Nonnull PageToken.PageTokenBuilder<?> pageTokenBuilder() { | ||
| return EntityIdPageToken.builder(); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,114 @@ | ||
| /* | ||
| * 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.jpa.models; | ||
|
|
||
| import java.util.List; | ||
| import org.apache.polaris.core.entity.PolarisBaseEntity; | ||
| import org.apache.polaris.core.persistence.pagination.PageToken; | ||
|
|
||
| /** | ||
| * A {@link PageToken} implementation that tracks the greatest ID from either {@link | ||
| * PolarisBaseEntity} or {@link ModelEntity} objects supplied in updates. Entities are meant to be | ||
| * filtered during listing such that only entities with and ID greater than the ID of the token are | ||
|
Contributor
There was a problem hiding this comment. Do we assume that entities are loaded in the order of their IDs?
Contributor
Author
There was a problem hiding this comment. Metastores that use this
Contributor
Author
There was a problem hiding this comment. If we want to add some new method to the metastore to canonicalize this check I think we can. The current check is similar to some other checks we have throughout the codebase that enforce a certain entity type is passed in.
Contributor
There was a problem hiding this comment. Oops, I missed that this page token was specific to JPA 🤦
Contributor
Author
There was a problem hiding this comment. Yeah, it's a little brittle though. Related to your comment above about the varying semantics of
Member
There was a problem hiding this comment. Noting: the order of values returned by database sequences is not necessarily strictly increasing. Aka: it is wrong to assume that each retrieved values from a database sequence is higher than any value retrieved before.
Contributor
Author
There was a problem hiding this comment. It's not an assumption; it's something the metastore has to enforce in order for this page token to be a valid implementation to use. |
||
| * returned. | ||
| */ | ||
| public class EntityIdPageToken extends PageToken { | ||
|
Contributor
There was a problem hiding this comment. The JPA-model module should only be used by EclipseLink. We need to either found a common place to put this class, or duplicate it in different persistence implmentations. |
||
| public long id; | ||
|
|
||
| private EntityIdPageToken(long id, int pageSize) { | ||
| this.id = id; | ||
| this.pageSize = pageSize; | ||
| validate(); | ||
| } | ||
|
|
||
| /** The minimum ID that could be attached to an entity */ | ||
| private static final long MINIMUM_ID = 0; | ||
|
|
||
| /** The entity ID to use to start with. */ | ||
| private static final long BASE_ID = MINIMUM_ID - 1; | ||
|
|
||
| @Override | ||
| protected List<String> getComponents() { | ||
| return List.of(String.valueOf(id), String.valueOf(pageSize)); | ||
| } | ||
|
|
||
| /** Get a new `EntityIdPageTokenBuilder` instance */ | ||
| public static PageTokenBuilder<EntityIdPageToken> builder() { | ||
| return new EntityIdPageToken.EntityIdPageTokenBuilder(); | ||
| } | ||
|
|
||
| @Override | ||
| protected PageTokenBuilder<?> getBuilder() { | ||
| return EntityIdPageToken.builder(); | ||
| } | ||
|
|
||
| /** A {@link PageTokenBuilder} implementation for {@link EntityIdPageToken} */ | ||
| public static class EntityIdPageTokenBuilder extends PageTokenBuilder<EntityIdPageToken> { | ||
|
|
||
| @Override | ||
| public String tokenPrefix() { | ||
| return "polaris-entity-id"; | ||
| } | ||
|
|
||
| @Override | ||
| public int expectedComponents() { | ||
| // id, pageSize | ||
| return 2; | ||
| } | ||
|
|
||
| @Override | ||
| protected EntityIdPageToken fromStringComponents(List<String> components) { | ||
| return new EntityIdPageToken( | ||
| Integer.parseInt(components.get(0)), Integer.parseInt(components.get(1))); | ||
| } | ||
|
|
||
| @Override | ||
| protected EntityIdPageToken fromLimitImpl(int limit) { | ||
| return new EntityIdPageToken(BASE_ID, limit); | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
| public PageToken updated(List<?> newData) { | ||
| if (newData == null || newData.size() < this.pageSize) { | ||
| return DONE; | ||
| } else { | ||
| var head = newData.getFirst(); | ||
| if (head instanceof ModelEntity) { | ||
| return new EntityIdPageToken(((ModelEntity) newData.getLast()).getId(), this.pageSize); | ||
| } else if (head instanceof PolarisBaseEntity) { | ||
| // Assumed to be sorted with the greatest entity ID last | ||
| return new EntityIdPageToken( | ||
| ((PolarisBaseEntity) newData.getLast()).getId(), this.pageSize); | ||
| } else { | ||
| throw new IllegalArgumentException( | ||
| "Cannot build a page token from: " + newData.getFirst().getClass().getSimpleName()); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
| public PageToken withPageSize(Integer pageSize) { | ||
| if (pageSize == null) { | ||
| return new EntityIdPageToken(BASE_ID, this.pageSize); | ||
| } else { | ||
| return new EntityIdPageToken(this.id, pageSize); | ||
| } | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does
Predicate.alwaysTruereturn something that can be compared and will that hold true for future?Predicates are rather functions/lambdas - those are not meant to be compared with anything.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's right. This is only meant to capture the predicate that's provided in the method overload that doesn't have a predicate, e.g. here. It is not meant to capture any predicate that's semantically "always true".