jerryshao commented on code in PR #12603:
URL: https://github.com/apache/gravitino/pull/12603#discussion_r4118354409
##########
core/src/main/java/org/apache/gravitino/storage/relational/mapper/provider/DefaultMapperPackageProvider.java:
##########
@@ -83,9 +83,9 @@ public List<Class<?>> getMapperClasses() {
PolicyVersionMapper.class,
RoleMetaMapper.class,
SchemaMetaMapper.class,
- SecurableObjectMapper.class,
SemanticModelMetaMapper.class,
SemanticModelVersionInfoMapper.class,
+ SecurableObjectMapper.class,
Review Comment:
[Question] This moves `SecurableObjectMapper` from before the two new
Semantic Model mappers to after them, which is the only change in this file and
is unrelated to list/alter/drop. The surrounding list is alphabetical and `Sec`
sorts before `Sem`, so the pre-PR order was the correct one; the new order is
the odd entry.
Was this deliberate -- e.g. a MyBatis registration-order requirement you hit
while adding `semanticModelPOResultMap` and the nested
`mapToSemanticModelVersionInfoPO` map? If so it deserves a comment, because a
future alphabetical tidy-up would silently undo it. If it was accidental,
please revert the move to keep the diff to the feature.
Verified by: this is the entire diff for the file (`git diff -U0
origin/main...HEAD` shows one removal at old line 86 and one addition at new
line 88); I grepped `SecurableObjectMapper` and found no `@ResultMap`/`@One`
reference to any Semantic Model result map, and no test asserts the contents or
order of `getMapperClasses()`.
##########
core/src/main/java/org/apache/gravitino/storage/relational/service/SemanticModelPOStorageOps.java:
##########
@@ -80,6 +88,38 @@ public SemanticModelPO getPOByFullName(
return po;
}
+ @Override
+ public List<SemanticModelPO> listPOs(SemanticModelMetaMapper mapper, Long
parentId) {
+ return mapper.listSemanticModelPOsBySchemaId(parentId);
+ }
+
+ @Override
+ public List<SemanticModelPO> listPOs(
+ SemanticModelMetaMapper mapper, List<Long> semanticModelIds) {
+ return mapper.listSemanticModelPOsBySemanticModelIds(semanticModelIds);
+ }
+
+ @Override
+ public List<SemanticModelPO> listPOsByNSFullName(
+ SemanticModelMetaMapper mapper, Namespace namespace) {
+ List<SemanticModelPO> pos =
+ mapper.listSemanticModelPOsByFullQualifiedName(
+ namespace.level(0), namespace.level(1), namespace.level(2));
+ if (pos.isEmpty()) {
+ throw new NoSuchEntityException(
+ NoSuchEntityException.NO_SUCH_ENTITY_MESSAGE,
+ Entity.EntityType.CATALOG.name().toLowerCase(Locale.ROOT),
+ namespace.level(1));
+ }
+ if (pos.get(0).getSchemaId() == null) {
+ throw new NoSuchEntityException(
+ NoSuchEntityException.NO_SUCH_ENTITY_MESSAGE,
+ Entity.EntityType.SCHEMA.name().toLowerCase(Locale.ROOT),
+ namespace.level(2));
Review Comment:
[Nit] Neither `NoSuchEntityException` branch here is covered, and the
full-qualified-name route itself is never taken in a test.
`TestSemanticModelJDBCBackend.testSemanticModelReadRoutesAndEntityIdResolver:326-332`
deliberately drives the single-entity read down both routes via
`readSemanticModelPO(ident, cacheEnabled)` and asserts the
missing-metalake/catalog/schema cases, but the list path has no counterpart:
`listSemanticModelsByNamespace` is only called at
`TestSemanticModelMetaService:115` and `:263` and `backend.list(...)` at
`TestSemanticModelJDBCBackend:409`, all of which take whichever route
`GravitinoEnv.cacheEnabled()` yields.
A sibling of the existing helper -- list under `missing_catalog` and under
`missing_schema` with `cacheEnabled=false` -- would pin both messages and cover
the empty-schema filter on line 120. This is the lowest-covered changed file in
the coverage report (63.89%), which matches.
Verified by: grep for `listSemanticModelsByNamespace`, `backend.list`, and
`listPOsByNSFullName` across both new test files; read `readSemanticModelPO` at
`TestSemanticModelJDBCBackend.java:616-626`.
##########
core/src/main/java/org/apache/gravitino/storage/relational/service/SemanticModelPOStorageOps.java:
##########
@@ -80,6 +88,38 @@ public SemanticModelPO getPOByFullName(
return po;
}
+ @Override
+ public List<SemanticModelPO> listPOs(SemanticModelMetaMapper mapper, Long
parentId) {
+ return mapper.listSemanticModelPOsBySchemaId(parentId);
+ }
+
+ @Override
+ public List<SemanticModelPO> listPOs(
+ SemanticModelMetaMapper mapper, List<Long> semanticModelIds) {
+ return mapper.listSemanticModelPOsBySemanticModelIds(semanticModelIds);
Review Comment:
[Nit] This `listPOs(mapper, List<Long>)` override and the
`listSemanticModelPOsBySemanticModelIds` mapper method plus its base and
PostgreSQL SQL have no caller. The only consumer of the id-list variant is
`MetadataObjectService` (`:362` functions, `:416` tables, `:565` views, `:636`
schemas), which has no `SEMANTIC_MODEL` branch, and `JDBCBackend.batchGet`
resolves semantic models one identifier at a time instead
(`JDBCBackend.java:368-380`).
Either point `batchGet` at a `batchGetSemanticModelByIdentifier` built on
this batch SQL -- the rows are already fetched by natural key, so only a
name-to-id step is missing -- or drop the override until the securable-object
work in the `TODO` at `SemanticModelMetaService.java:390-391` needs it. `VIEW`
has the same per-identifier loop under the `TODO` at `JDBCBackend.java:334`, so
following it is defensible; I am flagging it only because this PR ships the
batch query that would fix it.
Verified by: repo-wide grep for `listSemanticModelPOsBySemanticModelIds` and
for `ops.listPOs(` over `core/src/main/java` -- the only hits for the id-list
form are the four `MetadataObjectService` call sites and
`HierarchicalConversionPOStorageOps:99`.
--
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]
