Interface changes for pagination #1528

eric-maynard · 2025-05-05T17:19:29Z

The Apache Iceberg REST catalog spec describes how APIs like listNamespaces can implement pagination, but Polaris currently does not implement pagination.

This PR introduces a framework that will allow us to enable pagination for these APIs and that can also be extended to other APIs like listCatalogs.

This is a modified version of #273 that removes the actual pagination implementation at the persistence level.

…isGenericTableCatalog-2

eric-maynard · 2025-05-05T18:18:17Z

...ore/src/main/java/org/apache/polaris/core/persistence/pagination/ReadFromStartPageToken.java

+
+/**
+ * This is needed until real pagination is implemented just to capture limit / page-size without any
+ * particular start position TODO: Remove this type


This entire type can get removed once the pageTokenBuilder implementations are updated to return a more useful PageToken implementation.

It's needed for now because we are updating interfaces to remove int limit in lieu of a PageToken, but ReadEverythingPageToken (which will stay for testing and for calls that want no pagination) doesn't support a limit.

eric-maynard · 2025-05-05T18:22:44Z

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

+ * 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 {


Reviving the discussion on the name below...

It's currently called PageToken because it encapsulates the data in page-token sent by the client. It also encapsulates page-size and so there has been some objection to the naming.

At its core, this PageToken is used to describe how persistence should build a page of data. It is, by itself, not a sufficient "page request" in that it doesn't contain information like the type of entity to search for. But it is used in conjunction to with a request to filter results down.

Because the Iceberg spec is vague about the page-token implementation -- calling it "opaque" -- I feel that a page token which contains information about the size of the page is still consistent with the spec. page-size is still respected when it's present in a request alongside page-token.

To be clear, it's not just the Java type that contains the page size. The string page-token itself contains the page size information and the Java type is reflecting that.

...e/polaris/extension/persistence/impl/eclipselink/PolarisEclipseLinkMetaStoreSessionImpl.java

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

dimas-b

Some early feedback... I'll read/comment more presently

...re/src/main/java/org/apache/polaris/core/persistence/pagination/ReadEverythingPageToken.java

dimas-b · 2025-05-07T18:56:17Z

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

+      if (tokenString == null) {
+        throw new IllegalArgumentException("Cannot build page token from null string");
+      } else if (tokenString.isEmpty()) {
+        if (this instanceof ReadEverythingPageToken.ReadEverythingPageTokenBuilder) {
+          return ReadEverythingPageToken.get();


Is null that much different from an empty string?

Since the intention is to convey both the "page pointer" and the "page size" in PageToken, why have have a from(@Nullable String token, int pageSize) method?

It's a good question, let me briefly overview how it's intended to behave.

The user may provide a token, a page-size, both, or neither.

If they provide neither, there is no pagination.

If they provide just a page size, we start a new listing operation with the given page size

If they provide just a page token, we start from the location specified in the token, with the page size specified in the token.

If they provide both, we start from the location specified in the token, with the page size specified in the request.

So this method is useful for situations (2) and (4). Consider, too, the case where the user does (4), but then follows up with (3) -- I think they would expect the same page size to continue to apply in the second call, which is why we stick the page size in the token itself.

Also, good point re: null

For 1 I'd say we should use some server default page size for backends that support pagination. It may be large, but not unlimited.

For backends that do not support pagination, unlimited result set is ok.

From a more general perspective, I believe the server cannot be expected to always comply with the client's page size.

To safeguard against DOS attacks

For technical reasons:

A persistence query may return less data than expected. It might be preferable to finish the request with a smaller page and let the client re-request, than keep polling the backend.

A communication protocol may have restrictions based on the message size, not the number of items encoded in the message.

All in all, I think clients should not expect responses to have exactly as many entries as requested even when the result set is not exhausted. Polaris may be able to satisfy that expectation in most cases, but we should not make it a guaranteed feature.

This is mostly governed by the Iceberg spec and our goal should be to comply with that.

What you wrote is true, that the server may return more or less records than the requested page size (whether embedded in the token or explicitly requested by the client). But we still need a way for the client to request a given page size as per the spec. The four scenarios outlined above are still valid and are still supported by the APIs defined here.

Specs are not infallible :)

Quoting from the REST API spec

Servers that support pagination must return all results in a single response with the value of `next-page-token` set to `null` if the query parameter `pageToken` is not set in the request.

IMHO, this requirement is a recipe for disaster if, for example, a catalog has millions of entries.

I would not want to force Polaris into following this statement by the letter.

but now I did notice that the REST API spec indicates that the presence of an empty PageToken should be interpreted as the client's intention to use pagination.

That statement in itself is fine. Given my comment above, I think it might be worth interpreting null and empty page tokens differently. I'd still suggest to use a possibly large, but limited page size for the null token. We could use a smaller default page size for the empty token.

For at least the first iteration, I want to take a hard line on complying with the spec. Per the quote you cited that means that if there's no page token, we return everything.

Remember that today we just return all results no matter what the user asks for. I think it's okay to keep returning all results if the user asks for it, but at least now we will give users the option to ask for something else. If nothing else it's an incremental improvement.

wrt. null vs empty string, I'm a bit paranoid about us accidentally paginating results when the user doesn't expect it. So there too I think it's better to err on the side of the current (and still spec-compliant) behavior of just returning everything.

Fair point about minimizing disturbance to existing Polaris behaviour in this PR.

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

polaris-core/src/main/java/org/apache/polaris/core/persistence/BasePersistence.java

...re/src/main/java/org/apache/polaris/core/persistence/pagination/ReadEverythingPageToken.java

dimas-b

Some more comments. I'm done with my first review.

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

service/common/build.gradle.kts

...e/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java

...mmon/src/main/java/org/apache/polaris/service/types/ListNamespacesResponseWithPageToken.java

eric-maynard · 2025-05-08T04:46:54Z

Thanks @dimas-b for the thorough review! Based on your advice above I've refactored things quite a bit and seemingly simplified the interfaces greatly. Several types are gone and the way that we build PageTokens is much simpler now. Let me know what you think

dimas-b · 2025-05-08T15:16:09Z

...e/polaris/extension/persistence/impl/eclipselink/PolarisEclipseLinkMetaStoreSessionImpl.java

+            .map(ModelEntity::toEntity)
+            .filter(entityFilter);
+
+    if (pageToken instanceof HasPageSize hasPageSize) {


I'm not sure this is functionally correct. If the token is a sub-class of HasPageSize but has other information that the client provided, but EclipseLink does not recognise, I do not think it would be correct to just use the page size from the token.

I believe we should check for a specific type here and fail on all types, whose details cannot be fully processed.

This is correct for the current code, where HasPageSize is just used as a limit. When page tokens are implemented this logic will definitely change!

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

...ore/src/main/java/org/apache/polaris/core/persistence/pagination/ReadFromStartPageToken.java

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

...ore/src/main/java/org/apache/polaris/core/persistence/pagination/ReadFromStartPageToken.java

service/common/build.gradle.kts

dimas-b · 2025-05-08T16:37:23Z

...e/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java

+      return CatalogHandlers.listNamespaces(
+          namespaceCatalog, parent, pageToken, String.valueOf(pageSize));


I believe we can do better in Polaris by using the REST API of baseCatalog directly.

This can be deferred.

It may not be a REST catalog :)

Either way, I think we have to delegate pagination to the federated catalog if it supports pagination. CatalogHandlers.listNamespaces has a trivial implementation that is sub-optimal when the catalog is a REST Catalog.

ah, you're right. In most cases we should stop using CatalogHandlers methods as a fallback during federation or at least add our own logic, I think. Probably we can do this as a followup

Followup SGTM

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

dimas-b

LGTM 👍 Thanks, @eric-maynard !

* Doc: Add release guide on the website (apache#1539) * main: Update actions/stale digest to f78de97 (apache#1547) * main: Update dependency boto3 to v1.38.12 (apache#1548) * main: Update postgres Docker tag to v17.5 (apache#1549) * main: Update dependency com.adobe.testing:s3mock-testcontainers to v4.2.0 (apache#1551) * main: Update dependency com.nimbusds:nimbus-jose-jwt to v10.3 (apache#1552) * Interface changes for pagination (apache#1528) * add missing apis * more tests, fixes * clean up drop * autolint * changes per review * revert iceberg messages to comply with oss tests * another revert * more iceberg catalog changes * autolint * dependency issues * more wiring * continuing rebase * remaining issues are related to task loading * re-add tests * debugging * fix failing tests * fix another test * changes per review * autolint * some fixes * stable * updates for new persistence * fix * continuing work * more reverts * continue reverts * more reverts * yank tests * autolint * test reverts * try to support limit without real page tokens * autolint * Stable * change comment * autolint * remove catalog config for now * changes per review * more tweaks * simplify types per review * Stable, about to refactor more * re-stable * polish * autolint * more changes per review * stable * Introduce reserved-properties setting; reserve "polaris." by default (apache#1417) * initial commit * initial commit * try to test * quarkus fixes * fix a bunch of callsites * Start applying changes * autolint * chase todos * autolint * bugfix * stable * add one test * stable with more tests * autolint * more tests * autolint * stable tests * clean up * oops * stabilize on main * autolint * more changes per review * Add cleanup support for partition-level statistics files when `DROP TABLE PURGE` (apache#1508) * cleaning up partition stats * update partition stat file extension * update test partition stat write impl * Implement federation to HadoopCatalog (apache#1466) * wip * quarkus fixes * autolint * hadoop impl * autolint * Refactors * refactored * autolint * add config * autolint * stable * Remove breakpoint anchor * add line to application.properties * yank HADOOP * autolint * Spark: Use builder for CreateGenericTableRequest instead of constructor for easier API spec update (apache#1546) * main: Update docker.io/jaegertracing/all-in-one Docker tag to v1.69.0 (apache#1559) * main: Update dependency io.opentelemetry:opentelemetry-bom to v1.50.0 (apache#1558) * main: Update dependency software.amazon.awssdk:bom to v2.31.40 (apache#1567) * main: Update dependency boto3 to v1.38.13 (apache#1556) * Include DISCLAIMER in binary distributions (apache#1568) * NoSQL: merge/rebase-fixes * NoSQL: bump dependencies * fix markdown --------- Co-authored-by: JB Onofré <jbonofre@apache.org> Co-authored-by: Mend Renovate <bot@renovateapp.com> Co-authored-by: Eric Maynard <eric.maynard+oss@snowflake.com> Co-authored-by: danielhumanmod <danieltu.life@gmail.com> Co-authored-by: gh-yzou <167037035+gh-yzou@users.noreply.github.com>

eric-maynard added 30 commits March 20, 2025 13:18

add missing apis

4f775a6

more tests, fixes

e2fcd5e

clean up drop

467ebcb

autolint

5f5ca42

Merge branch 'main' of github.com:apache/polaris into implement-Polar…

c1b2b73

…isGenericTableCatalog-2

changes per review

34c8e96

revert iceberg messages to comply with oss tests

dab7279

another revert

54a47a5

more iceberg catalog changes

1ffbf07

autolint

113c941

dependency issues

b672986

more wiring

4a9ed69

continuing rebase

6d7c59b

remaining issues are related to task loading

5c969d7

re-add tests

e67b0e9

debugging

663dafb

resolve conflicts

90b4c73

fix failing tests

133b658

fix another test

98661be

changes per review

0798b63

autolint

241e7ed

some fixes

0ad4ced

rebase

fcaa2b7

stable

dc86441

rebase

4b58278

pull main

6050494

updates for new persistence

0fcec2f

fix

6b9031c

merge conflicts

01a8468

continuing work

81a7bf0

eric-maynard requested a review from HonahX as a code owner May 5, 2025 17:19

eric-maynard mentioned this pull request May 5, 2025

Implement pagination for list APIs #273

Closed

5 tasks

eric-maynard added 3 commits May 5, 2025 10:24

change comment

5c91235

autolint

e0911eb

remove catalog config for now

dde29d0

eric-maynard commented May 5, 2025

View reviewed changes

merge conflicts

b87bc63

eric-maynard requested a review from pingtimeout as a code owner May 7, 2025 17:36

dimas-b reviewed May 7, 2025

View reviewed changes

...e/polaris/extension/persistence/impl/eclipselink/PolarisEclipseLinkMetaStoreSessionImpl.java Outdated Show resolved Hide resolved

polaris-core/src/main/java/org/apache/polaris/core/persistence/dao/entity/EntitiesResult.java Outdated Show resolved Hide resolved

dimas-b reviewed May 7, 2025

View reviewed changes

eric-maynard added 2 commits May 7, 2025 12:32

changes per review

0d4bd9c

more tweaks

17dba07

dimas-b reviewed May 7, 2025

View reviewed changes

eric-maynard added 3 commits May 7, 2025 18:12

simplify types per review

a159f23

Stable, about to refactor more

941b34c

re-stable

8ec7d7c

eric-maynard added 2 commits May 7, 2025 21:56

polish

806454c

autolint

442341f

dimas-b reviewed May 8, 2025

View reviewed changes

eric-maynard added 2 commits May 8, 2025 10:40

more changes per review

9b6c591

stable

e578809

dimas-b approved these changes May 8, 2025

View reviewed changes

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

eric-maynard merged commit 44064cb into apache:main May 9, 2025
6 checks passed

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

This was referenced May 9, 2025

Persistence implementations for list pagination #1555

Open

Eliminate usage of CatalogHandlers; introduce CatalogHandlerUtils #1576

Merged

		return CatalogHandlers.listNamespaces(
		namespaceCatalog, parent, pageToken, String.valueOf(pageSize));

Interface changes for pagination #1528

Interface changes for pagination #1528

Uh oh!

Conversation

eric-maynard commented May 5, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eric-maynard May 5, 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!

Uh oh!

Uh oh!

dimas-b left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eric-maynard May 7, 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!

dimas-b May 7, 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!

dimas-b May 7, 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!

eric-maynard May 7, 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!

eric-maynard May 8, 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!

Uh oh!

Uh oh!

Uh oh!

dimas-b left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

eric-maynard commented May 8, 2025

Uh oh!

dimas-b May 8, 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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 May 8, 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

eric-maynard May 5, 2025 •

edited

Loading

eric-maynard May 7, 2025 •

edited

Loading

dimas-b May 7, 2025 •

edited

Loading

dimas-b May 7, 2025 •

edited

Loading

eric-maynard May 7, 2025 •

edited

Loading

eric-maynard May 8, 2025 •

edited

Loading

dimas-b May 8, 2025 •

edited

Loading

eric-maynard May 8, 2025 •

edited

Loading