eric-maynard commented on code in PR #273:
URL: https://github.com/apache/polaris/pull/273#discussion_r2022074128
##########
extension/persistence/eclipselink/src/main/java/org/apache/polaris/extension/persistence/impl/eclipselink/PolarisEclipseLinkStore.java:
##########
@@ -278,21 +281,40 @@ long countActiveChildEntities(
}
List<ModelEntity> lookupFullEntitiesActive(
- EntityManager session, long catalogId, long parentId, @Nonnull
PolarisEntityType entityType) {
+ EntityManager session,
+ long catalogId,
+ long parentId,
+ @Nonnull PolarisEntityType entityType,
+ PageToken pageToken) {
diagnosticServices.check(session != null, "session_is_null");
+ diagnosticServices.check(
+ (pageToken instanceof EntityIdPageToken || pageToken instanceof
ReadEverythingPageToken),
+ "unexpected_page_token");
checkInitialized();
// Currently check against ENTITIES not joining with ENTITIES_ACTIVE
String hql =
- "SELECT m from ModelEntity m where m.catalogId=:catalogId and
m.parentId=:parentId and m.typeCode=:typeCode";
+ "SELECT m from ModelEntity m "
+ + "where m.catalogId=:catalogId and m.parentId=:parentId and
m.typeCode=:typeCode and m.id > :tokenId";
+
+ if (pageToken instanceof EntityIdPageToken) {
Review Comment:
Ah, I think I understand your comment better now. You are pointing out that
depending on the page token type, the results may or may not get sorted by
entity ID? That's true.
Not sorting is more of an optimization for the case where we have a
`ReadEverythingPageToken` (i.e. no page token). We could remove this
optimization and always sort, but I added it to improve performance as in the
majority of cases we have no page token.
In fact, I am trying _not_ to couple page token implementations with
metastore implementations here. In theory, the treemap metastore could also
work with EntityIdPageToken.
I agree that the runtime check is a little awkward -- although we have such
runtime checks all over the place currently. Is there a way you think we can
achieve this same behavior without one?
I'm really hesitant to just always sort here. On the other hand, I think
adding methods to `PageToken` that are really only relevant to one
implementation (e.g. `requiresSortingByEntityId`) does not make a ton of sense
from the interface perspective. Further, I think flipping the dependency
between the metastore and page token types (e.g.
`PageToken::rewriteEclipseLinkQueryAsNeeded`) is not a good pattern either.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
