Extensible pagination token implementation #1938

snazy · 2025-06-25T10:30:03Z

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.

snazy · 2025-06-25T16:01:30Z

This approach also works for NoSQL #1189 (last commit in this branch)

eric-maynard · 2025-06-25T16:25:18Z

polaris-core/src/main/java/org/apache/polaris/core/persistence/dao/entity/EntitiesResult.java

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.

cc @dennishuo for visibility as well

It is a private constructor... @eric-maynard @dennishuo : could you give some more details about possible compatibility issues? What's the affected workflow?

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.

I do not see a "wire" either.
BTW: The same change wasn't considered an issue in 1838.

eric-maynard · 2025-06-25T16:25:59Z

...is-core/src/main/java/org/apache/polaris/core/persistence/dao/entity/ListEntitiesResult.java

ditto the comment on wire-compatibility with previous versions

@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?

eric-maynard · 2025-06-25T16:26:18Z

polaris-core/src/main/java/org/apache/polaris/core/persistence/pagination/EntityIdToken.java

Why would we use the property name i?

I believe short names are meant to reduce the side of the token on the wire.

Yes, as explained in Token

eric-maynard · 2025-06-25T16:26:32Z

polaris-core/src/main/java/org/apache/polaris/core/persistence/pagination/EntityIdToken.java

The reason is explained as doc in Token

eric-maynard · 2025-06-25T16:27:10Z

polaris-core/src/main/java/org/apache/polaris/core/persistence/pagination/EntityIdToken.java

Is this meant to be short for EntityId? Why not use an enum?

That contradicts the intent of being extensible.

eric-maynard · 2025-06-25T16:28:23Z

polaris-core/src/main/java/org/apache/polaris/core/persistence/pagination/Page.java

It should be expected that the iterator has no more items if the limit has been pushed down into whatever provided the items, no?

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.

There is a test for this - and it passes

I wonder whether the test passes because limits are not pushed down to Persistence queries anywhere 🤔

eric-maynard · 2025-06-25T16:28:54Z

polaris-core/src/main/java/org/apache/polaris/core/persistence/pagination/Page.java

Same comments from the previous PR -- encoding/decoding a given PageToken clearly seems to be within the purview of a given PageToken implementation

There is only one PageToken.

eric-maynard · 2025-06-25T16:29:09Z

polaris-core/src/main/java/org/apache/polaris/core/persistence/pagination/PageToken.java

Would rather avoid these if possible

eric-maynard · 2025-06-25T16:29:19Z

polaris-core/src/main/java/org/apache/polaris/core/persistence/pagination/PageToken.java

ditto the comment on i -- this is not a good property name.

eric-maynard · 2025-06-25T16:29:33Z

polaris-core/src/main/java/org/apache/polaris/core/persistence/pagination/PageToken.java

eric-maynard · 2025-06-25T16:29:57Z

polaris-core/src/main/java/org/apache/polaris/core/persistence/pagination/PageToken.java

What is value? This seems a bit abstract.

My take: value is the implementation-specific token data (e.g. the entity ID in sorted SQL result sets).

eric-maynard · 2025-06-25T16:30:17Z

polaris-core/src/main/java/org/apache/polaris/core/persistence/pagination/PageToken.java

It's possible per the spec to request pagination by providing a page token without page size. This logic is not correct.

The logic looks correct to me. Propagating page size from the previous token in handled in PageTokenUtil.decodePageRequest()

eric-maynard · 2025-06-25T16:30:46Z

polaris-core/src/main/java/org/apache/polaris/core/persistence/pagination/PageToken.java

Is READ_EVERYTHING a Util? Seems like it would belong in PageToken

PageToken provides a convenience getting for "read everything". PageTokenUtil groups several implementations.

eric-maynard · 2025-06-25T16:32:10Z

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.

eric-maynard · 2025-06-25T16:33:28Z

...a/org/apache/polaris/core/persistence/transactional/TreeMapTransactionalPersistenceImpl.java

Why break this into two calls?

(I think it came from my PR) No particular reason. Would you prefer a chained call?

dimas-b

Changes LGTM 👍

dimas-b · 2025-06-26T03:09:08Z

...t/resources/META-INF/services/org.apache.polaris.core.persistence.pagination.Token$TokenType

Token$TokenType looks awkward as a file name ($ and double Token)... WDYT about making TokenType a top-level class?

It's the class name.
I'd really prefer keeping these very related things (Token + TokenType) together - you have to implement both.

polaris-core/src/main/java/org/apache/polaris/core/persistence/pagination/PageToken.java

snazy · 2025-06-26T06:34:34Z

This change enables pagination also for #1189, whereas #1555 does not.

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 <[email protected]> Co-authored-by: Eric Maynard <[email protected]>

snazy · 2025-07-16T05:37:09Z

Thanks all!

eric-maynard · 2025-07-16T17:03:20Z

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 reverts commit fb418a2.

eric-maynard · 2025-07-16T18:57:21Z

polaris-core/src/main/java/org/apache/polaris/core/persistence/pagination/PageTokenUtil.java

