jerryshao commented on code in PR #12565: URL: https://github.com/apache/gravitino/pull/12565#discussion_r3850443126
########## core/src/main/java/org/apache/gravitino/catalog/ManagedSemanticModelOperations.java: ########## @@ -0,0 +1,103 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.gravitino.catalog; + +import java.util.Map; +import javax.annotation.Nullable; +import org.apache.gravitino.EntityStore; +import org.apache.gravitino.NameIdentifier; +import org.apache.gravitino.Namespace; +import org.apache.gravitino.exceptions.IllegalSemanticModelException; +import org.apache.gravitino.exceptions.NoSuchSchemaException; +import org.apache.gravitino.exceptions.NoSuchSemanticModelException; +import org.apache.gravitino.exceptions.SemanticModelAlreadyExistsException; +import org.apache.gravitino.semantic.SemanticModel; +import org.apache.gravitino.semantic.SemanticModelCatalog; +import org.apache.gravitino.semantic.SemanticModelChange; +import org.apache.gravitino.semantic.SemanticModelDefinition; +import org.apache.gravitino.storage.IdGenerator; + +/** + * Provides storage-level Semantic Model operations backed by Gravitino's {@link EntityStore}. + * + * <p>The framework establishes the managed-operation boundary. Semantic Model entity persistence + * and lifecycle implementations will be added in a follow-up change. + */ +public class ManagedSemanticModelOperations implements SemanticModelCatalog { + + @SuppressWarnings("UnusedVariable") + private final EntityStore store; + + @SuppressWarnings("UnusedVariable") + private final IdGenerator idGenerator; + + /** + * Creates managed Semantic Model operations. + * + * @param store The EntityStore used for persistence. + * @param idGenerator The stable entity ID generator. + */ + public ManagedSemanticModelOperations(EntityStore store, IdGenerator idGenerator) { + this.store = store; + this.idGenerator = idGenerator; + } + + @Override + public NameIdentifier[] listSemanticModels(Namespace namespace) throws NoSuchSchemaException { + // TODO: Implement when SemanticModelEntity is available. + throw new UnsupportedOperationException( + "listSemanticModels: SemanticModelEntity is not yet implemented"); + } + + @Override + public SemanticModel loadSemanticModel(NameIdentifier ident) throws NoSuchSemanticModelException { + // TODO: Implement when SemanticModelEntity is available. + throw new UnsupportedOperationException( Review Comment: **[Confirmed bug, surfaces via `SemanticModelCatalog#semanticModelExists` default]** This stub throws `UnsupportedOperationException`, but the inherited default `semanticModelExists(NameIdentifier)` (`api/.../semantic/SemanticModelCatalog.java:59`) only catches `NoSuchSemanticModelException` around its call to `loadSemanticModel`. Any caller invoking `semanticModelExists()` on an identifier whose parent schema exists (e.g. a future REST pre-check, or `SemanticModelOperationDispatcher`'s inherited default via `managedOperations.loadSemanticModel`) gets an uncaught `UnsupportedOperationException` instead of `true`/`false`, breaking the documented boolean contract of `semanticModelExists` even though the method is fully wired end-to-end through both dispatcher layers. Worth having this stub throw `NoSuchSemanticModelException` (or having `semanticModelExists` special-cased) until the real implementation lands. ########## core/src/main/java/org/apache/gravitino/GravitinoEnv.java: ########## @@ -869,6 +883,15 @@ private void initGravitinoServerComponents() { new ViewEventDispatcher(eventBus, viewNormalizeDispatcher); this.viewDispatcher = viewEventDispatcher; + // Semantic Model operation chain: SemanticModelNormalizeDispatcher -> + // SemanticModelOperationDispatcher -> ManagedSemanticModelOperations. + // TODO: Add event and hook layers with Semantic Model server integration. + SemanticModelOperationDispatcher semanticModelOperationDispatcher = + new SemanticModelOperationDispatcher( + catalogManager, internalSchemaDispatcher, entityStore, idGenerator, secretManager); Review Comment: **[Plausible efficiency]** `SemanticModelOperationDispatcher` is constructed with `internalSchemaDispatcher` (already schema-normalizing), unlike the sibling `FunctionOperationDispatcher` which is wired with the raw, un-normalized `schemaOperationDispatcher` (see this file around line 856-857). `SemanticModelNormalizeDispatcher.normalizeParentForLookup` already normalizes the schema name via `CapabilityHelpers` before delegating to `SemanticModelOperationDispatcher`; because `internalSchemaDispatcher == schemaNormalizeDispatcher`, every list/load/create/alter/drop call re-normalizes the schema name and performs a second `catalogManager.loadCatalogAndWrap` capability lookup per call — an extra round-trip on every Semantic Model operation that Function/Table dispatchers avoid by using the raw schema dispatcher. ########## core/src/main/java/org/apache/gravitino/catalog/SemanticModelOperationDispatcher.java: ########## @@ -0,0 +1,152 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.gravitino.catalog; + +import com.google.common.base.Preconditions; +import java.util.Arrays; +import java.util.Map; +import javax.annotation.Nullable; +import org.apache.gravitino.Catalog; +import org.apache.gravitino.EntityStore; +import org.apache.gravitino.NameIdentifier; +import org.apache.gravitino.Namespace; +import org.apache.gravitino.exceptions.IllegalSemanticModelException; +import org.apache.gravitino.exceptions.NoSuchSchemaException; +import org.apache.gravitino.exceptions.NoSuchSemanticModelException; +import org.apache.gravitino.exceptions.SemanticModelAlreadyExistsException; +import org.apache.gravitino.lock.LockType; +import org.apache.gravitino.lock.TreeLockUtils; +import org.apache.gravitino.secret.SecretManager; +import org.apache.gravitino.semantic.SemanticModel; +import org.apache.gravitino.semantic.SemanticModelChange; +import org.apache.gravitino.semantic.SemanticModelDefinition; +import org.apache.gravitino.storage.IdGenerator; + +/** Dispatches always-managed Semantic Model operations to Gravitino's EntityStore. */ +public class SemanticModelOperationDispatcher extends OperationDispatcher + implements SemanticModelDispatcher { + + private final CatalogManager catalogManager; + private final SchemaDispatcher schemaDispatcher; + private final ManagedSemanticModelOperations managedOperations; + + /** + * Creates a Semantic Model operation dispatcher. + * + * @param catalogManager The catalog manager. + * @param schemaDispatcher The internal schema dispatcher used for parent validation. + * @param store The EntityStore used for Semantic Model persistence. + * @param idGenerator The stable entity ID generator. + * @param secretManager The secret manager required by the operation dispatcher base class. + */ + public SemanticModelOperationDispatcher( + CatalogManager catalogManager, + SchemaDispatcher schemaDispatcher, + EntityStore store, + IdGenerator idGenerator, + SecretManager secretManager) { + super(catalogManager, store, idGenerator, secretManager); + this.catalogManager = catalogManager; + this.schemaDispatcher = schemaDispatcher; + this.managedOperations = new ManagedSemanticModelOperations(store, idGenerator); + } + + @Override + public NameIdentifier[] listSemanticModels(Namespace namespace) throws NoSuchSchemaException { + checkRelationalCatalog(namespace); + NameIdentifier schemaIdent = NameIdentifier.of(namespace.levels()); + schemaDispatcher.loadSchema(schemaIdent); + return TreeLockUtils.doWithTreeLock( + schemaIdent, LockType.READ, () -> managedOperations.listSemanticModels(namespace)); + } + + @Override + public SemanticModel loadSemanticModel(NameIdentifier ident) throws NoSuchSemanticModelException { + checkRelationalCatalog(ident.namespace()); + if (!schemaDispatcher.schemaExists(schemaIdentifier(ident))) { + throw new NoSuchSemanticModelException("Semantic Model %s does not exist", ident); + } + return TreeLockUtils.doWithTreeLock( + ident, LockType.READ, () -> managedOperations.loadSemanticModel(ident)); + } + + @Override + public SemanticModel createSemanticModel( + NameIdentifier ident, + @Nullable String comment, + SemanticModelDefinition definition, + Map<String, String> properties) + throws NoSuchSchemaException, SemanticModelAlreadyExistsException, + IllegalSemanticModelException { + Preconditions.checkArgument(definition != null, "Definition must not be null"); + Preconditions.checkArgument(properties != null, "Properties must not be null"); + checkRelationalCatalog(ident.namespace()); + NameIdentifier schemaIdent = schemaIdentifier(ident); + schemaDispatcher.loadSchema(schemaIdent); + return TreeLockUtils.doWithTreeLock( + schemaIdent, + LockType.WRITE, + () -> managedOperations.createSemanticModel(ident, comment, definition, properties)); + } + + @Override + public SemanticModel alterSemanticModel(NameIdentifier ident, SemanticModelChange... changes) + throws NoSuchSemanticModelException, SemanticModelAlreadyExistsException, + IllegalSemanticModelException { + Preconditions.checkArgument( Review Comment: **[Plausible simplification]** This re-validates `changes != null && changes.length > 0` even though `SemanticModelNormalizeDispatcher.alterSemanticModel` (line 90) already performs the identical check and is the only production entry point into this dispatcher, making this check unreachable dead code. `GravitinoEnv` always wires `SemanticModelNormalizeDispatcher` in front of this dispatcher, so an empty/null `changes` array is rejected by the normalize layer before ever reaching this method. No sibling normalize dispatcher (`FunctionNormalizeDispatcher`, `TableNormalizeDispatcher`) duplicates such validation — they only normalize and delegate — so this is a deviation that leaves dead validation logic here. ########## core/src/main/java/org/apache/gravitino/catalog/SemanticModelOperationDispatcher.java: ########## @@ -0,0 +1,152 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.gravitino.catalog; + +import com.google.common.base.Preconditions; +import java.util.Arrays; +import java.util.Map; +import javax.annotation.Nullable; +import org.apache.gravitino.Catalog; +import org.apache.gravitino.EntityStore; +import org.apache.gravitino.NameIdentifier; +import org.apache.gravitino.Namespace; +import org.apache.gravitino.exceptions.IllegalSemanticModelException; +import org.apache.gravitino.exceptions.NoSuchSchemaException; +import org.apache.gravitino.exceptions.NoSuchSemanticModelException; +import org.apache.gravitino.exceptions.SemanticModelAlreadyExistsException; +import org.apache.gravitino.lock.LockType; +import org.apache.gravitino.lock.TreeLockUtils; +import org.apache.gravitino.secret.SecretManager; +import org.apache.gravitino.semantic.SemanticModel; +import org.apache.gravitino.semantic.SemanticModelChange; +import org.apache.gravitino.semantic.SemanticModelDefinition; +import org.apache.gravitino.storage.IdGenerator; + +/** Dispatches always-managed Semantic Model operations to Gravitino's EntityStore. */ +public class SemanticModelOperationDispatcher extends OperationDispatcher + implements SemanticModelDispatcher { + + private final CatalogManager catalogManager; + private final SchemaDispatcher schemaDispatcher; + private final ManagedSemanticModelOperations managedOperations; + + /** + * Creates a Semantic Model operation dispatcher. + * + * @param catalogManager The catalog manager. + * @param schemaDispatcher The internal schema dispatcher used for parent validation. + * @param store The EntityStore used for Semantic Model persistence. + * @param idGenerator The stable entity ID generator. + * @param secretManager The secret manager required by the operation dispatcher base class. + */ + public SemanticModelOperationDispatcher( + CatalogManager catalogManager, + SchemaDispatcher schemaDispatcher, + EntityStore store, + IdGenerator idGenerator, + SecretManager secretManager) { + super(catalogManager, store, idGenerator, secretManager); + this.catalogManager = catalogManager; + this.schemaDispatcher = schemaDispatcher; + this.managedOperations = new ManagedSemanticModelOperations(store, idGenerator); + } + + @Override + public NameIdentifier[] listSemanticModels(Namespace namespace) throws NoSuchSchemaException { + checkRelationalCatalog(namespace); + NameIdentifier schemaIdent = NameIdentifier.of(namespace.levels()); + schemaDispatcher.loadSchema(schemaIdent); + return TreeLockUtils.doWithTreeLock( + schemaIdent, LockType.READ, () -> managedOperations.listSemanticModels(namespace)); + } + + @Override + public SemanticModel loadSemanticModel(NameIdentifier ident) throws NoSuchSemanticModelException { + checkRelationalCatalog(ident.namespace()); + if (!schemaDispatcher.schemaExists(schemaIdentifier(ident))) { + throw new NoSuchSemanticModelException("Semantic Model %s does not exist", ident); + } + return TreeLockUtils.doWithTreeLock( + ident, LockType.READ, () -> managedOperations.loadSemanticModel(ident)); + } + + @Override + public SemanticModel createSemanticModel( + NameIdentifier ident, + @Nullable String comment, + SemanticModelDefinition definition, + Map<String, String> properties) + throws NoSuchSchemaException, SemanticModelAlreadyExistsException, + IllegalSemanticModelException { + Preconditions.checkArgument(definition != null, "Definition must not be null"); + Preconditions.checkArgument(properties != null, "Properties must not be null"); + checkRelationalCatalog(ident.namespace()); + NameIdentifier schemaIdent = schemaIdentifier(ident); + schemaDispatcher.loadSchema(schemaIdent); + return TreeLockUtils.doWithTreeLock( + schemaIdent, + LockType.WRITE, + () -> managedOperations.createSemanticModel(ident, comment, definition, properties)); + } + + @Override + public SemanticModel alterSemanticModel(NameIdentifier ident, SemanticModelChange... changes) + throws NoSuchSemanticModelException, SemanticModelAlreadyExistsException, + IllegalSemanticModelException { + Preconditions.checkArgument( + changes != null && changes.length > 0, "At least one change is required"); + checkRelationalCatalog(ident.namespace()); + NameIdentifier schemaIdent = schemaIdentifier(ident); + if (!schemaDispatcher.schemaExists(schemaIdent)) { + throw new NoSuchSemanticModelException("Semantic Model %s does not exist", ident); + } + + boolean renaming = + Arrays.stream(changes) + .anyMatch(change -> change instanceof SemanticModelChange.RenameSemanticModel); + NameIdentifier lockIdent = renaming ? schemaIdent : ident; + return TreeLockUtils.doWithTreeLock( + lockIdent, LockType.WRITE, () -> managedOperations.alterSemanticModel(ident, changes)); + } + + @Override + public boolean dropSemanticModel(NameIdentifier ident) { + checkRelationalCatalog(ident.namespace()); + if (!schemaDispatcher.schemaExists(schemaIdentifier(ident))) { + return false; + } + return TreeLockUtils.doWithTreeLock( + ident, LockType.WRITE, () -> managedOperations.dropSemanticModel(ident)); + } + + private void checkRelationalCatalog(Namespace namespace) { Review Comment: **[Plausible simplification, x2]** 1) This hardcodes a `Catalog.Type.RELATIONAL`-only restriction as a new hand-rolled load-catalog-and-compare-type check, with no generalized capability-based or scope-to-catalog-type compatibility mechanism elsewhere in the codebase for this kind of restriction. Neither `FunctionOperationDispatcher` (closest always-managed analog) nor `CapabilityHelpers`/`Capability.java` provide a reusable catalog-type gate; a future resource type needing the same restriction, or a future catalog type that should support Semantic Models, requires hand-editing this private method rather than the catalog/connector declaring its own compatible-scope capability. 2) It also duplicates the load-catalog + compare-type + throw pattern already implemented in `AuthorizationUtils.checkCatalogType` (`core/.../authorization/AuthorizationUtils.java:634-643`), just with a different exception type and no shared helper. Not exploitable today (different purpose: privilege gating vs. operation gating), but it's now a second independent implementation of the same "reject unsupported catalog type" logic that could drift from the first (e.g. how the catalog identifier is derived: `NameIdentifier.of(level(0), level(1))` here vs `getCatalogIdentifier(ident)` there). ########## core/src/main/java/org/apache/gravitino/GravitinoEnv.java: ########## @@ -869,6 +883,15 @@ private void initGravitinoServerComponents() { new ViewEventDispatcher(eventBus, viewNormalizeDispatcher); this.viewDispatcher = viewEventDispatcher; + // Semantic Model operation chain: SemanticModelNormalizeDispatcher -> + // SemanticModelOperationDispatcher -> ManagedSemanticModelOperations. + // TODO: Add event and hook layers with Semantic Model server integration. Review Comment: **[Plausible]** `SemanticModelDispatcher` is wired with no `EventDispatcher` or `HookDispatcher` layer, unlike every other managed resource type in this file (Table, View, Fileset, Topic, Model, Function all get at least an `EventDispatcher` wrapper). The gap is explicitly TODO'd here ("Add event and hook layers with Semantic Model server integration"), but once a REST layer is wired to `GravitinoEnv.semanticModelDispatcher()`, Semantic Model create/alter/drop/list operations will fire no audit events at all — silently breaking parity with the audit/listener infrastructure every other resource type provides, unless the event layer is added before (not after) the dispatcher is exposed to callers. ########## core/src/main/java/org/apache/gravitino/catalog/SemanticModelOperationDispatcher.java: ########## @@ -0,0 +1,152 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.gravitino.catalog; + +import com.google.common.base.Preconditions; +import java.util.Arrays; +import java.util.Map; +import javax.annotation.Nullable; +import org.apache.gravitino.Catalog; +import org.apache.gravitino.EntityStore; +import org.apache.gravitino.NameIdentifier; +import org.apache.gravitino.Namespace; +import org.apache.gravitino.exceptions.IllegalSemanticModelException; +import org.apache.gravitino.exceptions.NoSuchSchemaException; +import org.apache.gravitino.exceptions.NoSuchSemanticModelException; +import org.apache.gravitino.exceptions.SemanticModelAlreadyExistsException; +import org.apache.gravitino.lock.LockType; +import org.apache.gravitino.lock.TreeLockUtils; +import org.apache.gravitino.secret.SecretManager; +import org.apache.gravitino.semantic.SemanticModel; +import org.apache.gravitino.semantic.SemanticModelChange; +import org.apache.gravitino.semantic.SemanticModelDefinition; +import org.apache.gravitino.storage.IdGenerator; + +/** Dispatches always-managed Semantic Model operations to Gravitino's EntityStore. */ +public class SemanticModelOperationDispatcher extends OperationDispatcher + implements SemanticModelDispatcher { + + private final CatalogManager catalogManager; + private final SchemaDispatcher schemaDispatcher; + private final ManagedSemanticModelOperations managedOperations; + + /** + * Creates a Semantic Model operation dispatcher. + * + * @param catalogManager The catalog manager. + * @param schemaDispatcher The internal schema dispatcher used for parent validation. + * @param store The EntityStore used for Semantic Model persistence. + * @param idGenerator The stable entity ID generator. + * @param secretManager The secret manager required by the operation dispatcher base class. + */ + public SemanticModelOperationDispatcher( + CatalogManager catalogManager, + SchemaDispatcher schemaDispatcher, + EntityStore store, + IdGenerator idGenerator, + SecretManager secretManager) { + super(catalogManager, store, idGenerator, secretManager); + this.catalogManager = catalogManager; + this.schemaDispatcher = schemaDispatcher; + this.managedOperations = new ManagedSemanticModelOperations(store, idGenerator); + } + + @Override + public NameIdentifier[] listSemanticModels(Namespace namespace) throws NoSuchSchemaException { + checkRelationalCatalog(namespace); + NameIdentifier schemaIdent = NameIdentifier.of(namespace.levels()); + schemaDispatcher.loadSchema(schemaIdent); + return TreeLockUtils.doWithTreeLock( + schemaIdent, LockType.READ, () -> managedOperations.listSemanticModels(namespace)); + } + + @Override + public SemanticModel loadSemanticModel(NameIdentifier ident) throws NoSuchSemanticModelException { + checkRelationalCatalog(ident.namespace()); + if (!schemaDispatcher.schemaExists(schemaIdentifier(ident))) { Review Comment: **[Plausible correctness, not novel to this PR]** `loadSemanticModel`, `alterSemanticModel`, and `dropSemanticModel` check `schemaDispatcher.schemaExists()` outside the tree lock, then perform the actual guarded operation inside a separately-acquired tree lock, leaving a TOCTOU window between the check and the locked operation. Thread A calls `loadSemanticModel(ident)`; `schemaExists()` returns true. Before Thread A acquires the READ tree lock, Thread B concurrently drops the parent schema. Thread A proceeds past the now-stale existence check into `managedOperations.loadSemanticModel(ident)` against a schema that no longer exists. This mirrors `FunctionOperationDispatcher`'s identical existing pattern, so it isn't new here, but it's the same real race replicated onto a new resource type rather than fixed at the shared `TreeLockUtils`/dispatcher level. ########## core/src/main/java/org/apache/gravitino/catalog/ManagedSemanticModelOperations.java: ########## @@ -0,0 +1,103 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.gravitino.catalog; + +import java.util.Map; +import javax.annotation.Nullable; +import org.apache.gravitino.EntityStore; +import org.apache.gravitino.NameIdentifier; +import org.apache.gravitino.Namespace; +import org.apache.gravitino.exceptions.IllegalSemanticModelException; +import org.apache.gravitino.exceptions.NoSuchSchemaException; +import org.apache.gravitino.exceptions.NoSuchSemanticModelException; +import org.apache.gravitino.exceptions.SemanticModelAlreadyExistsException; +import org.apache.gravitino.semantic.SemanticModel; +import org.apache.gravitino.semantic.SemanticModelCatalog; +import org.apache.gravitino.semantic.SemanticModelChange; +import org.apache.gravitino.semantic.SemanticModelDefinition; +import org.apache.gravitino.storage.IdGenerator; + +/** + * Provides storage-level Semantic Model operations backed by Gravitino's {@link EntityStore}. + * + * <p>The framework establishes the managed-operation boundary. Semantic Model entity persistence + * and lifecycle implementations will be added in a follow-up change. + */ +public class ManagedSemanticModelOperations implements SemanticModelCatalog { + + @SuppressWarnings("UnusedVariable") + private final EntityStore store; Review Comment: **[Plausible / minor]** `EntityStore` and `IdGenerator` are threaded through the constructor and stored as fields solely to be marked `@SuppressWarnings("UnusedVariable")`, even though every method in this class is currently a stub that always throws `UnsupportedOperationException` and never reads them. Not a functional bug, but it pre-commits to a constructor/field shape (and forces `TestManagedSemanticModelOperations` to mock both dependencies) ahead of the real persistence design being settled in the follow-up change; if that follow-up needs a different shape (e.g. a dedicated `SemanticModelEntity` builder or batched writes), this wiring will need to be revisited anyway. ########## core/src/main/java/org/apache/gravitino/catalog/CapabilityHelpers.java: ########## @@ -157,7 +157,8 @@ public static Namespace applyCaseSensitive( || identScope == Capability.Scope.FILESET || identScope == Capability.Scope.TOPIC || identScope == Capability.Scope.MODEL - || identScope == Capability.Scope.FUNCTION) { + || identScope == Capability.Scope.FUNCTION + || identScope == Capability.Scope.SEMANTIC_MODEL) { Review Comment: **[Plausible simplification]** `Capability.Scope` parent-namespace handling is implemented as two manually-maintained if-chains (this one in `applyCaseSensitive(Namespace...)`, and a matching one in `applyCapabilities(Namespace...)` around line 228) that must be kept in lockstep whenever a new schema-scoped `Capability.Scope` value is added. `SEMANTIC_MODEL` was correctly added to both this time, but `MODEL` is already present in only one of the two chains (a pre-existing gap, not touched by this PR) — showing the mechanism has no compiler or test safety net against a scope being added to one chain and missed in the other. A future resource type's `Capability.Scope` value could be added to only one of the two if-chains, silently falling through to `return namespace;` unnormalized in the other, producing case-sensitivity/namespace bugs that only surface when that scope path is exercised — exactly the kind of gap `MODEL` currently has in `applyCapabilities(Namespace...)`. -- 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]
