eric-maynard commented on code in PR #1555:
URL: https://github.com/apache/polaris/pull/1555#discussion_r2085101838
##########
extension/persistence/relational-jdbc/src/main/java/org/apache/polaris/extension/persistence/relational/jdbc/JdbcBasePersistenceImpl.java:
##########
@@ -414,6 +415,11 @@ public <T> Page<T> listEntities(
// Limit can't be pushed down, due to client side filtering
// absence of transaction.
String query = QueryGenerator.generateSelectQuery(new ModelEntity(),
params);
+
+ if (pageToken instanceof EntityIdPageToken entityIdPageToken) {
+ query += String.format(" AND id > %d ORDER BY id ASC",
entityIdPageToken.getId());
Review Comment:
I spoke with @singhpk234 who noted this is probably the same discussion as
[here on the old
PR](https://github.com/apache/polaris/pull/273#discussion_r2021548762). With
that context, I think we might be OK here.
IMO it's alright that the list ordering you'd get across metastores won't be
the same. Other than that difference, seems like everything should work with
JDBC's `IdGenerator`. Although the IDs aren't generated sequentially,
pagination only uses the entity ID as an essentially arbitrary consistent
ordering.
The key implication here is that if an entity gets created in the middle of
a listing operation (e.g. between list calls 2 and 3) it may or may not show up
in the next page. An alternative would be to try to filter it out so that the
behavior is more obvious & consistent, but I think the simple approach that
ultimately gives the user a chance to see these new entities is good.
--
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-unsubscr...@polaris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
