jerryshao commented on code in PR #12833:
URL: https://github.com/apache/gravitino/pull/12833#discussion_r4120962187
##########
core/src/main/java/org/apache/gravitino/catalog/SemanticModelOperationDispatcher.java:
##########
@@ -104,6 +104,9 @@ public SemanticModel createSemanticModel(
public SemanticModel alterSemanticModel(NameIdentifier ident,
SemanticModelChange... changes)
throws NoSuchSemanticModelException, SemanticModelAlreadyExistsException,
IllegalSemanticModelException {
+ if (changes == null || changes.length == 0) {
+ throw new IllegalSemanticModelException("At least one Semantic Model
change is required");
+ }
Review Comment:
[Question] The newly reachable
`listSemanticModels`/`alterSemanticModel`/`dropSemanticModel` paths take no
tree lock, unlike every sibling dispatcher:
`FunctionOperationDispatcher.java:96,115,151,180,199` wraps each operation in
`TreeLockUtils.doWithTreeLock`, and `ViewOperationDispatcher.dropView`
(`:318-353`) takes a schema `WRITE` lock around its check-then-delete. Here
`alterSemanticModel` and `dropSemanticModel` both do `schemaExists(...)` and
then mutate with nothing serializing them against a concurrent schema drop, and
`listSemanticModels` does `loadSchema` then `list` without a `READ` lock, so a
concurrent cascade can turn it into an empty list instead of
`NoSuchSchemaException`.
The storage layer does guard the cascade race itself - `insertSemanticModel`
uses `doWithSchemaWriteLock` and `updateSemanticModel` re-locks the observed
parent with `lockSchemaForEntityWrite`
(`SemanticModelMetaService.java:146-150`, `:238-248`) - so this may be a
deliberate substitution of DB-level locks for tree locks. If so it is worth a
one-line comment here, since the asymmetry with the other dispatchers will read
as an omission; if not, the three operations want the same tree locks their
siblings use.
Verified by: `git grep TreeLockUtils` across
`core/src/main/java/org/apache/gravitino/catalog/` (no hit in this file), and
reading the lock calls in `SemanticModelMetaService`.
##########
core/src/main/java/org/apache/gravitino/catalog/ManagedSemanticModelOperations.java:
##########
@@ -71,9 +73,17 @@ public ManagedSemanticModelOperations(
@Override
public NameIdentifier[] listSemanticModels(Namespace namespace) throws
NoSuchSchemaException {
- // TODO: Implement in the Semantic Model list/alter/drop capability.
- throw new UnsupportedOperationException(
- "listSemanticModels: list/alter/drop capability is not implemented");
+ try {
+ List<SemanticModelEntity> models =
+ store.list(namespace, SemanticModelEntity.class,
Entity.EntityType.SEMANTIC_MODEL);
+ return models.stream()
+ .map(model -> NameIdentifier.of(namespace, model.name()))
+ .toArray(NameIdentifier[]::new);
Review Comment:
[Nit] This names-only API materializes every model in full.
`RelationalEntityStore.list(namespace, type, entityType)` passes `allFields =
false` (`RelationalEntityStore.java:195-198`), but the `SEMANTIC_MODEL` branch
ignores that flag (`JDBCBackend.java:145-147`), the list SQL selects the joined
version columns including the definition blob
(`SemanticModelMetaBaseSQLProvider.java:48-58`), and
`SemanticModelPO.fromSemanticModelPO:109-131` Jackson-parses each definition
into a `SemanticModelDefinitionDTO` and converts it - all discarded here except
`model.name()`.
`MODEL`, `VIEW` and `FUNCTION` list the same way today, so this is
consistent rather than wrong, and honoring `allFields` would mean a new
names-only mapper query. Worth a follow-up if schemas are expected to hold many
models with large definitions.
Verified by: following `store.list` through `RelationalEntityStore` ->
`JDBCBackend.list` -> `listSemanticModelsByNamespace` ->
`listSemanticModelPOsBySchemaId`, and reading `fromSemanticModelPO`.
##########
core/src/main/java/org/apache/gravitino/catalog/ManagedSemanticModelOperations.java:
##########
@@ -131,15 +141,106 @@ public SemanticModel createSemanticModel(
public SemanticModel alterSemanticModel(NameIdentifier ident,
SemanticModelChange... changes)
throws NoSuchSemanticModelException, SemanticModelAlreadyExistsException,
IllegalSemanticModelException {
- // TODO: Implement in the Semantic Model list/alter/drop capability.
- throw new UnsupportedOperationException(
- "alterSemanticModel: list/alter/drop capability is not implemented");
+ boolean validateForWrite = requiresWriteValidation(changes);
+
+ try {
+ return store.update(
Review Comment:
[Question] Is a new definition snapshot per metadata-only alter intended?
This `store.update` call is the only production caller of
`SemanticModelMetaService.updateSemanticModel` (`JDBCBackend.java:1069-1070`),
and that path is unconditional: `updateSemanticModelPO` sets
`currentVersion`/`lastVersion` to `max(current, last) + 1`
(`SemanticModelMetaService.java:411-428`) and a full
`insertSemanticModelVersionInfo` row is always written (`:258-263`), carrying a
fresh copy of the serialized definition.
So `setProperty`, `updateComment`, a rename, and even
`removeProperty("absent")` - which changes nothing - each bump the version and
duplicate the whole definition JSON, which is the largest payload this entity
has. That also feeds `deleteSemanticModelVersionsByRetentionCount`, so metadata
churn can push real definition history out of the retention window.
If snapshots are meant to be definition history, consider reusing the
current version row when `newDefinition == oldEntity.definition()`; if a
snapshot per alter is the intent, a short comment here (or on
`updateSemanticModel`) saying so would keep the next reader from filing this as
a bug.
Verified by: reading `updateSemanticModel` and `updateSemanticModelPO` in
full, and `git grep updateSemanticModel(` to confirm `JDBCBackend` is the sole
caller and this line the sole entry point.
##########
core/src/main/java/org/apache/gravitino/catalog/ManagedSemanticModelOperations.java:
##########
@@ -131,15 +141,106 @@ public SemanticModel createSemanticModel(
public SemanticModel alterSemanticModel(NameIdentifier ident,
SemanticModelChange... changes)
throws NoSuchSemanticModelException, SemanticModelAlreadyExistsException,
IllegalSemanticModelException {
- // TODO: Implement in the Semantic Model list/alter/drop capability.
- throw new UnsupportedOperationException(
- "alterSemanticModel: list/alter/drop capability is not implemented");
+ boolean validateForWrite = requiresWriteValidation(changes);
+
+ try {
+ return store.update(
+ ident,
+ SemanticModelEntity.class,
+ Entity.EntityType.SEMANTIC_MODEL,
+ oldEntity -> {
+ SemanticModelEntity candidate = applyChanges(oldEntity, changes);
+ if (validateForWrite) {
+ writeValidator.accept(
+ NameIdentifier.of(candidate.namespace(), candidate.name()),
+ candidate.definition());
+ }
+ return candidate;
+ });
+ } catch (NoSuchEntityException e) {
+ throw new NoSuchSemanticModelException(e, "Semantic Model %s does not
exist", ident);
+ } catch (EntityAlreadyExistsException e) {
+ throw new SemanticModelAlreadyExistsException(
+ e, "A Semantic Model with the requested name already exists while
altering %s", ident);
+ } catch (IOException e) {
+ throw new RuntimeException("Failed to alter Semantic Model " + ident, e);
+ }
}
@Override
public boolean dropSemanticModel(NameIdentifier ident) {
- // TODO: Implement in the Semantic Model list/alter/drop capability.
- throw new UnsupportedOperationException(
- "dropSemanticModel: list/alter/drop capability is not implemented");
+ try {
+ return store.delete(ident, Entity.EntityType.SEMANTIC_MODEL);
Review Comment:
[Important] This is the first production caller of `store.delete(...,
SEMANTIC_MODEL, ...)`, and the storage delete path deliberately leaves
associated relations behind: `SemanticModelMetaService.java:384-385` carries
`// TODO: Soft-delete Semantic Model owner, tag, and securable-object relations
in this transaction and in the metalake/catalog/schema cascade delete paths`,
and `deleteSemanticModelWithVersion` only soft-deletes the meta row, its
version rows, and writes the DROP change-log entry.
Semantic Models are already addressable as metadata objects -
`MetadataObjectUtil.java:67` maps `MetadataObject.Type.SEMANTIC_MODEL` to
`Entity.EntityType.SEMANTIC_MODEL` and `:131` resolves it in `toEntityIdent`,
which is the path `TagManager` (`TagManager.java:391,423,496`) and the owner
APIs go through. So once drop is reachable, tagging or assigning an owner to a
model and then dropping it leaves orphaned relation rows that no cascade cleans
up.
Nothing in this PR has to change if that is the plan - the Core dispatcher
has no REST surface yet - but please confirm the TODO is tracked by an issue
and closed before the alter/drop REST path (#12626) lands, and say so in the PR
description so it is not lost.
Verified by: reading `deleteSemanticModelWithVersion`
(`SemanticModelMetaService.java:373-409`) in full, `git grep` for other callers
of `store.delete` with `SEMANTIC_MODEL` (only this line), and
`MetadataObjectUtil`/`TagManager` for reachability.
--
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]
