You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@polaris.apache.org by "snazy (via GitHub)" <gi...@apache.org> on 2025/06/25 10:30:07 UTC
[PR] Extensible pagination token implementation [polaris]
snazy opened a new pull request, #1938:
URL: https://github.com/apache/polaris/pull/1938
Based on #1838, following up on #1555
* Allows multiple implementations of `Token` referencing the "next page", encapsulated in `PageToken`. No changes to `polaris-core` needed to add custom `Token` implementations.
* Extensible to (later) support (cryptographic) signatures to prevent tampered page-token
* Refactor pagination code to delineate API-level page tokens and internal "pointers to data"
* Requests deal with the "previous" token, user-provided page size (optional) and the previous request's page size.
* Concentrate the logic of combining page size requests and previous tokens in `PageTokenUtil`
* `PageToken` subclasses are no longer necessary.
* Serialzation of `PageToken` uses Jackson serialization (smile format)
Since no (metastore level) implementation handling pagination existed before, no backwards compatibility is needed.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Extensible pagination token implementation [polaris]
Posted by "eric-maynard (via GitHub)" <gi...@apache.org>.
eric-maynard commented on PR #1938:
URL: https://github.com/apache/polaris/pull/1938#issuecomment-3079483197
This seems to have caused a regression:
```
2025-07-16 10:01:26,449 ERROR [io.qua.ver.htt.run.QuarkusErrorHandler] [,POLARIS] [,,,] (executor-thread-1) HTTP Request to /api/management/v1/catalogs failed, error id: afec5ba7-078c-4917-8d4d-2feaf219fe13-1: java.util.ServiceConfigurationError: com.fasterxml.jackson.databind.Module: Provider com.fasterxml.jackson.module.scala.DefaultScalaModule could not be instantiated
at java.base/java.util.ServiceLoader.fail(ServiceLoader.java:586)
at java.base/java.util.ServiceLoader$ProviderImpl.newInstance(ServiceLoader.java:813)
at java.base/java.util.ServiceLoader$ProviderImpl.get(ServiceLoader.java:729)
at java.base/java.util.ServiceLoader$3.next(ServiceLoader.java:1403)
at com.fasterxml.jackson.databind.ObjectMapper.findModules(ObjectMapper.java:1161)
at com.fasterxml.jackson.databind.ObjectMapper.findModules(ObjectMapper.java:1145)
at com.fasterxml.jackson.databind.ObjectMapper.findAndRegisterModules(ObjectMapper.java:1195)
at org.apache.polaris.core.persistence.pagination.PageTokenUtil.<clinit>(PageTokenUtil.java:46)
at org.apache.polaris.core.persistence.pagination.PageToken.readEverything(PageToken.java:70)
at org.apache.polaris.service.admin.PolarisAdminService.listCatalogsUnsafe(PolarisAdminService.java:964)
```
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Extensible pagination token implementation [polaris]
Posted by "snazy (via GitHub)" <gi...@apache.org>.
snazy commented on PR #1938:
URL: https://github.com/apache/polaris/pull/1938#issuecomment-3076844676
Thanks all!
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Extensible pagination token implementation [polaris]
Posted by "eric-maynard (via GitHub)" <gi...@apache.org>.
eric-maynard commented on code in PR #1938:
URL: https://github.com/apache/polaris/pull/1938#discussion_r2167134566
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/dao/entity/EntitiesResult.java:
##########
@@ -48,41 +43,19 @@ public static EntitiesResult fromPage(Page<PolarisBaseEntity> page) {
public EntitiesResult(@Nonnull ReturnStatus errorStatus, @Nullable String extraInformation) {
super(errorStatus, extraInformation);
this.entities = null;
- this.pageTokenOpt = Optional.empty();
- }
-
- public EntitiesResult(@Nonnull List<PolarisBaseEntity> entities) {
- this(entities, Optional.empty());
}
/**
* Constructor for success
*
* @param entities list of entities being returned, implies success
*/
- public EntitiesResult(
- @Nonnull List<PolarisBaseEntity> entities, @Nonnull Optional<PageToken> pageTokenOpt) {
+ public EntitiesResult(@Nonnull Page<PolarisBaseEntity> entities) {
super(ReturnStatus.SUCCESS);
this.entities = entities;
- this.pageTokenOpt = pageTokenOpt;
- }
-
- @JsonCreator
- private EntitiesResult(
Review Comment:
cc @dennishuo for visibility as well
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Extensible pagination token implementation [polaris]
Posted by "eric-maynard (via GitHub)" <gi...@apache.org>.
eric-maynard commented on code in PR #1938:
URL: https://github.com/apache/polaris/pull/1938#discussion_r2167136065
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/pagination/EntityIdToken.java:
##########
@@ -0,0 +1,66 @@
+/*
+ * 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 com.fasterxml.jackson.annotation.JsonProperty;
+import com.fasterxml.jackson.databind.annotation.JsonDeserialize;
+import com.fasterxml.jackson.databind.annotation.JsonSerialize;
+import jakarta.annotation.Nullable;
+import org.apache.polaris.core.entity.PolarisBaseEntity;
+import org.apache.polaris.immutables.PolarisImmutable;
+
+/** Pagination {@linkplain Token token} backed by {@link PolarisBaseEntity#getId() entity ID}. */
+@PolarisImmutable
+@JsonSerialize(as = ImmutableEntityIdToken.class)
+@JsonDeserialize(as = ImmutableEntityIdToken.class)
+public interface EntityIdToken extends Token {
+ String ID = "e";
+
+ @JsonProperty("i")
Review Comment:
Why would we use the property name `i`?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Extensible pagination token implementation [polaris]
Posted by "eric-maynard (via GitHub)" <gi...@apache.org>.
eric-maynard commented on code in PR #1938:
URL: https://github.com/apache/polaris/pull/1938#discussion_r2167140017
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/pagination/Page.java:
##########
@@ -18,25 +18,99 @@
*/
package org.apache.polaris.core.persistence.pagination;
+import static java.util.Spliterators.iterator;
+
+import jakarta.annotation.Nullable;
+import java.util.ArrayList;
+import java.util.Iterator;
import java.util.List;
+import java.util.function.Function;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
/**
- * An immutable page of items plus their paging cursor. The {@link PageToken} here can be used to
- * continue the listing operation that generated the `items`.
+ * An immutable page of items plus the next-page token value, if there are more items. The {@link
+ * #encodedResponseToken()} here can be used to continue the listing operation that generated the
+ * `items`.
*/
public class Page<T> {
- public final PageToken pageToken;
- public final List<T> items;
+ private final PageToken request;
+ private final List<T> items;
+ @Nullable private final Token nextToken;
- public Page(PageToken pageToken, List<T> items) {
- this.pageToken = pageToken;
+ private Page(PageToken request, @Nullable Token nextToken, List<T> items) {
+ this.request = request;
+ this.nextToken = nextToken;
this.items = items;
}
/**
- * Used to wrap a {@link List<T>} of items into a {@link Page <T>} when there are no more pages
+ * Builds a complete response page for the full list of relevant items. No subsequence pages of
+ * related data exist.
*/
public static <T> Page<T> fromItems(List<T> items) {
- return new Page<>(new DonePageToken(), items);
+ return new Page<>(PageToken.readEverything(), null, items);
+ }
+
+ /**
+ * Produces a response page by consuming the number of items from the provided stream according to
+ * the {@code request} parameter. Source items can be converted to a different type by providing a
+ * {@code mapper} function. The page token for the response will be produced from the request data
+ * combined with the pointer to the next page of data provided by the {@code dataPointer}
+ * function.
+ *
+ * @param request defines pagination parameters that were uses to produce this page of data.
+ * @param items stream of source data
+ * @param mapper converter from source data types to response data types.
+ * @param tokenBuilder determines the {@link Token} used to start the next page of data given the
+ * last item from the previous page. The output of this function will be available from {@link
+ * PageToken#value()} associated with the request for the next page.
+ */
+ public static <R, T> Page<R> mapped(
+ PageToken request, Stream<T> items, Function<T, R> mapper, Function<T, Token> tokenBuilder) {
+ List<R> data;
+ T last = null;
+ if (!request.paginationRequested()) {
+ // short-cut for "no pagination"
+ data = items.map(mapper).collect(Collectors.toList());
+ } else {
+ data = new ArrayList<>(request.pageSize().orElse(10));
+
+ Iterator<T> it = iterator(items.spliterator());
+ int limit = request.pageSize().orElse(Integer.MAX_VALUE);
+ while (it.hasNext() && data.size() < limit) {
+ last = it.next();
+ data.add(mapper.apply(last));
+ }
+
+ // Signal "no more data" if the number of items is less than the requested page size or if
+ // there is no more data.
+ if (data.size() < limit || !it.hasNext()) {
Review Comment:
It should be expected that the iterator has no more items if the limit has been pushed down into whatever provided the `items`, no?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Extensible pagination token implementation [polaris]
Posted by "snazy (via GitHub)" <gi...@apache.org>.
snazy merged PR #1938:
URL: https://github.com/apache/polaris/pull/1938
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Extensible pagination token implementation [polaris]
Posted by "eric-maynard (via GitHub)" <gi...@apache.org>.
eric-maynard commented on code in PR #1938:
URL: https://github.com/apache/polaris/pull/1938#discussion_r2167141327
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/pagination/PageToken.java:
##########
@@ -18,82 +18,75 @@
*/
package org.apache.polaris.core.persistence.pagination;
-import java.util.List;
-import java.util.Objects;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.fasterxml.jackson.databind.annotation.JsonDeserialize;
+import com.fasterxml.jackson.databind.annotation.JsonSerialize;
+import jakarta.annotation.Nullable;
+import java.util.Optional;
+import java.util.OptionalInt;
+import org.apache.polaris.immutables.PolarisImmutable;
-/**
- * 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);
- }
+/** A wrapper for pagination information passed in as part of a request. */
+@PolarisImmutable
+@JsonSerialize(as = ImmutablePageToken.class)
+@JsonDeserialize(as = ImmutablePageToken.class)
Review Comment:
Would rather avoid these if possible
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Extensible pagination token implementation [polaris]
Posted by "snazy (via GitHub)" <gi...@apache.org>.
snazy commented on code in PR #1938:
URL: https://github.com/apache/polaris/pull/1938#discussion_r2168238831
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/pagination/Page.java:
##########
@@ -18,25 +18,99 @@
*/
package org.apache.polaris.core.persistence.pagination;
+import static java.util.Spliterators.iterator;
+
+import jakarta.annotation.Nullable;
+import java.util.ArrayList;
+import java.util.Iterator;
import java.util.List;
+import java.util.function.Function;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
/**
- * An immutable page of items plus their paging cursor. The {@link PageToken} here can be used to
- * continue the listing operation that generated the `items`.
+ * An immutable page of items plus the next-page token value, if there are more items. The {@link
+ * #encodedResponseToken()} here can be used to continue the listing operation that generated the
+ * `items`.
*/
public class Page<T> {
- public final PageToken pageToken;
- public final List<T> items;
+ private final PageToken request;
+ private final List<T> items;
+ @Nullable private final Token nextToken;
- public Page(PageToken pageToken, List<T> items) {
- this.pageToken = pageToken;
+ private Page(PageToken request, @Nullable Token nextToken, List<T> items) {
+ this.request = request;
+ this.nextToken = nextToken;
this.items = items;
}
/**
- * Used to wrap a {@link List<T>} of items into a {@link Page <T>} when there are no more pages
+ * Builds a complete response page for the full list of relevant items. No subsequence pages of
+ * related data exist.
*/
public static <T> Page<T> fromItems(List<T> items) {
- return new Page<>(new DonePageToken(), items);
+ return new Page<>(PageToken.readEverything(), null, items);
+ }
+
+ /**
+ * Produces a response page by consuming the number of items from the provided stream according to
+ * the {@code request} parameter. Source items can be converted to a different type by providing a
+ * {@code mapper} function. The page token for the response will be produced from the request data
+ * combined with the pointer to the next page of data provided by the {@code dataPointer}
+ * function.
+ *
+ * @param request defines pagination parameters that were uses to produce this page of data.
+ * @param items stream of source data
+ * @param mapper converter from source data types to response data types.
+ * @param tokenBuilder determines the {@link Token} used to start the next page of data given the
+ * last item from the previous page. The output of this function will be available from {@link
+ * PageToken#value()} associated with the request for the next page.
+ */
+ public static <R, T> Page<R> mapped(
+ PageToken request, Stream<T> items, Function<T, R> mapper, Function<T, Token> tokenBuilder) {
+ List<R> data;
+ T last = null;
+ if (!request.paginationRequested()) {
+ // short-cut for "no pagination"
+ data = items.map(mapper).collect(Collectors.toList());
+ } else {
+ data = new ArrayList<>(request.pageSize().orElse(10));
+
+ Iterator<T> it = iterator(items.spliterator());
+ int limit = request.pageSize().orElse(Integer.MAX_VALUE);
+ while (it.hasNext() && data.size() < limit) {
+ last = it.next();
+ data.add(mapper.apply(last));
+ }
+
+ // Signal "no more data" if the number of items is less than the requested page size or if
+ // there is no more data.
+ if (data.size() < limit || !it.hasNext()) {
+ last = null;
+ }
+ }
+
+ return new Page<>(request, tokenBuilder.apply(last), data);
+ }
+
+ public List<T> items() {
+ return items;
+ }
+
+ /**
+ * Returns a page token in encoded form suitable for returning to API clients. The string returned
+ * from this method is expected to be parsed by {@link PageToken#build(String, Integer)} when
+ * servicing the request for the next page of related data.
+ */
+ public @Nullable String encodedResponseToken() {
+ return PageTokenUtil.encodePageToken(request, nextToken);
Review Comment:
There is only one `PageToken`.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Extensible pagination token implementation [polaris]
Posted by "snazy (via GitHub)" <gi...@apache.org>.
snazy commented on code in PR #1938:
URL: https://github.com/apache/polaris/pull/1938#discussion_r2168236417
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/pagination/EntityIdToken.java:
##########
@@ -0,0 +1,66 @@
+/*
+ * 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 com.fasterxml.jackson.annotation.JsonProperty;
+import com.fasterxml.jackson.databind.annotation.JsonDeserialize;
+import com.fasterxml.jackson.databind.annotation.JsonSerialize;
+import jakarta.annotation.Nullable;
+import org.apache.polaris.core.entity.PolarisBaseEntity;
+import org.apache.polaris.immutables.PolarisImmutable;
+
+/** Pagination {@linkplain Token token} backed by {@link PolarisBaseEntity#getId() entity ID}. */
+@PolarisImmutable
+@JsonSerialize(as = ImmutableEntityIdToken.class)
+@JsonDeserialize(as = ImmutableEntityIdToken.class)
+public interface EntityIdToken extends Token {
+ String ID = "e";
Review Comment:
That contradicts the intent of being extensible.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Extensible pagination token implementation [polaris]
Posted by "eric-maynard (via GitHub)" <gi...@apache.org>.
eric-maynard commented on code in PR #1938:
URL: https://github.com/apache/polaris/pull/1938#discussion_r2167143319
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/pagination/PageToken.java:
##########
@@ -18,82 +18,75 @@
*/
package org.apache.polaris.core.persistence.pagination;
-import java.util.List;
-import java.util.Objects;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.fasterxml.jackson.databind.annotation.JsonDeserialize;
+import com.fasterxml.jackson.databind.annotation.JsonSerialize;
+import jakarta.annotation.Nullable;
+import java.util.Optional;
+import java.util.OptionalInt;
+import org.apache.polaris.immutables.PolarisImmutable;
-/**
- * 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);
- }
+/** A wrapper for pagination information passed in as part of a request. */
+@PolarisImmutable
+@JsonSerialize(as = ImmutablePageToken.class)
+@JsonDeserialize(as = ImmutablePageToken.class)
+public interface PageToken {
+ // Serialization property names are intentionally short to reduce the size of the serialized
+ // paging token.
- /** 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);
- }
+ /** The requested page size (optional). */
+ @JsonProperty("p")
+ OptionalInt 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 LimitPageToken(pageSize);
- } else {
- return new ReadEverythingPageToken();
- }
- } else {
- // TODO implement, split out by the token's prefix
- throw new IllegalArgumentException("Unrecognized page token: " + token);
- }
+ /** Convenience for {@code pageSize().isPresent()}. */
+ default boolean paginationRequested() {
+ return pageSize().isPresent();
Review Comment:
It's possible per the spec to request pagination by providing a page token without page size. This logic is not correct.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Extensible pagination token implementation [polaris]
Posted by "eric-maynard (via GitHub)" <gi...@apache.org>.
eric-maynard commented on code in PR #1938:
URL: https://github.com/apache/polaris/pull/1938#discussion_r2167135430
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/dao/entity/ListEntitiesResult.java:
##########
@@ -46,43 +41,26 @@ public static ListEntitiesResult fromPage(Page<EntityNameLookupRecord> page) {
* @param errorCode error code, cannot be SUCCESS
* @param extraInformation extra information
*/
- public ListEntitiesResult(
- @Nonnull ReturnStatus errorCode,
- @Nullable String extraInformation,
- @Nonnull Optional<PageToken> pageTokenOpt) {
+ public ListEntitiesResult(@Nonnull ReturnStatus errorCode, @Nullable String extraInformation) {
super(errorCode, extraInformation);
this.entities = null;
- this.pageTokenOpt = pageTokenOpt;
}
/**
* Constructor for success
*
* @param entities list of entities being returned, implies success
*/
- public ListEntitiesResult(
- @Nonnull List<EntityNameLookupRecord> entities, @Nonnull Optional<PageToken> pageTokenOpt) {
+ public ListEntitiesResult(Page<EntityNameLookupRecord> entities) {
super(ReturnStatus.SUCCESS);
this.entities = entities;
- this.pageTokenOpt = pageTokenOpt;
}
- @JsonCreator
- private ListEntitiesResult(
Review Comment:
ditto the comment on wire-compatibility with previous versions
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Extensible pagination token implementation [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #1938:
URL: https://github.com/apache/polaris/pull/1938#discussion_r2168015563
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/pagination/EntityIdToken.java:
##########
@@ -0,0 +1,66 @@
+/*
+ * 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 com.fasterxml.jackson.annotation.JsonProperty;
+import com.fasterxml.jackson.databind.annotation.JsonDeserialize;
+import com.fasterxml.jackson.databind.annotation.JsonSerialize;
+import jakarta.annotation.Nullable;
+import org.apache.polaris.core.entity.PolarisBaseEntity;
+import org.apache.polaris.immutables.PolarisImmutable;
+
+/** Pagination {@linkplain Token token} backed by {@link PolarisBaseEntity#getId() entity ID}. */
+@PolarisImmutable
+@JsonSerialize(as = ImmutableEntityIdToken.class)
+@JsonDeserialize(as = ImmutableEntityIdToken.class)
+public interface EntityIdToken extends Token {
+ String ID = "e";
+
+ @JsonProperty("i")
Review Comment:
I believe short names are meant to reduce the side of the token on the wire.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Extensible pagination token implementation [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #1938:
URL: https://github.com/apache/polaris/pull/1938#discussion_r2168014176
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/dao/entity/EntitiesResult.java:
##########
@@ -48,41 +43,19 @@ public static EntitiesResult fromPage(Page<PolarisBaseEntity> page) {
public EntitiesResult(@Nonnull ReturnStatus errorStatus, @Nullable String extraInformation) {
super(errorStatus, extraInformation);
this.entities = null;
- this.pageTokenOpt = Optional.empty();
- }
-
- public EntitiesResult(@Nonnull List<PolarisBaseEntity> entities) {
- this(entities, Optional.empty());
}
/**
* Constructor for success
*
* @param entities list of entities being returned, implies success
*/
- public EntitiesResult(
- @Nonnull List<PolarisBaseEntity> entities, @Nonnull Optional<PageToken> pageTokenOpt) {
+ public EntitiesResult(@Nonnull Page<PolarisBaseEntity> entities) {
super(ReturnStatus.SUCCESS);
this.entities = entities;
- this.pageTokenOpt = pageTokenOpt;
- }
-
- @JsonCreator
- private EntitiesResult(
Review Comment:
I do not see any roundtrips through JSON for `EntitiesResult` in Apache Polaris code. If this is a use case downstream, it would be nice to have tests in this repo to assert the expected behaviour.
That said, I think this specific change is not critical to pagination. `EntitiesResult` is not involved handling paginated data. So, I suppose this change can be rolled back.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Extensible pagination token implementation [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #1938:
URL: https://github.com/apache/polaris/pull/1938#discussion_r2168008808
##########
polaris-core/src/test/resources/META-INF/services/org.apache.polaris.core.persistence.pagination.Token$TokenType:
##########
Review Comment:
`Token$TokenType` looks awkward as a file name (`$` and double `Token`)... WDYT about making `TokenType` a top-level class?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Extensible pagination token implementation [polaris]
Posted by "eric-maynard (via GitHub)" <gi...@apache.org>.
eric-maynard commented on code in PR #1938:
URL: https://github.com/apache/polaris/pull/1938#discussion_r2167136566
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/pagination/EntityIdToken.java:
##########
@@ -0,0 +1,66 @@
+/*
+ * 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 com.fasterxml.jackson.annotation.JsonProperty;
+import com.fasterxml.jackson.databind.annotation.JsonDeserialize;
+import com.fasterxml.jackson.databind.annotation.JsonSerialize;
+import jakarta.annotation.Nullable;
+import org.apache.polaris.core.entity.PolarisBaseEntity;
+import org.apache.polaris.immutables.PolarisImmutable;
+
+/** Pagination {@linkplain Token token} backed by {@link PolarisBaseEntity#getId() entity ID}. */
+@PolarisImmutable
+@JsonSerialize(as = ImmutableEntityIdToken.class)
+@JsonDeserialize(as = ImmutableEntityIdToken.class)
+public interface EntityIdToken extends Token {
+ String ID = "e";
+
+ @JsonProperty("i")
+ long entityId();
+
+ @Override
+ default String getT() {
Review Comment:
`getT`?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Extensible pagination token implementation [polaris]
Posted by "eric-maynard (via GitHub)" <gi...@apache.org>.
eric-maynard commented on code in PR #1938:
URL: https://github.com/apache/polaris/pull/1938#discussion_r2167144153
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/pagination/PageToken.java:
##########
@@ -18,82 +18,75 @@
*/
package org.apache.polaris.core.persistence.pagination;
-import java.util.List;
-import java.util.Objects;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.fasterxml.jackson.databind.annotation.JsonDeserialize;
+import com.fasterxml.jackson.databind.annotation.JsonSerialize;
+import jakarta.annotation.Nullable;
+import java.util.Optional;
+import java.util.OptionalInt;
+import org.apache.polaris.immutables.PolarisImmutable;
-/**
- * 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);
- }
+/** A wrapper for pagination information passed in as part of a request. */
+@PolarisImmutable
+@JsonSerialize(as = ImmutablePageToken.class)
+@JsonDeserialize(as = ImmutablePageToken.class)
+public interface PageToken {
+ // Serialization property names are intentionally short to reduce the size of the serialized
+ // paging token.
- /** 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);
- }
+ /** The requested page size (optional). */
+ @JsonProperty("p")
+ OptionalInt 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 LimitPageToken(pageSize);
- } else {
- return new ReadEverythingPageToken();
- }
- } else {
- // TODO implement, split out by the token's prefix
- throw new IllegalArgumentException("Unrecognized page token: " + token);
- }
+ /** Convenience for {@code pageSize().isPresent()}. */
+ default boolean paginationRequested() {
+ return pageSize().isPresent();
}
- /** Serialize a {@link PageToken} into a string */
- public abstract String toTokenString();
-
/**
- * Builds a new page token to reflect new data that's been read. If the amount of data read is
- * less than the pageSize, this will return a {@link DonePageToken}
+ * Paging token value, if present. Serialized paging tokens always have a value, but "synthetic"
+ * paging tokens like {@link #readEverything()} or {@link #fromLimit(int)} do not have a token
+ * value.
*/
- protected abstract PageToken updated(List<?> newData);
+ @JsonProperty("v")
+ Optional<Token> value();
+
+ // Note: another property can be added to contain a (cryptographic) signature, if we want to
+ // ensure that a paging-token hasn't been tampered.
/**
- * Builds a {@link Page <T>} from a {@link List<T>}. The {@link PageToken} attached to the new
- * {@link Page <T>} is the same as the result of calling {@link #updated(List)} on this {@link
- * PageToken}.
+ * Paging token value, if it is present and an instance of the given {@code type}. This is a
+ * convenience to prevent duplication of type casts.
*/
- public final <T> Page<T> buildNextPage(List<T> data) {
- return new Page<T>(updated(data), data);
+ default <T extends Token> Optional<T> valueAs(Class<T> type) {
+ return value()
+ .flatMap(
+ t ->
+ type.isAssignableFrom(t.getClass()) ? Optional.of(type.cast(t)) : Optional.empty());
+ }
+
+ /** Represents a non-paginated request. */
+ static PageToken readEverything() {
+ return PageTokenUtil.READ_EVERYTHING;
Review Comment:
Is `READ_EVERYTHING` a `Util`? Seems like it would belong in `PageToken`
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Extensible pagination token implementation [polaris]
Posted by "eric-maynard (via GitHub)" <gi...@apache.org>.
eric-maynard commented on code in PR #1938:
URL: https://github.com/apache/polaris/pull/1938#discussion_r2167140911
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/pagination/Page.java:
##########
@@ -18,25 +18,99 @@
*/
package org.apache.polaris.core.persistence.pagination;
+import static java.util.Spliterators.iterator;
+
+import jakarta.annotation.Nullable;
+import java.util.ArrayList;
+import java.util.Iterator;
import java.util.List;
+import java.util.function.Function;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
/**
- * An immutable page of items plus their paging cursor. The {@link PageToken} here can be used to
- * continue the listing operation that generated the `items`.
+ * An immutable page of items plus the next-page token value, if there are more items. The {@link
+ * #encodedResponseToken()} here can be used to continue the listing operation that generated the
+ * `items`.
*/
public class Page<T> {
- public final PageToken pageToken;
- public final List<T> items;
+ private final PageToken request;
+ private final List<T> items;
+ @Nullable private final Token nextToken;
- public Page(PageToken pageToken, List<T> items) {
- this.pageToken = pageToken;
+ private Page(PageToken request, @Nullable Token nextToken, List<T> items) {
+ this.request = request;
+ this.nextToken = nextToken;
this.items = items;
}
/**
- * Used to wrap a {@link List<T>} of items into a {@link Page <T>} when there are no more pages
+ * Builds a complete response page for the full list of relevant items. No subsequence pages of
+ * related data exist.
*/
public static <T> Page<T> fromItems(List<T> items) {
- return new Page<>(new DonePageToken(), items);
+ return new Page<>(PageToken.readEverything(), null, items);
+ }
+
+ /**
+ * Produces a response page by consuming the number of items from the provided stream according to
+ * the {@code request} parameter. Source items can be converted to a different type by providing a
+ * {@code mapper} function. The page token for the response will be produced from the request data
+ * combined with the pointer to the next page of data provided by the {@code dataPointer}
+ * function.
+ *
+ * @param request defines pagination parameters that were uses to produce this page of data.
+ * @param items stream of source data
+ * @param mapper converter from source data types to response data types.
+ * @param tokenBuilder determines the {@link Token} used to start the next page of data given the
+ * last item from the previous page. The output of this function will be available from {@link
+ * PageToken#value()} associated with the request for the next page.
+ */
+ public static <R, T> Page<R> mapped(
+ PageToken request, Stream<T> items, Function<T, R> mapper, Function<T, Token> tokenBuilder) {
+ List<R> data;
+ T last = null;
+ if (!request.paginationRequested()) {
+ // short-cut for "no pagination"
+ data = items.map(mapper).collect(Collectors.toList());
+ } else {
+ data = new ArrayList<>(request.pageSize().orElse(10));
+
+ Iterator<T> it = iterator(items.spliterator());
+ int limit = request.pageSize().orElse(Integer.MAX_VALUE);
+ while (it.hasNext() && data.size() < limit) {
+ last = it.next();
+ data.add(mapper.apply(last));
+ }
+
+ // Signal "no more data" if the number of items is less than the requested page size or if
+ // there is no more data.
+ if (data.size() < limit || !it.hasNext()) {
+ last = null;
+ }
+ }
+
+ return new Page<>(request, tokenBuilder.apply(last), data);
+ }
+
+ public List<T> items() {
+ return items;
+ }
+
+ /**
+ * Returns a page token in encoded form suitable for returning to API clients. The string returned
+ * from this method is expected to be parsed by {@link PageToken#build(String, Integer)} when
+ * servicing the request for the next page of related data.
+ */
+ public @Nullable String encodedResponseToken() {
+ return PageTokenUtil.encodePageToken(request, nextToken);
Review Comment:
Same comments from the previous PR -- encoding/decoding a given PageToken clearly seems to be within the purview of a given PageToken implementation
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Extensible pagination token implementation [polaris]
Posted by "snazy (via GitHub)" <gi...@apache.org>.
snazy commented on code in PR #1938:
URL: https://github.com/apache/polaris/pull/1938#discussion_r2168252015
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/dao/entity/EntitiesResult.java:
##########
@@ -48,41 +43,19 @@ public static EntitiesResult fromPage(Page<PolarisBaseEntity> page) {
public EntitiesResult(@Nonnull ReturnStatus errorStatus, @Nullable String extraInformation) {
super(errorStatus, extraInformation);
this.entities = null;
- this.pageTokenOpt = Optional.empty();
- }
-
- public EntitiesResult(@Nonnull List<PolarisBaseEntity> entities) {
- this(entities, Optional.empty());
}
/**
* Constructor for success
*
* @param entities list of entities being returned, implies success
*/
- public EntitiesResult(
- @Nonnull List<PolarisBaseEntity> entities, @Nonnull Optional<PageToken> pageTokenOpt) {
+ public EntitiesResult(@Nonnull Page<PolarisBaseEntity> entities) {
super(ReturnStatus.SUCCESS);
this.entities = entities;
- this.pageTokenOpt = pageTokenOpt;
- }
-
- @JsonCreator
- private EntitiesResult(
Review Comment:
I do not see a "wire" either.
BTW: The same change wasn't considered an issue [in 1838](https://github.com/apache/polaris/pull/1838/files#diff-3fc6d3601347843472388857f7b6237399973e08711fc3d04b6b7589b1cedc4eL70).
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Extensible pagination token implementation [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #1938:
URL: https://github.com/apache/polaris/pull/1938#discussion_r2167155668
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/dao/entity/EntitiesResult.java:
##########
@@ -48,41 +43,19 @@ public static EntitiesResult fromPage(Page<PolarisBaseEntity> page) {
public EntitiesResult(@Nonnull ReturnStatus errorStatus, @Nullable String extraInformation) {
super(errorStatus, extraInformation);
this.entities = null;
- this.pageTokenOpt = Optional.empty();
- }
-
- public EntitiesResult(@Nonnull List<PolarisBaseEntity> entities) {
- this(entities, Optional.empty());
}
/**
* Constructor for success
*
* @param entities list of entities being returned, implies success
*/
- public EntitiesResult(
- @Nonnull List<PolarisBaseEntity> entities, @Nonnull Optional<PageToken> pageTokenOpt) {
+ public EntitiesResult(@Nonnull Page<PolarisBaseEntity> entities) {
super(ReturnStatus.SUCCESS);
this.entities = entities;
- this.pageTokenOpt = pageTokenOpt;
- }
-
- @JsonCreator
- private EntitiesResult(
Review Comment:
It is a `private` constructor... @eric-maynard @dennishuo : could you give some more details about possible compatibility issues? What's the affected workflow?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Extensible pagination token implementation [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #1938:
URL: https://github.com/apache/polaris/pull/1938#discussion_r2168062040
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/pagination/PageToken.java:
##########
@@ -18,82 +18,75 @@
*/
package org.apache.polaris.core.persistence.pagination;
-import java.util.List;
-import java.util.Objects;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.fasterxml.jackson.databind.annotation.JsonDeserialize;
+import com.fasterxml.jackson.databind.annotation.JsonSerialize;
+import jakarta.annotation.Nullable;
+import java.util.Optional;
+import java.util.OptionalInt;
+import org.apache.polaris.immutables.PolarisImmutable;
-/**
- * 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() {
Review Comment:
Could we keep this utility method here?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Extensible pagination token implementation [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #1938:
URL: https://github.com/apache/polaris/pull/1938#discussion_r2168024000
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/pagination/PageToken.java:
##########
@@ -18,82 +18,75 @@
*/
package org.apache.polaris.core.persistence.pagination;
-import java.util.List;
-import java.util.Objects;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.fasterxml.jackson.databind.annotation.JsonDeserialize;
+import com.fasterxml.jackson.databind.annotation.JsonSerialize;
+import jakarta.annotation.Nullable;
+import java.util.Optional;
+import java.util.OptionalInt;
+import org.apache.polaris.immutables.PolarisImmutable;
-/**
- * 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);
- }
+/** A wrapper for pagination information passed in as part of a request. */
+@PolarisImmutable
+@JsonSerialize(as = ImmutablePageToken.class)
+@JsonDeserialize(as = ImmutablePageToken.class)
+public interface PageToken {
+ // Serialization property names are intentionally short to reduce the size of the serialized
+ // paging token.
- /** 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);
- }
+ /** The requested page size (optional). */
+ @JsonProperty("p")
+ OptionalInt 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 LimitPageToken(pageSize);
- } else {
- return new ReadEverythingPageToken();
- }
- } else {
- // TODO implement, split out by the token's prefix
- throw new IllegalArgumentException("Unrecognized page token: " + token);
- }
+ /** Convenience for {@code pageSize().isPresent()}. */
+ default boolean paginationRequested() {
+ return pageSize().isPresent();
Review Comment:
The logic looks correct to me. Propagating page size from the previous token in handled in `PageTokenUtil.decodePageRequest()`
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Extensible pagination token implementation [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #1938:
URL: https://github.com/apache/polaris/pull/1938#discussion_r2168025609
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/transactional/TreeMapTransactionalPersistenceImpl.java:
##########
@@ -361,13 +362,23 @@ public List<EntityNameLookupRecord> lookupEntityActiveBatchInCurrentTxn(
.map(
nameRecord ->
this.lookupEntityInCurrentTxn(
- callCtx, catalogId, nameRecord.getId(), entityType.getCode()))
- .filter(entityFilter);
- if (pageToken instanceof HasPageSize) {
- data = data.limit(((HasPageSize) pageToken).getPageSize());
- }
+ callCtx, catalogId, nameRecord.getId(), entityType.getCode()));
+
+ Predicate<PolarisBaseEntity> tokenFilter =
+ pageToken
+ .valueAs(EntityIdToken.class)
+ .map(
+ entityIdToken -> {
+ var nextId = entityIdToken.entityId();
+ return (Predicate<PolarisBaseEntity>) e -> e.getId() > nextId;
+ })
+ .orElse(e -> true);
+
+ data = data.sorted(Comparator.comparingLong(PolarisEntityCore::getId)).filter(tokenFilter);
+
+ data = data.filter(entityFilter);
Review Comment:
(I think it came from my PR) No particular reason. Would you prefer a chained call?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Extensible pagination token implementation [polaris]
Posted by "eric-maynard (via GitHub)" <gi...@apache.org>.
eric-maynard commented on code in PR #1938:
URL: https://github.com/apache/polaris/pull/1938#discussion_r2167142786
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/pagination/PageToken.java:
##########
@@ -18,82 +18,75 @@
*/
package org.apache.polaris.core.persistence.pagination;
-import java.util.List;
-import java.util.Objects;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.fasterxml.jackson.databind.annotation.JsonDeserialize;
+import com.fasterxml.jackson.databind.annotation.JsonSerialize;
+import jakarta.annotation.Nullable;
+import java.util.Optional;
+import java.util.OptionalInt;
+import org.apache.polaris.immutables.PolarisImmutable;
-/**
- * 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);
- }
+/** A wrapper for pagination information passed in as part of a request. */
+@PolarisImmutable
+@JsonSerialize(as = ImmutablePageToken.class)
+@JsonDeserialize(as = ImmutablePageToken.class)
+public interface PageToken {
+ // Serialization property names are intentionally short to reduce the size of the serialized
+ // paging token.
- /** 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);
- }
+ /** The requested page size (optional). */
+ @JsonProperty("p")
+ OptionalInt 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 LimitPageToken(pageSize);
- } else {
- return new ReadEverythingPageToken();
- }
- } else {
- // TODO implement, split out by the token's prefix
- throw new IllegalArgumentException("Unrecognized page token: " + token);
- }
+ /** Convenience for {@code pageSize().isPresent()}. */
+ default boolean paginationRequested() {
+ return pageSize().isPresent();
}
- /** Serialize a {@link PageToken} into a string */
- public abstract String toTokenString();
-
/**
- * Builds a new page token to reflect new data that's been read. If the amount of data read is
- * less than the pageSize, this will return a {@link DonePageToken}
+ * Paging token value, if present. Serialized paging tokens always have a value, but "synthetic"
+ * paging tokens like {@link #readEverything()} or {@link #fromLimit(int)} do not have a token
+ * value.
*/
- protected abstract PageToken updated(List<?> newData);
+ @JsonProperty("v")
+ Optional<Token> value();
Review Comment:
What is `value`? This seems a bit abstract.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Extensible pagination token implementation [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #1938:
URL: https://github.com/apache/polaris/pull/1938#discussion_r2208143657
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/pagination/PageToken.java:
##########
@@ -18,82 +18,75 @@
*/
package org.apache.polaris.core.persistence.pagination;
-import java.util.List;
-import java.util.Objects;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.fasterxml.jackson.databind.annotation.JsonDeserialize;
+import com.fasterxml.jackson.databind.annotation.JsonSerialize;
+import jakarta.annotation.Nullable;
+import java.util.Optional;
+import java.util.OptionalInt;
+import org.apache.polaris.immutables.PolarisImmutable;
-/**
- * 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);
- }
+/** A wrapper for pagination information passed in as part of a request. */
+@PolarisImmutable
+@JsonSerialize(as = ImmutablePageToken.class)
+@JsonDeserialize(as = ImmutablePageToken.class)
+public interface PageToken {
+ // Serialization property names are intentionally short to reduce the size of the serialized
+ // paging token.
- /** 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);
- }
+ /** The requested page size (optional). */
+ @JsonProperty("p")
+ OptionalInt 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 LimitPageToken(pageSize);
- } else {
- return new ReadEverythingPageToken();
- }
- } else {
- // TODO implement, split out by the token's prefix
- throw new IllegalArgumentException("Unrecognized page token: " + token);
- }
+ /** Convenience for {@code pageSize().isPresent()}. */
+ default boolean paginationRequested() {
+ return pageSize().isPresent();
}
- /** Serialize a {@link PageToken} into a string */
- public abstract String toTokenString();
-
/**
- * Builds a new page token to reflect new data that's been read. If the amount of data read is
- * less than the pageSize, this will return a {@link DonePageToken}
+ * Paging token value, if present. Serialized paging tokens always have a value, but "synthetic"
+ * paging tokens like {@link #readEverything()} or {@link #fromLimit(int)} do not have a token
+ * value.
*/
- protected abstract PageToken updated(List<?> newData);
+ @JsonProperty("v")
+ Optional<Token> value();
Review Comment:
My take: `value` is the implementation-specific token data (e.g. the entity ID in sorted SQL result sets).
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Extensible pagination token implementation [polaris]
Posted by "eric-maynard (via GitHub)" <gi...@apache.org>.
eric-maynard commented on code in PR #1938:
URL: https://github.com/apache/polaris/pull/1938#discussion_r2167137765
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/pagination/EntityIdToken.java:
##########
@@ -0,0 +1,66 @@
+/*
+ * 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 com.fasterxml.jackson.annotation.JsonProperty;
+import com.fasterxml.jackson.databind.annotation.JsonDeserialize;
+import com.fasterxml.jackson.databind.annotation.JsonSerialize;
+import jakarta.annotation.Nullable;
+import org.apache.polaris.core.entity.PolarisBaseEntity;
+import org.apache.polaris.immutables.PolarisImmutable;
+
+/** Pagination {@linkplain Token token} backed by {@link PolarisBaseEntity#getId() entity ID}. */
+@PolarisImmutable
+@JsonSerialize(as = ImmutableEntityIdToken.class)
+@JsonDeserialize(as = ImmutableEntityIdToken.class)
+public interface EntityIdToken extends Token {
+ String ID = "e";
Review Comment:
Is this meant to be short for `EntityId`? Why not use an enum?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Extensible pagination token implementation [polaris]
Posted by "eric-maynard (via GitHub)" <gi...@apache.org>.
eric-maynard commented on PR #1938:
URL: https://github.com/apache/polaris/pull/1938#issuecomment-3005415828
Thanks for taking a crack at this. I appreciate the extra extensibility compared to #1838, but considering this diff is twice the size of #1555, I'm not sure there's enough functionality here to justify all the changes. I took a quick pass for now, but will try to circle back and review the rest of the PR in more detail later.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Extensible pagination token implementation [polaris]
Posted by "snazy (via GitHub)" <gi...@apache.org>.
snazy commented on code in PR #1938:
URL: https://github.com/apache/polaris/pull/1938#discussion_r2168234339
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/pagination/EntityIdToken.java:
##########
@@ -0,0 +1,66 @@
+/*
+ * 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 com.fasterxml.jackson.annotation.JsonProperty;
+import com.fasterxml.jackson.databind.annotation.JsonDeserialize;
+import com.fasterxml.jackson.databind.annotation.JsonSerialize;
+import jakarta.annotation.Nullable;
+import org.apache.polaris.core.entity.PolarisBaseEntity;
+import org.apache.polaris.immutables.PolarisImmutable;
+
+/** Pagination {@linkplain Token token} backed by {@link PolarisBaseEntity#getId() entity ID}. */
+@PolarisImmutable
+@JsonSerialize(as = ImmutableEntityIdToken.class)
+@JsonDeserialize(as = ImmutableEntityIdToken.class)
+public interface EntityIdToken extends Token {
+ String ID = "e";
+
+ @JsonProperty("i")
+ long entityId();
+
+ @Override
+ default String getT() {
Review Comment:
The reason is explained as doc in `Token`
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/pagination/EntityIdToken.java:
##########
@@ -0,0 +1,66 @@
+/*
+ * 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 com.fasterxml.jackson.annotation.JsonProperty;
+import com.fasterxml.jackson.databind.annotation.JsonDeserialize;
+import com.fasterxml.jackson.databind.annotation.JsonSerialize;
+import jakarta.annotation.Nullable;
+import org.apache.polaris.core.entity.PolarisBaseEntity;
+import org.apache.polaris.immutables.PolarisImmutable;
+
+/** Pagination {@linkplain Token token} backed by {@link PolarisBaseEntity#getId() entity ID}. */
+@PolarisImmutable
+@JsonSerialize(as = ImmutableEntityIdToken.class)
+@JsonDeserialize(as = ImmutableEntityIdToken.class)
+public interface EntityIdToken extends Token {
+ String ID = "e";
+
+ @JsonProperty("i")
Review Comment:
Yes, as explained in `Token`
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Extensible pagination token implementation [polaris]
Posted by "snazy (via GitHub)" <gi...@apache.org>.
snazy commented on code in PR #1938:
URL: https://github.com/apache/polaris/pull/1938#discussion_r2168261305
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/pagination/PageToken.java:
##########
@@ -18,82 +18,75 @@
*/
package org.apache.polaris.core.persistence.pagination;
-import java.util.List;
-import java.util.Objects;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.fasterxml.jackson.databind.annotation.JsonDeserialize;
+import com.fasterxml.jackson.databind.annotation.JsonSerialize;
+import jakarta.annotation.Nullable;
+import java.util.Optional;
+import java.util.OptionalInt;
+import org.apache.polaris.immutables.PolarisImmutable;
-/**
- * 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() {
Review Comment:
It is still there?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Extensible pagination token implementation [polaris]
Posted by "eric-maynard (via GitHub)" <gi...@apache.org>.
eric-maynard commented on code in PR #1938:
URL: https://github.com/apache/polaris/pull/1938#discussion_r2211309234
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/pagination/PageTokenUtil.java:
##########
@@ -0,0 +1,267 @@
+/*
+ * 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 static com.google.common.base.Preconditions.checkArgument;
+import static java.lang.String.format;
+import static java.util.Collections.unmodifiableMap;
+
+import com.fasterxml.jackson.annotation.JsonTypeInfo;
+import com.fasterxml.jackson.core.JsonProcessingException;
+import com.fasterxml.jackson.databind.DatabindContext;
+import com.fasterxml.jackson.databind.JavaType;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.fasterxml.jackson.databind.jsontype.impl.TypeIdResolverBase;
+import com.fasterxml.jackson.dataformat.smile.databind.SmileMapper;
+import com.google.common.annotations.VisibleForTesting;
+import jakarta.annotation.Nullable;
+import java.io.IOException;
+import java.util.Base64;
+import java.util.HashMap;
+import java.util.Locale;
+import java.util.Map;
+import java.util.Optional;
+import java.util.OptionalInt;
+import java.util.ServiceLoader;
+
+final class PageTokenUtil {
+
+ private static final ObjectMapper SMILE_MAPPER = new SmileMapper().findAndRegisterModules();
Review Comment:
I think the issue I mentioned is likely happening when there are multiple Scala dependencies on the classpath?
If we are calling `findAndRegisterModules`, I wonder if we should be explicitly adding a dependency for Scala, e.g.:
```
implementation("org.scala-lang:scala-library:2.12.15")
```
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Extensible pagination token implementation [polaris]
Posted by "eric-maynard (via GitHub)" <gi...@apache.org>.
eric-maynard commented on code in PR #1938:
URL: https://github.com/apache/polaris/pull/1938#discussion_r2213771840
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/pagination/PageTokenUtil.java:
##########
@@ -0,0 +1,267 @@
+/*
+ * 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 static com.google.common.base.Preconditions.checkArgument;
+import static java.lang.String.format;
+import static java.util.Collections.unmodifiableMap;
+
+import com.fasterxml.jackson.annotation.JsonTypeInfo;
+import com.fasterxml.jackson.core.JsonProcessingException;
+import com.fasterxml.jackson.databind.DatabindContext;
+import com.fasterxml.jackson.databind.JavaType;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.fasterxml.jackson.databind.jsontype.impl.TypeIdResolverBase;
+import com.fasterxml.jackson.dataformat.smile.databind.SmileMapper;
+import com.google.common.annotations.VisibleForTesting;
+import jakarta.annotation.Nullable;
+import java.io.IOException;
+import java.util.Base64;
+import java.util.HashMap;
+import java.util.Locale;
+import java.util.Map;
+import java.util.Optional;
+import java.util.OptionalInt;
+import java.util.ServiceLoader;
+
+final class PageTokenUtil {
+
+ private static final ObjectMapper SMILE_MAPPER = new SmileMapper().findAndRegisterModules();
Review Comment:
Well, it might, depending on the extensions you have. From what I can tell this actually functions without `findAndRegisterModules` -- why do we need it?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Extensible pagination token implementation [polaris]
Posted by "snazy (via GitHub)" <gi...@apache.org>.
snazy commented on PR #1938:
URL: https://github.com/apache/polaris/pull/1938#issuecomment-3005321996
This approach also works for NoSQL #1189 (last commit in [this branch](https://github.com/snazy/polaris/tree/persistence-nosql-paging))
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Extensible pagination token implementation [polaris]
Posted by "snazy (via GitHub)" <gi...@apache.org>.
snazy commented on code in PR #1938:
URL: https://github.com/apache/polaris/pull/1938#discussion_r2168253055
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/pagination/Page.java:
##########
@@ -18,25 +18,99 @@
*/
package org.apache.polaris.core.persistence.pagination;
+import static java.util.Spliterators.iterator;
+
+import jakarta.annotation.Nullable;
+import java.util.ArrayList;
+import java.util.Iterator;
import java.util.List;
+import java.util.function.Function;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
/**
- * An immutable page of items plus their paging cursor. The {@link PageToken} here can be used to
- * continue the listing operation that generated the `items`.
+ * An immutable page of items plus the next-page token value, if there are more items. The {@link
+ * #encodedResponseToken()} here can be used to continue the listing operation that generated the
+ * `items`.
*/
public class Page<T> {
- public final PageToken pageToken;
- public final List<T> items;
+ private final PageToken request;
+ private final List<T> items;
+ @Nullable private final Token nextToken;
- public Page(PageToken pageToken, List<T> items) {
- this.pageToken = pageToken;
+ private Page(PageToken request, @Nullable Token nextToken, List<T> items) {
+ this.request = request;
+ this.nextToken = nextToken;
this.items = items;
}
/**
- * Used to wrap a {@link List<T>} of items into a {@link Page <T>} when there are no more pages
+ * Builds a complete response page for the full list of relevant items. No subsequence pages of
+ * related data exist.
*/
public static <T> Page<T> fromItems(List<T> items) {
- return new Page<>(new DonePageToken(), items);
+ return new Page<>(PageToken.readEverything(), null, items);
+ }
+
+ /**
+ * Produces a response page by consuming the number of items from the provided stream according to
+ * the {@code request} parameter. Source items can be converted to a different type by providing a
+ * {@code mapper} function. The page token for the response will be produced from the request data
+ * combined with the pointer to the next page of data provided by the {@code dataPointer}
+ * function.
+ *
+ * @param request defines pagination parameters that were uses to produce this page of data.
+ * @param items stream of source data
+ * @param mapper converter from source data types to response data types.
+ * @param tokenBuilder determines the {@link Token} used to start the next page of data given the
+ * last item from the previous page. The output of this function will be available from {@link
+ * PageToken#value()} associated with the request for the next page.
+ */
+ public static <R, T> Page<R> mapped(
+ PageToken request, Stream<T> items, Function<T, R> mapper, Function<T, Token> tokenBuilder) {
+ List<R> data;
+ T last = null;
+ if (!request.paginationRequested()) {
+ // short-cut for "no pagination"
+ data = items.map(mapper).collect(Collectors.toList());
+ } else {
+ data = new ArrayList<>(request.pageSize().orElse(10));
+
+ Iterator<T> it = iterator(items.spliterator());
+ int limit = request.pageSize().orElse(Integer.MAX_VALUE);
+ while (it.hasNext() && data.size() < limit) {
+ last = it.next();
+ data.add(mapper.apply(last));
+ }
+
+ // Signal "no more data" if the number of items is less than the requested page size or if
+ // there is no more data.
+ if (data.size() < limit || !it.hasNext()) {
Review Comment:
There is a test for this - and it passes
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Extensible pagination token implementation [polaris]
Posted by "snazy (via GitHub)" <gi...@apache.org>.
snazy commented on code in PR #1938:
URL: https://github.com/apache/polaris/pull/1938#discussion_r2168263352
##########
polaris-core/src/test/resources/META-INF/services/org.apache.polaris.core.persistence.pagination.Token$TokenType:
##########
Review Comment:
It's the class name.
I'd really prefer keeping these very related things (`Token` + `TokenType`) together - you have to implement both.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Extensible pagination token implementation [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #1938:
URL: https://github.com/apache/polaris/pull/1938#discussion_r2168018610
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/pagination/Page.java:
##########
@@ -18,25 +18,99 @@
*/
package org.apache.polaris.core.persistence.pagination;
+import static java.util.Spliterators.iterator;
+
+import jakarta.annotation.Nullable;
+import java.util.ArrayList;
+import java.util.Iterator;
import java.util.List;
+import java.util.function.Function;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
/**
- * An immutable page of items plus their paging cursor. The {@link PageToken} here can be used to
- * continue the listing operation that generated the `items`.
+ * An immutable page of items plus the next-page token value, if there are more items. The {@link
+ * #encodedResponseToken()} here can be used to continue the listing operation that generated the
+ * `items`.
*/
public class Page<T> {
- public final PageToken pageToken;
- public final List<T> items;
+ private final PageToken request;
+ private final List<T> items;
+ @Nullable private final Token nextToken;
- public Page(PageToken pageToken, List<T> items) {
- this.pageToken = pageToken;
+ private Page(PageToken request, @Nullable Token nextToken, List<T> items) {
+ this.request = request;
+ this.nextToken = nextToken;
this.items = items;
}
/**
- * Used to wrap a {@link List<T>} of items into a {@link Page <T>} when there are no more pages
+ * Builds a complete response page for the full list of relevant items. No subsequence pages of
+ * related data exist.
*/
public static <T> Page<T> fromItems(List<T> items) {
- return new Page<>(new DonePageToken(), items);
+ return new Page<>(PageToken.readEverything(), null, items);
+ }
+
+ /**
+ * Produces a response page by consuming the number of items from the provided stream according to
+ * the {@code request} parameter. Source items can be converted to a different type by providing a
+ * {@code mapper} function. The page token for the response will be produced from the request data
+ * combined with the pointer to the next page of data provided by the {@code dataPointer}
+ * function.
+ *
+ * @param request defines pagination parameters that were uses to produce this page of data.
+ * @param items stream of source data
+ * @param mapper converter from source data types to response data types.
+ * @param tokenBuilder determines the {@link Token} used to start the next page of data given the
+ * last item from the previous page. The output of this function will be available from {@link
+ * PageToken#value()} associated with the request for the next page.
+ */
+ public static <R, T> Page<R> mapped(
+ PageToken request, Stream<T> items, Function<T, R> mapper, Function<T, Token> tokenBuilder) {
+ List<R> data;
+ T last = null;
+ if (!request.paginationRequested()) {
+ // short-cut for "no pagination"
+ data = items.map(mapper).collect(Collectors.toList());
+ } else {
+ data = new ArrayList<>(request.pageSize().orElse(10));
+
+ Iterator<T> it = iterator(items.spliterator());
+ int limit = request.pageSize().orElse(Integer.MAX_VALUE);
+ while (it.hasNext() && data.size() < limit) {
+ last = it.next();
+ data.add(mapper.apply(last));
+ }
+
+ // Signal "no more data" if the number of items is less than the requested page size or if
+ // there is no more data.
+ if (data.size() < limit || !it.hasNext()) {
Review Comment:
Good point! It also looks like we do not push limits down to the database.
I propose to remove `!it.hasNext()` for the sake of simplicity, accept possible empty last pages, and work on exact page boundaries later.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Extensible pagination token implementation [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #1938:
URL: https://github.com/apache/polaris/pull/1938#discussion_r2208156852
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/pagination/PageToken.java:
##########
@@ -18,82 +18,75 @@
*/
package org.apache.polaris.core.persistence.pagination;
-import java.util.List;
-import java.util.Objects;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.fasterxml.jackson.databind.annotation.JsonDeserialize;
+import com.fasterxml.jackson.databind.annotation.JsonSerialize;
+import jakarta.annotation.Nullable;
+import java.util.Optional;
+import java.util.OptionalInt;
+import org.apache.polaris.immutables.PolarisImmutable;
-/**
- * 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);
- }
+/** A wrapper for pagination information passed in as part of a request. */
+@PolarisImmutable
+@JsonSerialize(as = ImmutablePageToken.class)
+@JsonDeserialize(as = ImmutablePageToken.class)
+public interface PageToken {
+ // Serialization property names are intentionally short to reduce the size of the serialized
+ // paging token.
- /** 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);
- }
+ /** The requested page size (optional). */
+ @JsonProperty("p")
+ OptionalInt 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 LimitPageToken(pageSize);
- } else {
- return new ReadEverythingPageToken();
- }
- } else {
- // TODO implement, split out by the token's prefix
- throw new IllegalArgumentException("Unrecognized page token: " + token);
- }
+ /** Convenience for {@code pageSize().isPresent()}. */
+ default boolean paginationRequested() {
+ return pageSize().isPresent();
}
- /** Serialize a {@link PageToken} into a string */
- public abstract String toTokenString();
-
/**
- * Builds a new page token to reflect new data that's been read. If the amount of data read is
- * less than the pageSize, this will return a {@link DonePageToken}
+ * Paging token value, if present. Serialized paging tokens always have a value, but "synthetic"
+ * paging tokens like {@link #readEverything()} or {@link #fromLimit(int)} do not have a token
+ * value.
*/
- protected abstract PageToken updated(List<?> newData);
+ @JsonProperty("v")
+ Optional<Token> value();
+
+ // Note: another property can be added to contain a (cryptographic) signature, if we want to
+ // ensure that a paging-token hasn't been tampered.
/**
- * Builds a {@link Page <T>} from a {@link List<T>}. The {@link PageToken} attached to the new
- * {@link Page <T>} is the same as the result of calling {@link #updated(List)} on this {@link
- * PageToken}.
+ * Paging token value, if it is present and an instance of the given {@code type}. This is a
+ * convenience to prevent duplication of type casts.
*/
- public final <T> Page<T> buildNextPage(List<T> data) {
- return new Page<T>(updated(data), data);
+ default <T extends Token> Optional<T> valueAs(Class<T> type) {
+ return value()
+ .flatMap(
+ t ->
+ type.isAssignableFrom(t.getClass()) ? Optional.of(type.cast(t)) : Optional.empty());
+ }
+
+ /** Represents a non-paginated request. */
+ static PageToken readEverything() {
+ return PageTokenUtil.READ_EVERYTHING;
Review Comment:
`PageToken` provides a convenience getting for "read everything". `PageTokenUtil` groups several implementations.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Extensible pagination token implementation [polaris]
Posted by "snazy (via GitHub)" <gi...@apache.org>.
snazy commented on code in PR #1938:
URL: https://github.com/apache/polaris/pull/1938#discussion_r2212296209
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/pagination/PageTokenUtil.java:
##########
@@ -0,0 +1,267 @@
+/*
+ * 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 static com.google.common.base.Preconditions.checkArgument;
+import static java.lang.String.format;
+import static java.util.Collections.unmodifiableMap;
+
+import com.fasterxml.jackson.annotation.JsonTypeInfo;
+import com.fasterxml.jackson.core.JsonProcessingException;
+import com.fasterxml.jackson.databind.DatabindContext;
+import com.fasterxml.jackson.databind.JavaType;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.fasterxml.jackson.databind.jsontype.impl.TypeIdResolverBase;
+import com.fasterxml.jackson.dataformat.smile.databind.SmileMapper;
+import com.google.common.annotations.VisibleForTesting;
+import jakarta.annotation.Nullable;
+import java.io.IOException;
+import java.util.Base64;
+import java.util.HashMap;
+import java.util.Locale;
+import java.util.Map;
+import java.util.Optional;
+import java.util.OptionalInt;
+import java.util.ServiceLoader;
+
+final class PageTokenUtil {
+
+ private static final ObjectMapper SMILE_MAPPER = new SmileMapper().findAndRegisterModules();
Review Comment:
I don't think so. That code is for Polaris server, which doesn't have anything from Scala.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Extensible pagination token implementation [polaris]
Posted by "snazy (via GitHub)" <gi...@apache.org>.
snazy commented on PR #1938:
URL: https://github.com/apache/polaris/pull/1938#issuecomment-3007308929
This change enables pagination also for #1189, whereas #1555 does not.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Extensible pagination token implementation [polaris]
Posted by "eric-maynard (via GitHub)" <gi...@apache.org>.
eric-maynard commented on code in PR #1938:
URL: https://github.com/apache/polaris/pull/1938#discussion_r2167134069
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/dao/entity/EntitiesResult.java:
##########
@@ -48,41 +43,19 @@ public static EntitiesResult fromPage(Page<PolarisBaseEntity> page) {
public EntitiesResult(@Nonnull ReturnStatus errorStatus, @Nullable String extraInformation) {
super(errorStatus, extraInformation);
this.entities = null;
- this.pageTokenOpt = Optional.empty();
- }
-
- public EntitiesResult(@Nonnull List<PolarisBaseEntity> entities) {
- this(entities, Optional.empty());
}
/**
* Constructor for success
*
* @param entities list of entities being returned, implies success
*/
- public EntitiesResult(
- @Nonnull List<PolarisBaseEntity> entities, @Nonnull Optional<PageToken> pageTokenOpt) {
+ public EntitiesResult(@Nonnull Page<PolarisBaseEntity> entities) {
super(ReturnStatus.SUCCESS);
this.entities = entities;
- this.pageTokenOpt = pageTokenOpt;
- }
-
- @JsonCreator
- private EntitiesResult(
Review Comment:
If we can avoid it, I think it's better not to change this. This will be a breaking change for any persistence implementation depending on wire-compatibility of the result.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Extensible pagination token implementation [polaris]
Posted by "eric-maynard (via GitHub)" <gi...@apache.org>.
eric-maynard commented on code in PR #1938:
URL: https://github.com/apache/polaris/pull/1938#discussion_r2167141665
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/pagination/PageToken.java:
##########
@@ -18,82 +18,75 @@
*/
package org.apache.polaris.core.persistence.pagination;
-import java.util.List;
-import java.util.Objects;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.fasterxml.jackson.databind.annotation.JsonDeserialize;
+import com.fasterxml.jackson.databind.annotation.JsonSerialize;
+import jakarta.annotation.Nullable;
+import java.util.Optional;
+import java.util.OptionalInt;
+import org.apache.polaris.immutables.PolarisImmutable;
-/**
- * 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);
- }
+/** A wrapper for pagination information passed in as part of a request. */
+@PolarisImmutable
+@JsonSerialize(as = ImmutablePageToken.class)
+@JsonDeserialize(as = ImmutablePageToken.class)
+public interface PageToken {
+ // Serialization property names are intentionally short to reduce the size of the serialized
+ // paging token.
- /** 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);
- }
+ /** The requested page size (optional). */
+ @JsonProperty("p")
Review Comment:
ditto the comment -- this is not a good property name.
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/pagination/PageToken.java:
##########
@@ -18,82 +18,75 @@
*/
package org.apache.polaris.core.persistence.pagination;
-import java.util.List;
-import java.util.Objects;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.fasterxml.jackson.databind.annotation.JsonDeserialize;
+import com.fasterxml.jackson.databind.annotation.JsonSerialize;
+import jakarta.annotation.Nullable;
+import java.util.Optional;
+import java.util.OptionalInt;
+import org.apache.polaris.immutables.PolarisImmutable;
-/**
- * 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);
- }
+/** A wrapper for pagination information passed in as part of a request. */
+@PolarisImmutable
+@JsonSerialize(as = ImmutablePageToken.class)
+@JsonDeserialize(as = ImmutablePageToken.class)
+public interface PageToken {
+ // Serialization property names are intentionally short to reduce the size of the serialized
+ // paging token.
- /** 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);
- }
+ /** The requested page size (optional). */
+ @JsonProperty("p")
Review Comment:
ditto the comment on `i` -- this is not a good property name.
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/pagination/PageToken.java:
##########
@@ -18,82 +18,75 @@
*/
package org.apache.polaris.core.persistence.pagination;
-import java.util.List;
-import java.util.Objects;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.fasterxml.jackson.databind.annotation.JsonDeserialize;
+import com.fasterxml.jackson.databind.annotation.JsonSerialize;
+import jakarta.annotation.Nullable;
+import java.util.Optional;
+import java.util.OptionalInt;
+import org.apache.polaris.immutables.PolarisImmutable;
-/**
- * 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);
- }
+/** A wrapper for pagination information passed in as part of a request. */
+@PolarisImmutable
+@JsonSerialize(as = ImmutablePageToken.class)
+@JsonDeserialize(as = ImmutablePageToken.class)
+public interface PageToken {
+ // Serialization property names are intentionally short to reduce the size of the serialized
+ // paging token.
- /** 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);
- }
+ /** The requested page size (optional). */
+ @JsonProperty("p")
+ OptionalInt 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 LimitPageToken(pageSize);
- } else {
- return new ReadEverythingPageToken();
- }
- } else {
- // TODO implement, split out by the token's prefix
- throw new IllegalArgumentException("Unrecognized page token: " + token);
- }
+ /** Convenience for {@code pageSize().isPresent()}. */
+ default boolean paginationRequested() {
+ return pageSize().isPresent();
}
- /** Serialize a {@link PageToken} into a string */
- public abstract String toTokenString();
-
/**
- * Builds a new page token to reflect new data that's been read. If the amount of data read is
- * less than the pageSize, this will return a {@link DonePageToken}
+ * Paging token value, if present. Serialized paging tokens always have a value, but "synthetic"
+ * paging tokens like {@link #readEverything()} or {@link #fromLimit(int)} do not have a token
+ * value.
*/
- protected abstract PageToken updated(List<?> newData);
+ @JsonProperty("v")
Review Comment:
ditto
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Extensible pagination token implementation [polaris]
Posted by "eric-maynard (via GitHub)" <gi...@apache.org>.
eric-maynard commented on code in PR #1938:
URL: https://github.com/apache/polaris/pull/1938#discussion_r2167148664
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/transactional/TreeMapTransactionalPersistenceImpl.java:
##########
@@ -361,13 +362,23 @@ public List<EntityNameLookupRecord> lookupEntityActiveBatchInCurrentTxn(
.map(
nameRecord ->
this.lookupEntityInCurrentTxn(
- callCtx, catalogId, nameRecord.getId(), entityType.getCode()))
- .filter(entityFilter);
- if (pageToken instanceof HasPageSize) {
- data = data.limit(((HasPageSize) pageToken).getPageSize());
- }
+ callCtx, catalogId, nameRecord.getId(), entityType.getCode()));
+
+ Predicate<PolarisBaseEntity> tokenFilter =
+ pageToken
+ .valueAs(EntityIdToken.class)
+ .map(
+ entityIdToken -> {
+ var nextId = entityIdToken.entityId();
+ return (Predicate<PolarisBaseEntity>) e -> e.getId() > nextId;
+ })
+ .orElse(e -> true);
+
+ data = data.sorted(Comparator.comparingLong(PolarisEntityCore::getId)).filter(tokenFilter);
+
+ data = data.filter(entityFilter);
Review Comment:
Why break this into two calls?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Extensible pagination token implementation [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #1938:
URL: https://github.com/apache/polaris/pull/1938#discussion_r2168018610
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/pagination/Page.java:
##########
@@ -18,25 +18,99 @@
*/
package org.apache.polaris.core.persistence.pagination;
+import static java.util.Spliterators.iterator;
+
+import jakarta.annotation.Nullable;
+import java.util.ArrayList;
+import java.util.Iterator;
import java.util.List;
+import java.util.function.Function;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
/**
- * An immutable page of items plus their paging cursor. The {@link PageToken} here can be used to
- * continue the listing operation that generated the `items`.
+ * An immutable page of items plus the next-page token value, if there are more items. The {@link
+ * #encodedResponseToken()} here can be used to continue the listing operation that generated the
+ * `items`.
*/
public class Page<T> {
- public final PageToken pageToken;
- public final List<T> items;
+ private final PageToken request;
+ private final List<T> items;
+ @Nullable private final Token nextToken;
- public Page(PageToken pageToken, List<T> items) {
- this.pageToken = pageToken;
+ private Page(PageToken request, @Nullable Token nextToken, List<T> items) {
+ this.request = request;
+ this.nextToken = nextToken;
this.items = items;
}
/**
- * Used to wrap a {@link List<T>} of items into a {@link Page <T>} when there are no more pages
+ * Builds a complete response page for the full list of relevant items. No subsequence pages of
+ * related data exist.
*/
public static <T> Page<T> fromItems(List<T> items) {
- return new Page<>(new DonePageToken(), items);
+ return new Page<>(PageToken.readEverything(), null, items);
+ }
+
+ /**
+ * Produces a response page by consuming the number of items from the provided stream according to
+ * the {@code request} parameter. Source items can be converted to a different type by providing a
+ * {@code mapper} function. The page token for the response will be produced from the request data
+ * combined with the pointer to the next page of data provided by the {@code dataPointer}
+ * function.
+ *
+ * @param request defines pagination parameters that were uses to produce this page of data.
+ * @param items stream of source data
+ * @param mapper converter from source data types to response data types.
+ * @param tokenBuilder determines the {@link Token} used to start the next page of data given the
+ * last item from the previous page. The output of this function will be available from {@link
+ * PageToken#value()} associated with the request for the next page.
+ */
+ public static <R, T> Page<R> mapped(
+ PageToken request, Stream<T> items, Function<T, R> mapper, Function<T, Token> tokenBuilder) {
+ List<R> data;
+ T last = null;
+ if (!request.paginationRequested()) {
+ // short-cut for "no pagination"
+ data = items.map(mapper).collect(Collectors.toList());
+ } else {
+ data = new ArrayList<>(request.pageSize().orElse(10));
+
+ Iterator<T> it = iterator(items.spliterator());
+ int limit = request.pageSize().orElse(Integer.MAX_VALUE);
+ while (it.hasNext() && data.size() < limit) {
+ last = it.next();
+ data.add(mapper.apply(last));
+ }
+
+ // Signal "no more data" if the number of items is less than the requested page size or if
+ // there is no more data.
+ if (data.size() < limit || !it.hasNext()) {
Review Comment:
Good point! I wonder why it does not break any tests, though :thinking:
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Extensible pagination token implementation [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #1938:
URL: https://github.com/apache/polaris/pull/1938#discussion_r2168015025
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/dao/entity/ListEntitiesResult.java:
##########
@@ -46,43 +41,26 @@ public static ListEntitiesResult fromPage(Page<EntityNameLookupRecord> page) {
* @param errorCode error code, cannot be SUCCESS
* @param extraInformation extra information
*/
- public ListEntitiesResult(
- @Nonnull ReturnStatus errorCode,
- @Nullable String extraInformation,
- @Nonnull Optional<PageToken> pageTokenOpt) {
+ public ListEntitiesResult(@Nonnull ReturnStatus errorCode, @Nullable String extraInformation) {
super(errorCode, extraInformation);
this.entities = null;
- this.pageTokenOpt = pageTokenOpt;
}
/**
* Constructor for success
*
* @param entities list of entities being returned, implies success
*/
- public ListEntitiesResult(
- @Nonnull List<EntityNameLookupRecord> entities, @Nonnull Optional<PageToken> pageTokenOpt) {
+ public ListEntitiesResult(Page<EntityNameLookupRecord> entities) {
super(ReturnStatus.SUCCESS);
this.entities = entities;
- this.pageTokenOpt = pageTokenOpt;
}
- @JsonCreator
- private ListEntitiesResult(
Review Comment:
@eric-maynard : It's good that you're able to identify this change as a regression, but what if you're not available ? :) WDYT about encoding this expectation in a unit test?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org