Skip to content

Persistence implementation for pagination in some requests #1838

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

dimas-b
Copy link
Contributor

@dimas-b dimas-b commented Jun 9, 2025

Following up on #1555

  • 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. EntityIdPaging handles
    pagination over ordered result sets with static helper methods.

Copy link
Member

@snazy snazy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall the approach LGTM.

Some places however could be simplified and clarified though.

* @param requestedPageSize optional page size for the next page. If not set, the page size of the
* previus page (encoded in the previous page token) will be reused.
*/
public static PageRequest nextPage(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function name and phrase 'from the previous API-level...are a bit confusing: what's "next"? What's "previous"? Why's there no "current"? All it does is constructing aPageRequest` from the two pagination-parameters.

Why not call this function constructFromParameters?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

refactored to PageToken.decode()... Is it better?

}

/** Represents a request to start paginating with a particular page size. */
public static PageRequest firstPage(int limit) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the general use case of this one? It's only used in some drop*() implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is now PageToken.fromLimit() (same name as before).

The use case is for handling initial paged requests where a size is provided, but a token in not. For API-based calls that is taken care of by the decode() method. This method is a convenience entry point for internal calls.

@dimas-b dimas-b changed the title Delineate pagination requests and tokens Persistence implementation for pagination in some requests Jun 10, 2025
@dimas-b
Copy link
Contributor Author

dimas-b commented Jun 10, 2025

Squashed, rebased, resolved conflicts and addressed some feedback. Please review again.

Following up on apache#1555

* 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. EntityIdPaging handles
  pagination over ordered result sets with static helper methods.

Co-authored-by: Eric Maynard <eric.maynard+oss@snowflake.com>
if (pageToken.paginationRequested()) {
hql += " order by m.id asc";

if (pageToken.hasDataReference()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is still polymorphism, we're just not taking advantage of the fact that this is built into the language already:

if (pageToken isinstanceof EntityIdPageToken e) {
  tokenId = e.getEntityId();
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Polymorphism at the java type level is not necessary in this case. Users of the "data pointer" know how to parse it based on the API method called. It is not a matter of providing a sub-type, but about understanding the format.

Currently parsing is achieved via static methods in EntityIdPaging.

* @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 dataPointer determines the internal pointer to the next page of data given the last item
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what this is or why we're calling it a "data pointer". it looks like a function that converts a list of elements into a page token, except for some reason we're not using the Java type PageToken and we're going directly to the serialized representation of a PageToken

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The java type PageToken is a container for a tuple of:

  1. Page size
  2. the "data pointer"

The latter is an opaque piece of data as far as the java type PageToken is concerned. "Data pointers" are interpreted only by the code that actually produces them.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need a Java type for what you're calling a "data pointer" -- for what the API calls page-token. In #1555, I called this PageToken. If we need to wrap this PageToken in a PageRequest, that's okay.

public record PageToken() {
  public void getEncodedString() { ... }
  public static PageToken fromEncodedString() { ... }
}

public record PageRequest(Optional<PageToken> pageToken, Optional<Integer> pageSize) {}

Copy link
Contributor Author

@dimas-b dimas-b Jun 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about keeping current PageToken class mostly as it is, but replacing its String encodedDataReference with DataPointer dataPointer?

It does make the code nicer by having a specific type, but IMHO it adds a difficulty from the processing perspective:

  • Parsing DataPointer to a specific sub-type when PageToken is constructed will require encoding type information in the token. That is redundant IMHO, but I'm ok with that change.
  • Persistence methods that need to "understand" a DataPointer will have to do type casts, which is not elegant, IMHO. Currently they do not have to, persistence implementations currently just call a known parse method, which then will raise exceptions if the data is malformed.

As a middle ground option WDYT about using DataPointer to wrap the encoded String (no sub-types), but still use static parsing methods to be called from Persistence?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think just wrapping the encoded string is an adequate Java representation of the page-token -- we should extract the information from that encoded string.

static PageToken decodePageRequest(
@Nullable String requestedPageToken, @Nullable Integer requestedPageSize) {
int pageSize;
String encodedDataReference = null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If a token has multiple components, should that be modeled as one "encoded data reference"? Multiple?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The interpretation of encodedDataReference is up to the code that constructs it for paged responses. Let's deal with complex cases when we have them in practice.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we're defining the serde/deserde of the page token now, let's do it in a way that captures future page tokens we think are reasonable. I think that includes page requests with more than 2 discrete pieces of information.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current approach does allow for future extensions. Different "data pointers" can be introduced when a Persistence impl. needs them. Since the "data pointer" always makes a round trip from Persistence code to the client and back to the same persistence code, that code is free to introduce new encoding/decoding methods without affecting anything else.

It is remotely possible that a page token ends up in an API method call different from the one that produced it. However, having "data pointer" sub-types (as opposed to just pairs of encode/decode methods) does not add any safety here. We could encode some "place" identifier into the token to bind it firmly to the API method, but I think it's an overkill.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this thread is just about not hard-coding the number of components in a page token right? Yes, we could add different encode/decode methods, but this one in PageTokenUtil looks like the canonical one in this design

return new PageToken(encodedDataReference, pageSize);
}

static @Nullable String encodePageToken(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this not just a member of PageToken?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To co-locate encoding and decoding code in one java file.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could co-locate them in PageToken, so I don't think that really answers the question

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

decodePageRequest is called from PageToken when the token is received, but encodePageToken is called fromPage when it is returned to the client. This class is the place to share code between PageToken and Page.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still don't understand why this encoding isn't an attribute of the PageToken. I think this may just be a side effect of us directly using the encoded string instead of a PageToken, as is discussed above.

* servicing the request for the next page of related data.
*/
public @Nullable String encodedResponseToken() {
return PageTokenUtil.encodePageToken(request, encodedDataReference);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, this looks like quite a thin method

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it does :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think it would be reasonable for callers to just call PageTokenUtil.encodePageToken directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Calling PageTokenUtil.encodePageToken(Page) from a place that has a reference to a Page is less elegant than calling Page.encodedResponseToken()... This is a matter of opinion, of course :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can make this change if you prefer. No worries.

Comment on lines +46 to +47
* Reconstructs a page token from the API-level page token string (returned to the client in the
* response to a previous request for similar data) and an API-level new requested page size.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does "API-level" mean here? When we get a table identifier in a method like IcebergCatalog.newTableOps, we don't say it's an API-level TableIdentifier just because it came in via the API.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"API-level" here means any round trip to a client (specifically the Iceberg REST Catalog API).

Do you have a suggestion for improving this javadoc text?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't it just "a page token string"?


/**
* Produces the reference to the next page of data in the form of a numerical entity ID. Entities
* in the associated stream of data are assumed to be ordered by ID.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What stream? This is just taking one entity and returning Long.toString(entity.getId()).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is in reference to how the document method is used in relation to handling paginated requests and responses (which deal with streams of data).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found this a little confusing, as the doc doesn't seem to match the actual behavior of the method

import org.apache.polaris.core.entity.PolarisBaseEntity;

/** Utility class for processing pagination of streams of entities ordered by some ID property. */
public final class EntityIdPaging {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure how useful this is going to be, since its methods are static there can't be another implementation. The methods both look very thin.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EntityIdPaging is a specific case of constructing and parsing PageToken.encodedDataReference() for Persistence implementations that rely of sorted streams of entities.

Having these static methods allows having the encoding/decoding code across several use cases in the current Polaris code.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, but I'm trying to understand what this will look like if we have SomeOtherEntityIdPaging. Will callers have some way of selecting a ...Paging based on the token structure or method?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If SomeOtherEntityIdPaging produces a single primitive value from a "data pointer" it would be very similar to EntityIdPaging, I guess.

If it produces a rich value, then it will need a companion type to hold the parsed data... but that is something to be done if and when we need it. It could be something like static NameAndIdPaging.entityBoundary(PageToken pageToken) returning a new NameAndIdPaging.Boundary(name, id).

Copy link
Contributor

@eric-maynard eric-maynard Jun 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but that is something to be done if and when we need it

We have an existing PR in #1555 that supports this use case, so I have some reservations about an alternative design that loses this functionality

snazy pushed a commit to snazy/polaris that referenced this pull request Jun 25, 2025
Based on apache#1838, following up on apache#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.
snazy pushed a commit to snazy/polaris that referenced this pull request Jun 25, 2025
Based on apache#1838, following up on apache#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.
snazy pushed a commit to snazy/polaris that referenced this pull request Jun 26, 2025
Based on apache#1838, following up on apache#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.
snazy pushed a commit to snazy/polaris that referenced this pull request Jun 27, 2025
Based on apache#1838, following up on apache#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.
snazy pushed a commit to snazy/polaris that referenced this pull request Jun 30, 2025
Based on apache#1838, following up on apache#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.
snazy pushed a commit to snazy/polaris that referenced this pull request Jul 1, 2025
Based on apache#1838, following up on apache#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.
snazy pushed a commit to snazy/polaris that referenced this pull request Jul 4, 2025
Based on apache#1838, following up on apache#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.
snazy pushed a commit to snazy/polaris that referenced this pull request Jul 4, 2025
Based on apache#1838, following up on apache#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.
snazy pushed a commit to snazy/polaris that referenced this pull request Jul 4, 2025
Based on apache#1838, following up on apache#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.
snazy pushed a commit to snazy/polaris that referenced this pull request Jul 7, 2025
Based on apache#1838, following up on apache#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.
snazy pushed a commit to snazy/polaris that referenced this pull request Jul 9, 2025
Based on apache#1838, following up on apache#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.
snazy pushed a commit to snazy/polaris that referenced this pull request Jul 11, 2025
Based on apache#1838, following up on apache#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.
snazy added a commit to snazy/polaris that referenced this pull request Jul 12, 2025
Based on apache#1838, following up on apache#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.

Co-authored-by: Dmitri Bourlatchkov <dmitri.bourlatchkov@gmail.com>
Co-authored-by: Eric Maynard <eric.maynard+oss@snowflake.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants