dennishuo commented on PR #2262: URL: https://github.com/apache/polaris/pull/2262#issuecomment-3177535471
@XN137 Good digging! I didn't realize that method 1 callsites had regressed. I did a bit more archaeology and it looks like https://github.com/apache/polaris/pull/1938 was actually what introduced the regression for the main `listTables`/`listNamespaces` behaviors: Previously [AtomicMetaStoreManager](https://github.com/apache/polaris/commit/fb418a2613715c219620f69fa4e9b7a1827898be#diff-b7a84ab04cb5601360515b443d5a3ccc1c4ed64a80f011980952b062d6844b8f) (and TransactionalMetaStoreManagerImpl): // prune the returned list with only entities matching the entity subtype if (entitySubType != PolarisEntitySubType.ANY_SUBTYPE) { resultPage = pageToken.buildNextPage( resultPage.items.stream() .filter(rec -> rec.getSubTypeCode() == entitySubType.getCode()) .collect(Collectors.toList())); } After: // prune the returned list with only entities matching the entity subtype Predicate<PolarisBaseEntity> filter = entitySubType != PolarisEntitySubType.ANY_SUBTYPE ? e -> e.getSubTypeCode() == entitySubType.getCode() : entity -> true; Page<EntityNameLookupRecord> resultPage = ms.listEntities(callCtx, catalogId, parentId, entityType, filter, pageToken); Fortunately this looks like this never made it into 1.0.x: https://github.com/apache/polaris/blob/d7e28e44d062d9ed38ca851a189886f1dc3605c9/polaris-core/src/main/java/org/apache/polaris/core/persistence/AtomicOperationMetaStoreManager.java#L713 That change would've been somewhat worse than the more-noticeable removal of the overloads for anyone using the standard `AtomicMetaStoreManager` or `TransactionalMetaStoreManagerImpl` with a private implementation of `BasePersistence`/`TransactionalPersistence` who may have come to depend strongly on the differentiation in performance between the different overloads, since unfortunately https://github.com/apache/polaris/pull/1938 would *silently* break this behavioral expectation. The *very* original introduction of both `filter` and `transformer` was honestly probably too rushed as a short-term crutch, and was exclusively for the [loadTasks](https://github.com/apache/polaris/blob/067bb9dc4992baa5f5dd023ffa9127f10f54f4f9/polaris-core/src/main/java/org/apache/polaris/core/persistence/AtomicOperationMetaStoreManager.java#L1496) which required very open-ended *post-processing* filtering. Overall I think your other two PRs are the right direction. The various `listEntities...map(loadEntity...)` flows are indeed one of the big known pain points no one got around to fixing yet so that's actually great to see! I'll comment on specifics within each PR. -- 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]
