You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@polaris.apache.org by "ayushtkn (via GitHub)" <gi...@apache.org> on 2026/08/07 08:56:59 UTC
[PR] Fix JDBC listEntities fetching all remaining rows instead of the requested page [polaris]
ayushtkn opened a new pull request, #5255:
URL: https://github.com/apache/polaris/pull/5255
`JdbcBasePersistenceImpl.buildEntityQuery` switches to `keyset pagination` when the caller requests a page β ordering by `id` and adding `id > ?` from the page token β but never emits a matching `LIMIT`, so the statement asks the database for every remaining matching row and `Page.mapped` then keeps the first `pageSize` and discards the rest. Abandoning the stream early does not avoid the work: `executeSelectOverStream` never sets a fetch size and this path borrows a connection with autoCommit left on, so the PostgreSQL driver has no server-side cursor and materialises the entire result set client-side before the first row is read.
Concretely, listing a namespace of `1,000` tables with a page size of `100` fetches all `1,000` rows and returns `100`, then fetches the remaining `900` and returns `100`, then `800`, and so on down to the final page β `5,500` rows transferred and materialised to return `1,000`, with the cost of every page paid again on the next one.
That makes paginating a full listing quadratic rather than bounding it, so the effect grows sharply with size: the same walk over `100k` entities moves on the order of `50M` rows, and the more carefully a client pages the worse it gets.
This change passes `pageSize + 1` through as a SQL `LIMIT` whenever pagination is requested; the `+1` is what lets `Page.mapped` still see that a further row exists and emit a continuation token, and without it a final page whose size exactly divides the total would look like the end of the data and silently truncate the listing. `QueryGenerator's` internal query builder already renders both `ORDER BY` and `LIMIT`, so this only adds one public overload that carries the limit through alongside the existing ordering and keyset arguments, leaving every other call site unchanged.
## Checklist
- [ ] π‘οΈ Don't disclose security issues! (contact security@apache.org)
- [ ] π Clearly explained why the changes are needed, or linked related issues: Fixes #
- [ ] π§ͺ Added/updated tests with good coverage, or manually tested (and explained how)
- [ ] π‘ Added comments for complex logic
- [ ] π§Ύ Updated `CHANGELOG.md` (if needed)
- [ ] π Updated documentation in `site/content/in-dev/unreleased` (if 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] Fix JDBC listEntities fetching all remaining rows instead of the requested page [polaris]
Posted by "ayushtkn (via GitHub)" <gi...@apache.org>.
ayushtkn commented on code in PR #5255:
URL: https://github.com/apache/polaris/pull/5255#discussion_r3734702067
##########
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcBasePersistenceImpl.java:
##########
@@ -635,7 +649,8 @@ public <T> Page<T> listFullEntities(
entityType,
entitySubType,
pageToken,
- ModelEntity.getAllColumnNames(schemaVersion));
+ ModelEntity.getAllColumnNames(schemaVersion),
+ false);
Review Comment:
TLDR: It is same as before & intentional as mentioned in the comment as well. I have updated the description line which triggered this, but the PR title already mentioned `listEntities`
I too saw this and my plan around this, I will have a new PR around it:
`listFullEntities` applies `entityFilter` as a Java Predicate after the rows come back, so a SQL `LIMIT` there is not equivalent to one on `listEntities`. If the bound cuts the result set and the predicate then rejects some of what survived, the page comes back under-filled, `Page.mapped` sees `data.size() < limit`, sets `last = null`, and no continuation token is emitted β the client stops early with rows still unread. That is silent truncation rather than a slow query, and it's the same failure mode the new paginatedListEntitiesWalksEveryPageWhenTotalIsAMultipleOfPageSize test guards against on the listEntities side. Since the predicate is arbitrary Java, there's no way for `buildEntityQuery` to know from the inside whether bounding is safe, so `false` is the only correct default. Note this also means the PR doesn't change `listFullEntities` behaviour at all β it was unbounded before and stays unbounded; the flag just makes the existing behaviour explicit at the call site.
On the suggested fast-path for entity -> true: I looked at the call sites and it's narrower than it appears. `listFullEntitiesAll` passes `PageToken.readEverything()`, so it never requests pagination and is unaffected. `loadTasks` passes a genuinely selective predicate, and has no production caller today.
That leaves exactly one production caller that reaches this path with a page size β the catalog-role count in `AtomicOperationMetaStoreManager.dropEntity`, which uses `PageToken.fromLimit(2)` with an unconditional predicate.
I'd rather fix that one at the caller than add a "trust me, my predicate is total" parameter to the persistence API, because the caller shouldn't be using `listFullEntities` in the first place. It only needs to know whether more than one catalog role exists and then drop the remaining one, and `dropEntity` re-resolves that entity via `lookupEntity(catalogId, id, typeCode)` anyway β so the full rows it fetches, `JSONB` property blobs included, are thrown away. I checked what the drop helper actually reads from the entity: name, dropTimestamp, type/subType, catalogId and id. deleteEntity and deleteAllEntityGrantRecords both key on (id, catalogId) alone, and the policy-mapping and principal-secret branches are unreachable for a catalog role. All of that is present in the `EntityNameLookupRecord` that `listEntities` returns β which this PR bounds β so the call site can switch over with no loss and no extra round trip. I'll raise that separately so this PR stays scoped to the `li
stEntities` defect.
--
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] Fix JDBC listEntities fetching all remaining rows instead of the requested page [polaris]
Posted by "ayushtkn (via GitHub)" <gi...@apache.org>.
ayushtkn commented on code in PR #5255:
URL: https://github.com/apache/polaris/pull/5255#discussion_r3734702067
##########
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcBasePersistenceImpl.java:
##########
@@ -635,7 +649,8 @@ public <T> Page<T> listFullEntities(
entityType,
entitySubType,
pageToken,
- ModelEntity.getAllColumnNames(schemaVersion));
+ ModelEntity.getAllColumnNames(schemaVersion),
+ false);
Review Comment:
TLDR: It is same as before & intentional as mentioned in the comment as well. I have updated the description line which triggered this, but the PR title already mentioned `listEntities`
I too saw this and my plan around this, I will have a new PR around it:
`listFullEntities` applies `entityFilter` as a Java Predicate after the rows come back, so a SQL `LIMIT` there is not equivalent to one on `listEntities`. If the bound cuts the result set and the predicate then rejects some of what survived, the page comes back under-filled, `Page.mapped` sees `data.size() < limit`, sets `last = null`, and no continuation token is emitted β the client stops early with rows still unread. That is silent truncation rather than a slow query, and it's the same failure mode the new paginatedListEntitiesWalksEveryPageWhenTotalIsAMultipleOfPageSize test guards against on the listEntities side. Since the predicate is arbitrary Java, there's no way for `buildEntityQuery` to know from the inside whether bounding is safe, so `false` is the only correct default. Note this also means the PR doesn't change `listFullEntities` behaviour at all β it was unbounded before and stays unbounded; the flag just makes the existing behaviour explicit at the call site.
On the suggested fast-path for entity -> true: I looked at the call sites and it's narrower than it appears. `listFullEntitiesAll` passes `PageToken.readEverything()`, so it never requests pagination and is unaffected. `loadTasks` passes a genuinely selective predicate, and has no production caller today.
That leaves exactly one production caller that reaches this path with a page size β the catalog-role count in `AtomicOperationMetaStoreManager.dropEntity`, which uses `PageToken.fromLimit(2)` with an unconditional predicate.
I'd rather fix that one at the caller than add a "trust me, my predicate is total" parameter to the persistence API, because the caller shouldn't be using `listFullEntities` in the first place. It only needs to know whether more than one catalog role exists and then drop the remaining one, and `dropEntity` re-resolves that entity via `lookupEntity(catalogId, id, typeCode)` anyway β so the full rows it fetches, `JSONB` property blobs included, are thrown away. I checked what the drop helper actually reads from the entity: name, dropTimestamp, type/subType, catalogId and id. deleteEntity and deleteAllEntityGrantRecords both key on (id, catalogId) alone, and the policy-mapping and principal-secret branches are unreachable for a catalog role. All of that is present in the `EntityNameLookupRecord` that `listEntities` returns β which this PR bounds β so the call site can switch over with no loss and no extra round trip. I'll raise that separately so this PR stays scoped to the `li
stEntities` defect.
Thinking maybe for this method if there is actual prod use case we can do batch filter -> fetch -> check if above page size -> if not fetch again or so is also a possiblity, like `NoSqlMetaStore.listEntitiesBuildPage`
--
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] Fix JDBC listEntities fetching all remaining rows instead of the requested page [polaris]
Posted by "ayushtkn (via GitHub)" <gi...@apache.org>.
ayushtkn commented on PR #5255:
URL: https://github.com/apache/polaris/pull/5255#issuecomment-5247388302
resolved a conflict around import in the test file, rest stays same as before
--
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] Fix JDBC listEntities fetching all remaining rows instead of the requested page [polaris]
Posted by "ayushtkn (via GitHub)" <gi...@apache.org>.
ayushtkn commented on code in PR #5255:
URL: https://github.com/apache/polaris/pull/5255#discussion_r3739704417
##########
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcBasePersistenceImpl.java:
##########
@@ -575,12 +578,22 @@ private PreparedQuery buildEntityQuery(
entityIdToken ->
Map.<String, Object>of(ModelEntity.ID_COLUMN, entityIdToken.entityId()))
.orElse(Map.of());
+ OptionalInt pageSize = pageToken.pageSize();
+ if (boundByPageSize && pageSize.isPresent() && pageSize.getAsInt() < Integer.MAX_VALUE) {
Review Comment:
Thanx @dimas-b for the review!
Definitely makes sense. It would apply wherever `LIST_PAGINATION_ENABLED` is honoured β the flag is read in a single place (shouldDecodeToken()), feeding the three PageToken.build(...) calls in `IcebergCatalogHandler`, so a cap sitting next to it covers exactly the same endpoints and nothing else. That's also above the persistence layer, so `JDBC` and `NoSQL` both inherit it without either backend knowing about it β clamping inside the `JDBC` query builder would leave `NoSQL` uncovered.
The main thing to decide is what happens when a client asks for more than the configured limit: silently reduce, or reject. My preference is to reduce rather than throw β the Iceberg REST spec already allows a server to return fewer results than requested, clients follow the continuation token regardless, and rejecting would break callers that pass a large page size today and currently succeed.
For the default I was thinking 100, though I'm not attached to it β low enough to be a real bound, high enough that typical clients never notice, with the trade-off that anyone genuinely wanting larger pages just gets more round trips. Happy to go higher if that's too conservative. I'll open a follow-up PR if this sounds convincing and maybe we can have some more feedback around the behaviour and defaults :-)
--
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] Fix JDBC listEntities fetching all remaining rows instead of the requested page [polaris]
Posted by "ayushtkn (via GitHub)" <gi...@apache.org>.
ayushtkn commented on code in PR #5255:
URL: https://github.com/apache/polaris/pull/5255#discussion_r3734702067
##########
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcBasePersistenceImpl.java:
##########
@@ -635,7 +649,8 @@ public <T> Page<T> listFullEntities(
entityType,
entitySubType,
pageToken,
- ModelEntity.getAllColumnNames(schemaVersion));
+ ModelEntity.getAllColumnNames(schemaVersion),
+ false);
Review Comment:
TLDR: It is same as before
`listFullEntities` applies `entityFilter` as a Java Predicate after the rows come back, so a SQL `LIMIT` there is not equivalent to one on `listEntities`. If the bound cuts the result set and the predicate then rejects some of what survived, the page comes back under-filled, `Page.mapped` sees `data.size() < limit`, sets `last = null`, and no continuation token is emitted β the client stops early with rows still unread. That is silent truncation rather than a slow query, and it's the same failure mode the new paginatedListEntitiesWalksEveryPageWhenTotalIsAMultipleOfPageSize test guards against on the listEntities side. Since the predicate is arbitrary Java, there's no way for `buildEntityQuery` to know from the inside whether bounding is safe, so `false` is the only correct default. Note this also means the PR doesn't change `listFullEntities` behaviour at all β it was unbounded before and stays unbounded; the flag just makes the existing behaviour explicit at the call site.
On the suggested fast-path for entity -> true: I looked at the call sites and it's narrower than it appears. `listFullEntitiesAll` passes `PageToken.readEverything()`, so it never requests pagination and is unaffected. `loadTasks` passes a genuinely selective predicate, and has no production caller today.
That leaves exactly one production caller that reaches this path with a page size β the catalog-role count in `AtomicOperationMetaStoreManager.dropEntity`, which uses `PageToken.fromLimit(2)` with an unconditional predicate.
I'd rather fix that one at the caller than add a "trust me, my predicate is total" parameter to the persistence API, because the caller shouldn't be using `listFullEntities` in the first place. It only needs to know whether more than one catalog role exists and then drop the remaining one, and `dropEntity` re-resolves that entity via `lookupEntity(catalogId, id, typeCode)` anyway β so the full rows it fetches, `JSONB` property blobs included, are thrown away. I checked what the drop helper actually reads from the entity: name, dropTimestamp, type/subType, catalogId and id. deleteEntity and deleteAllEntityGrantRecords both key on (id, catalogId) alone, and the policy-mapping and principal-secret branches are unreachable for a catalog role. All of that is present in the `EntityNameLookupRecord` that `listEntities` returns β which this PR bounds β so the call site can switch over with no loss and no extra round trip. I'll raise that separately so this PR stays scoped to the `li
stEntities` defect.
--
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] Fix JDBC listEntities fetching all remaining rows instead of the requested page [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b merged PR #5255:
URL: https://github.com/apache/polaris/pull/5255
--
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] Fix JDBC listEntities fetching all remaining rows instead of the requested page [polaris]
Posted by "ayushtkn (via GitHub)" <gi...@apache.org>.
ayushtkn commented on code in PR #5255:
URL: https://github.com/apache/polaris/pull/5255#discussion_r3739698259
##########
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcBasePersistenceImpl.java:
##########
@@ -635,7 +649,8 @@ public <T> Page<T> listFullEntities(
entityType,
entitySubType,
pageToken,
- ModelEntity.getAllColumnNames(schemaVersion));
+ ModelEntity.getAllColumnNames(schemaVersion),
+ false);
Review Comment:
renamed and added a comment
--
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] Fix JDBC listEntities fetching all remaining rows instead of the requested page [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #5255:
URL: https://github.com/apache/polaris/pull/5255#discussion_r3739712350
##########
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcBasePersistenceImpl.java:
##########
@@ -575,12 +578,22 @@ private PreparedQuery buildEntityQuery(
entityIdToken ->
Map.<String, Object>of(ModelEntity.ID_COLUMN, entityIdToken.entityId()))
.orElse(Map.of());
+ OptionalInt pageSize = pageToken.pageSize();
+ if (boundByPageSize && pageSize.isPresent() && pageSize.getAsInt() < Integer.MAX_VALUE) {
Review Comment:
Per IRC spec `page-size` in the request is an upper bound, which totally makes sense. So servers can use a lower value.
This should certainly be addresses uniformly for all persistence types and in a separate PR :wink:
##########
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcBasePersistenceImpl.java:
##########
@@ -575,12 +578,22 @@ private PreparedQuery buildEntityQuery(
entityIdToken ->
Map.<String, Object>of(ModelEntity.ID_COLUMN, entityIdToken.entityId()))
.orElse(Map.of());
+ OptionalInt pageSize = pageToken.pageSize();
+ if (boundByPageSize && pageSize.isPresent() && pageSize.getAsInt() < Integer.MAX_VALUE) {
Review Comment:
Per IRC spec `page-size` in the request is an upper bound, which totally makes sense. So servers can use a lower value.
This should certainly be addressed uniformly for all persistence types and in a separate PR :wink:
--
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] Fix JDBC listEntities fetching all remaining rows instead of the requested page [polaris]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #5255:
URL: https://github.com/apache/polaris/pull/5255#discussion_r3734642901
##########
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcBasePersistenceImpl.java:
##########
@@ -635,7 +649,8 @@ public <T> Page<T> listFullEntities(
entityType,
entitySubType,
pageToken,
- ModelEntity.getAllColumnNames(schemaVersion));
+ ModelEntity.getAllColumnNames(schemaVersion),
+ false);
Review Comment:
`listFullEntities(...)` now calls `buildEntityQuery(..., boundByPageSize=false)`, which means paginated full-entity listings still wonβt emit a SQL `LIMIT` even when the caller provides a `PageToken` with a page size. The PR description says the change βpasses pageSize + 1 through as a SQL LIMIT whenever pagination is requestedβ, but thatβs only true for `listEntities(...)`.
If this is intentional because `entityFilter` is applied post-query (so a SQL LIMIT could under-fill pages and break continuation tokens), please update the PR description to reflect that. Otherwise, consider adding a bounded fast-path for the common `entity -> true` case (many call sites pass an unconditional predicate) so paginated full-entity walks donβt remain unbounded.
--
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] Fix JDBC listEntities fetching all remaining rows instead of the requested page [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #5255:
URL: https://github.com/apache/polaris/pull/5255#discussion_r3739628879
##########
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcBasePersistenceImpl.java:
##########
@@ -635,7 +649,8 @@ public <T> Page<T> listFullEntities(
entityType,
entitySubType,
pageToken,
- ModelEntity.getAllColumnNames(schemaVersion));
+ ModelEntity.getAllColumnNames(schemaVersion),
+ false);
Review Comment:
Maybe rename the `boundByPageSize` parameter to `applyPageSizeLimit` for clarity and add comment here?
##########
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcBasePersistenceImpl.java:
##########
@@ -575,12 +578,22 @@ private PreparedQuery buildEntityQuery(
entityIdToken ->
Map.<String, Object>of(ModelEntity.ID_COLUMN, entityIdToken.entityId()))
.orElse(Map.of());
+ OptionalInt pageSize = pageToken.pageSize();
+ if (boundByPageSize && pageSize.isPresent() && pageSize.getAsInt() < Integer.MAX_VALUE) {
Review Comment:
Tangential: Even limits slightly less than `Integer.MAX_VALUE` are probably not very meaningful practically speaking. Servers are allowed to reduce requested page sizes. We should probably have a configurable upper bound. WDYT?
--
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] Fix JDBC listEntities fetching all remaining rows instead of the requested page [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #5255:
URL: https://github.com/apache/polaris/pull/5255#discussion_r3739712350
##########
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcBasePersistenceImpl.java:
##########
@@ -575,12 +578,22 @@ private PreparedQuery buildEntityQuery(
entityIdToken ->
Map.<String, Object>of(ModelEntity.ID_COLUMN, entityIdToken.entityId()))
.orElse(Map.of());
+ OptionalInt pageSize = pageToken.pageSize();
+ if (boundByPageSize && pageSize.isPresent() && pageSize.getAsInt() < Integer.MAX_VALUE) {
Review Comment:
Per IRC spec `page-size` in the request is an upper bound, which totally makes sense. So servers can use a lower value.
This should certainly be addressed uniformly for all persistence types and in a separate PR :wink:
100 SGTM.
--
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] Fix JDBC listEntities fetching all remaining rows instead of the requested page [polaris]
Posted by "ayushtkn (via GitHub)" <gi...@apache.org>.
ayushtkn commented on code in PR #5255:
URL: https://github.com/apache/polaris/pull/5255#discussion_r3734702067
##########
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcBasePersistenceImpl.java:
##########
@@ -635,7 +649,8 @@ public <T> Page<T> listFullEntities(
entityType,
entitySubType,
pageToken,
- ModelEntity.getAllColumnNames(schemaVersion));
+ ModelEntity.getAllColumnNames(schemaVersion),
+ false);
Review Comment:
TLDR: It is same as before & intentional as mentioned in the comment as well. I have updated the description line which triggered this, but the PR title already mentioned `listEntities`
I too saw this and my plan around this, I will have a new PR around it:
`listFullEntities` applies `entityFilter` as a Java Predicate after the rows come back, so a SQL `LIMIT` there is not equivalent to one on `listEntities`. If the bound cuts the result set and the predicate then rejects some of what survived, the page comes back under-filled, `Page.mapped` sees `data.size() < limit`, sets `last = null`, and no continuation token is emitted β the client stops early with rows still unread. That is silent truncation rather than a slow query, and it's the same failure mode the new paginatedListEntitiesWalksEveryPageWhenTotalIsAMultipleOfPageSize test guards against on the listEntities side. Since the predicate is arbitrary Java, there's no way for `buildEntityQuery` to know from the inside whether bounding is safe, so `false` is the only correct default. Note this also means the PR doesn't change `listFullEntities` behaviour at all β it was unbounded before and stays unbounded; the flag just makes the existing behaviour explicit at the call site.
On the suggested fast-path for entity -> true: I looked at the call sites and it's narrower than it appears. `listFullEntitiesAll` passes `PageToken.readEverything()`, so it never requests pagination and is unaffected. `loadTasks` passes a genuinely selective predicate, and has no production caller today.
That leaves exactly one production caller that reaches this path with a page size β the catalog-role count in `AtomicOperationMetaStoreManager.dropEntity`, which uses `PageToken.fromLimit(2)` with an unconditional predicate.
I'd rather fix that one at the caller than add a "trust me, my predicate is total" parameter to the persistence API, because the caller shouldn't be using `listFullEntities` in the first place. It only needs to know whether more than one catalog role exists and then drop the remaining one, and `dropEntity` re-resolves that entity via `lookupEntity(catalogId, id, typeCode)` anyway β so the full rows it fetches, `JSONB` property blobs included, are thrown away. I checked what the drop helper actually reads from the entity: name, dropTimestamp, type/subType, catalogId and id. deleteEntity and deleteAllEntityGrantRecords both key on (id, catalogId) alone, and the policy-mapping and principal-secret branches are unreachable for a catalog role. All of that is present in the `EntityNameLookupRecord` that `listEntities` returns β which this PR bounds β so the call site can switch over with no loss and no extra round trip. I'll raise that separately so this PR stays scoped to the `li
stEntities` defect.
Thinking maybe for this method if there is actual prod use case we can do batch filter -> fetch -> check if above page size -> if not fetch again or so is also a possiblity
--
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