+
+final class PageTokenUtil {
+
+  private static final ObjectMapper SMILE_MAPPER = new SmileMapper().findAndRegisterModules();


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")

I don't think so. That code is for Polaris server, which doesn't have anything from Scala.

Well, it might, depending on the extensions you have. From what I can tell this actually functions without findAndRegisterModules -- why do we need it?

The enforcement of the LIST_PAGINATION_ENABLED flag was missed in apache#1938. This change make the flag effective as discussed in apache#2296. Note: this causes a change in the default Polaris behaviour (no pagination by default) with respect to the previous state of `main`. However, there is no behaviour change with respect to 1.0.0 or 1.0.1 as previous releases did not have apache#1938.

The enforcement of the LIST_PAGINATION_ENABLED flag was missed in #1938. This change make the flag effective as discussed in #2296. Note: this causes a change in the default Polaris behaviour (no pagination by default) with respect to the previous state of `main`. However, there is no behaviour change with respect to 1.0.0 or 1.0.1 as previous releases did not have #1938.

The enforcement of the LIST_PAGINATION_ENABLED flag was missed in apache#1938. This change make the flag effective as discussed in apache#2296. Note: this causes a change in the default Polaris behaviour (no pagination by default) with respect to the previous state of `main`. However, there is no behaviour change with respect to 1.0.0 or 1.0.1 as previous releases did not have apache#1938.

* feat: enforce LIST_PAGINATION_ENABLED The enforcement of the LIST_PAGINATION_ENABLED flag was missed in #1938. This change make the flag effective as discussed in #2296. Note: this causes a change in the default Polaris behaviour (no pagination by default) with respect to the previous state of `main`. However, there is no behaviour change with respect to 1.0.0 or 1.0.1 as previous releases did not have #1938.

github-project-automation bot added this to Basic Kanban Board Jun 25, 2025

github-project-automation bot moved this to PRs In Progress in Basic Kanban Board Jun 25, 2025

snazy force-pushed the pagination-alt-flex branch from 12ab744 to 1b42120 Compare June 25, 2025 10:45

snazy requested a review from dimas-b June 25, 2025 13:16

dimas-b requested a review from eric-maynard June 25, 2025 16:25

eric-maynard reviewed Jun 25, 2025

View reviewed changes

polaris-core/src/main/java/org/apache/polaris/core/persistence/pagination/PageToken.java Outdated

Copy link

Contributor

eric-maynard Jun 25, 2025

Choose a reason for hiding this comment

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

ditto

eric-maynard reviewed Jun 25, 2025

View reviewed changes

dimas-b reviewed Jun 26, 2025

View reviewed changes

polaris-core/src/main/java/org/apache/polaris/core/persistence/pagination/PageToken.java Outdated Show resolved Hide resolved

snazy force-pushed the pagination-alt-flex branch 3 times, most recently from 191d99d to 6cdb73c Compare July 1, 2025 04:56

snazy force-pushed the pagination-alt-flex branch 3 times, most recently from d072060 to 88ae32d Compare July 7, 2025 08:49

snazy force-pushed the pagination-alt-flex branch 5 times, most recently from 09b4b76 to 5ff8b7c Compare July 14, 2025 19:26

snazy force-pushed the pagination-alt-flex branch from 5ff8b7c to b67b787 Compare July 15, 2025 15:55

dimas-b approved these changes Jul 15, 2025

View reviewed changes

github-project-automation bot moved this from PRs In Progress to Ready to merge in Basic Kanban Board Jul 15, 2025

snazy merged commit fb418a2 into apache:main Jul 16, 2025
12 checks passed

snazy deleted the pagination-alt-flex branch July 16, 2025 05:35

github-project-automation bot moved this from Ready to merge to Done in Basic Kanban Board Jul 16, 2025

eric-maynard added a commit that referenced this pull request Jul 16, 2025

Revert "Extensible pagination token implementation (#1938)"

276e418

This reverts commit fb418a2.

eric-maynard mentioned this pull request Jul 16, 2025

Revert "Extensible pagination token implementation" #2123

Closed

eric-maynard reviewed Jul 16, 2025

View reviewed changes

eric-maynard mentioned this pull request Jul 22, 2025

Persistence implementation for pagination in some requests #1838

Closed

eric-maynard mentioned this pull request Aug 7, 2025

Remove unused LIST_PAGINATION_ENABLED feature configuration #2296

Closed

dennishuo mentioned this pull request Aug 12, 2025

Remove overloads of BasePersistence.listEntities #2262

Closed

dimas-b mentioned this pull request Aug 19, 2025

feat: enforce LIST_PAGINATION_ENABLED #2401

Merged


		final class PageTokenUtil {

		private static final ObjectMapper SMILE_MAPPER = new SmileMapper().findAndRegisterModules();

Extensible pagination token implementation #1938

Extensible pagination token implementation #1938

Uh oh!

Conversation

snazy commented Jun 25, 2025

Uh oh!

snazy commented Jun 25, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dimas-b Jun 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dimas-b Jun 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eric-maynard Jun 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eric-maynard commented Jun 25, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dimas-b left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

dimas-b Jun 26, 2025 •

edited

Loading

dimas-b Jun 26, 2025 •

edited

Loading

eric-maynard Jun 25, 2025 •

edited

Loading

dimas-b left a comment •

edited

Loading