jerryshao commented on code in PR #12831: URL: https://github.com/apache/gravitino/pull/12831#discussion_r4091124534
########## core/src/main/java/org/apache/gravitino/catalog/SemanticModelValidator.java: ########## @@ -0,0 +1,350 @@ +/* + * 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.HashMap; +import java.util.Map; +import javax.annotation.Nullable; +import org.apache.gravitino.NameIdentifier; +import org.apache.gravitino.exceptions.IllegalSemanticModelException; +import org.apache.gravitino.semantic.AIContext; +import org.apache.gravitino.semantic.AIContextObject; +import org.apache.gravitino.semantic.CustomExtension; +import org.apache.gravitino.semantic.Dataset; +import org.apache.gravitino.semantic.DialectExpression; +import org.apache.gravitino.semantic.Expression; +import org.apache.gravitino.semantic.Field; +import org.apache.gravitino.semantic.Metric; +import org.apache.gravitino.semantic.Relationship; +import org.apache.gravitino.semantic.SemanticModelDefinition; + +/** + * Validates Semantic Model writes using Gravitino's Java value model. + * + * <p>At implementation time, Apache Ossie commit {@code 88e0011148283302c9a04cd0287e00e0b9d87354}, + * whose core specification version is {@code 0.2.0.dev0}, did not publish a reusable Java SDK or + * general-purpose Java validator artifact. The Java schema validation in that upstream tree was + * converter-specific. Gravitino therefore implements the applicable structural and model-local + * rules directly in Java. If a future Ossie release publishes a compatible Java SDK or validator, + * Gravitino should evaluate replacing this implementation with that upstream library. + * + * <p>Validation is deterministic and performs no catalog I/O. Catalog-backed source validation, + * including existence, columns, and authorization, must be completed by the caller before this + * validator is invoked. SQL expression semantics, transitive View semantics, and query engine + * compatibility are outside this validator's scope. + */ +final class SemanticModelValidator { + + private SemanticModelValidator() {} + + static void validateDefinition(@Nullable SemanticModelDefinition definition) { + if (definition == null) { + throw invalid("$", "definition must not be null"); + } + + validateAIContext(definition.aiContext(), "aiContext"); + + Dataset[] datasets = definition.datasets(); + if (datasets == null || datasets.length == 0) { + throw invalid("datasets", "must not be null or empty"); + } + + Map<String, String> datasetNames = new HashMap<>(); + for (int index = 0; index < datasets.length; index++) { + validateDataset(datasets[index], "datasets[" + index + "]", datasetNames); + } + + validateRelationships(definition.relationships(), datasetNames); + validateMetrics(definition.metrics()); + validateCustomExtensions(definition.customExtensions(), "customExtensions"); + } + + // TODO(#12594): Validate source existence, columns, and authorization in the caller before + // invoking this definition-only validator. + static void validateForWrite( + NameIdentifier semanticModelIdent, @Nullable SemanticModelDefinition definition) { + if (semanticModelIdent == null || semanticModelIdent.namespace().length() != 3) { + throw invalid("$", "Semantic Model identifier must use metalake.catalog.schema.name"); + } + validateDefinition(definition); + } Review Comment: [Question] Can the identifier check at lines 82-84 ever fire through the production caller? `SemanticModelOperationDispatcher.createSemanticModel` (`SemanticModelOperationDispatcher.java:91-98`) runs `checkRelationalCatalog(ident.namespace())` and `schemaDispatcher.loadSchema(schemaIdentifier(ident))` before `managedOperations.createSemanticModel` ever reaches this validator. `checkRelationalCatalog` calls `namespace.level(0)` and `namespace.level(1)` (`SemanticModelOperationDispatcher.java:124-125`), and `Namespace.level(int)` (`api/src/main/java/org/apache/gravitino/Namespace.java:108-111`) rejects out-of-range positions — so a 3-level-or-shorter identifier fails there with a namespace error, never with `Semantic Model identifier must use metalake.catalog.schema.name`. With a longer namespace, `loadSchema` on the over-long schema identifier fails first instead. If the intent is that a caller gets this message, the check probably belongs at the top of the dispatcher's entry points, before `checkRelationalCatalog`; otherwise it can go, since `NameIdentifierUtil.checkSemanticModel` in the store already guards the persistence path. Verified by: read the dispatcher's create path end to end and `Namespace.level`; `TestSemanticModelValidator.java:146-155` exercises this only by calling `validateForWrite` directly, never through the dispatcher. ########## core/src/main/java/org/apache/gravitino/catalog/SemanticModelValidator.java: ########## @@ -0,0 +1,350 @@ +/* + * 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.HashMap; +import java.util.Map; +import javax.annotation.Nullable; +import org.apache.gravitino.NameIdentifier; +import org.apache.gravitino.exceptions.IllegalSemanticModelException; +import org.apache.gravitino.semantic.AIContext; +import org.apache.gravitino.semantic.AIContextObject; +import org.apache.gravitino.semantic.CustomExtension; +import org.apache.gravitino.semantic.Dataset; +import org.apache.gravitino.semantic.DialectExpression; +import org.apache.gravitino.semantic.Expression; +import org.apache.gravitino.semantic.Field; +import org.apache.gravitino.semantic.Metric; +import org.apache.gravitino.semantic.Relationship; +import org.apache.gravitino.semantic.SemanticModelDefinition; + +/** + * Validates Semantic Model writes using Gravitino's Java value model. + * + * <p>At implementation time, Apache Ossie commit {@code 88e0011148283302c9a04cd0287e00e0b9d87354}, + * whose core specification version is {@code 0.2.0.dev0}, did not publish a reusable Java SDK or + * general-purpose Java validator artifact. The Java schema validation in that upstream tree was + * converter-specific. Gravitino therefore implements the applicable structural and model-local + * rules directly in Java. If a future Ossie release publishes a compatible Java SDK or validator, + * Gravitino should evaluate replacing this implementation with that upstream library. + * + * <p>Validation is deterministic and performs no catalog I/O. Catalog-backed source validation, + * including existence, columns, and authorization, must be completed by the caller before this + * validator is invoked. SQL expression semantics, transitive View semantics, and query engine + * compatibility are outside this validator's scope. + */ +final class SemanticModelValidator { + + private SemanticModelValidator() {} + + static void validateDefinition(@Nullable SemanticModelDefinition definition) { + if (definition == null) { + throw invalid("$", "definition must not be null"); + } + + validateAIContext(definition.aiContext(), "aiContext"); + + Dataset[] datasets = definition.datasets(); + if (datasets == null || datasets.length == 0) { + throw invalid("datasets", "must not be null or empty"); + } + + Map<String, String> datasetNames = new HashMap<>(); + for (int index = 0; index < datasets.length; index++) { + validateDataset(datasets[index], "datasets[" + index + "]", datasetNames); + } + + validateRelationships(definition.relationships(), datasetNames); + validateMetrics(definition.metrics()); + validateCustomExtensions(definition.customExtensions(), "customExtensions"); + } + + // TODO(#12594): Validate source existence, columns, and authorization in the caller before + // invoking this definition-only validator. + static void validateForWrite( + NameIdentifier semanticModelIdent, @Nullable SemanticModelDefinition definition) { + if (semanticModelIdent == null || semanticModelIdent.namespace().length() != 3) { + throw invalid("$", "Semantic Model identifier must use metalake.catalog.schema.name"); + } + validateDefinition(definition); + } + + private static void validateDataset( + @Nullable Dataset dataset, String path, Map<String, String> datasetNames) { + if (dataset == null) { + throw invalid(path, "must not be null"); + } + + String namePath = path + ".name"; + validateRequiredString(dataset.name(), namePath); + validateUniqueName(dataset.name(), namePath, "dataset", datasetNames); + validateSource(dataset.source(), path + ".source"); + validateOptionalColumnNames(dataset.primaryKey(), path + ".primaryKey"); + validateUniqueKeys(dataset.uniqueKeys(), path + ".uniqueKeys"); + validateAIContext(dataset.aiContext(), path + ".aiContext"); + validateFields(dataset.fields(), path + ".fields"); + validateCustomExtensions(dataset.customExtensions(), path + ".customExtensions"); + } + + private static void validateSource(@Nullable NameIdentifier source, String path) { + if (source == null) { + throw invalid(path, "must not be null"); + } + if (source.namespace().length() != 2) { + throw invalid( + path, "must contain exactly catalog.schema.name, but was '" + source.toString() + "'"); + } + } + + private static void validateUniqueKeys(@Nullable String[][] uniqueKeys, String path) { + if (uniqueKeys == null) { + return; + } + + for (int index = 0; index < uniqueKeys.length; index++) { + String keyPath = path + "[" + index + "]"; + String[] uniqueKey = uniqueKeys[index]; + if (uniqueKey == null || uniqueKey.length == 0) { + throw invalid(keyPath, "must not be null or empty"); + } + validateColumnNames(uniqueKey, keyPath, false); + } + } + + private static void validateFields(@Nullable Field[] fields, String path) { + if (fields == null) { + return; + } + + Map<String, String> fieldNames = new HashMap<>(); + for (int index = 0; index < fields.length; index++) { + String fieldPath = path + "[" + index + "]"; + Field field = fields[index]; + if (field == null) { + throw invalid(fieldPath, "must not be null"); + } + + String namePath = fieldPath + ".name"; + validateRequiredString(field.name(), namePath); + validateUniqueName(field.name(), namePath, "field", fieldNames); + validateExpression(field.expression(), fieldPath + ".expression"); + validateAIContext(field.aiContext(), fieldPath + ".aiContext"); + validateCustomExtensions(field.customExtensions(), fieldPath + ".customExtensions"); + } + } + + private static void validateRelationships( + @Nullable Relationship[] relationships, Map<String, String> datasetNames) { + if (relationships == null) { + return; + } + + Map<String, String> relationshipNames = new HashMap<>(); + for (int index = 0; index < relationships.length; index++) { + String path = "relationships[" + index + "]"; + Relationship relationship = relationships[index]; + if (relationship == null) { + throw invalid(path, "must not be null"); + } + + String namePath = path + ".name"; + validateRequiredString(relationship.name(), namePath); + validateUniqueName(relationship.name(), namePath, "relationship", relationshipNames); + validateEndpoint(relationship.from(), path + ".from", datasetNames); + validateEndpoint(relationship.to(), path + ".to", datasetNames); + + String[] fromColumns = relationship.fromColumns(); + String[] toColumns = relationship.toColumns(); + validateColumnNames(fromColumns, path + ".fromColumns", false); + validateColumnNames(toColumns, path + ".toColumns", false); + if (fromColumns.length != toColumns.length) { + throw invalid( + path + ".toColumns", + "must contain " + + fromColumns.length + + " columns to match " + + path + + ".fromColumns, but contained " + + toColumns.length); + } + validateAIContext(relationship.aiContext(), path + ".aiContext"); + validateCustomExtensions(relationship.customExtensions(), path + ".customExtensions"); + } + } + + private static void validateEndpoint( + @Nullable String endpoint, String path, Map<String, String> datasetNames) { + validateRequiredString(endpoint, path); + if (!datasetNames.containsKey(endpoint)) { + throw invalid( + path, + "unknown dataset '" + + endpoint + + "'; relationship endpoints must reference datasets in the same model"); + } + } + + private static void validateMetrics(@Nullable Metric[] metrics) { + if (metrics == null) { + return; + } + + Map<String, String> metricNames = new HashMap<>(); + for (int index = 0; index < metrics.length; index++) { + String path = "metrics[" + index + "]"; + Metric metric = metrics[index]; + if (metric == null) { + throw invalid(path, "must not be null"); + } + + String namePath = path + ".name"; + validateRequiredString(metric.name(), namePath); + validateUniqueName(metric.name(), namePath, "metric", metricNames); + validateExpression(metric.expression(), path + ".expression"); + validateAIContext(metric.aiContext(), path + ".aiContext"); + validateCustomExtensions(metric.customExtensions(), path + ".customExtensions"); + } + } + + private static void validateExpression(@Nullable Expression expression, String path) { + if (expression == null) { + throw invalid(path, "must not be null"); + } + + DialectExpression[] dialectExpressions = expression.dialects(); + if (dialectExpressions == null || dialectExpressions.length == 0) { + throw invalid(path + ".dialects", "must not be null or empty"); + } + + Map<String, String> dialectPaths = new HashMap<>(); + for (int index = 0; index < dialectExpressions.length; index++) { + String dialectExpressionPath = path + ".dialects[" + index + "]"; + DialectExpression dialectExpression = dialectExpressions[index]; + if (dialectExpression == null) { + throw invalid(dialectExpressionPath, "must not be null"); + } + + String dialect = dialectExpression.dialect(); + String dialectPath = dialectExpressionPath + ".dialect"; + validateRequiredString(dialect, dialectPath); + String firstPath = dialectPaths.putIfAbsent(dialect, dialectPath); + if (firstPath != null) { + throw invalid( + dialectPath, "duplicate dialect '" + dialect + "'; first declared at " + firstPath); + } + validateRequiredString(dialectExpression.expression(), dialectExpressionPath + ".expression"); + } Review Comment: [Important] Most of this validator restates invariants the `api` value objects already enforce, so those branches cannot fire for any object built through the public builders, and the error contract they define is not the one callers will hit. This block is the clearest case: `Expression.Builder.build()` (`api/src/main/java/org/apache/gravitino/semantic/Expression.java:119-129`) already rejects null/empty `dialects`, null elements, **and duplicate dialects** via its own `seenDialects` set. So the duplicate-dialect error at lines 246-250 is unreachable — `TestSemanticModelValidator.java:297-300` has to `mock(Expression.class)` to produce two identical `DialectExpression`s. The same holds across the class: `SemanticModelDefinition.build()` (`:266-273`) rejects null/empty `datasets` and null elements in every array; `Dataset.build()` (`:325-334`) rejects empty names, null sources, empty/null `primaryKey` elements and malformed `uniqueKeys`; `Relationship.build()` (`:295-311`) rejects empty names/endpoints and already enforces `fromColumns.length == toColumns.length`; `Field.build()` (`:325-331`) and `Metric.build()` (`:264-270`) reject empty names and null expressions; `CustomExtension.build()` (`:139-143`) rejects null `vendorName`/`data`; `AIContext` has only two private constructors, each nulling the other variant, so lines 262-264 cannot fire; `AIContextObject.build()` (`:229-235`) already rejects null `synonyms`/`examples` elements and null `additionalProperties`, and `immutableAdditionalProperties` (`:238-245`) rejects null property names. What is left that the value model genuinely cannot express is small and valuable: dataset/field/metric/relationship name uniqueness, relationship endpoint resolution against the declared datasets, and the `catalog.schema.name` source shape. Two consequences of keeping the rest: roughly 200 of these 350 lines are dead, and — once the REST layer converts a DTO into these objects — a malformed request will surface as the builder's `IllegalArgumentException`, not the `IllegalSemanticModelException` this validator promises, so the promised error type is only delivered for the handful of checks that are actually reachable. Suggest trimming to the model-local rules, or, if this is deliberate defense-in-depth against instances built by some future non-builder path (deserialization?), saying so in the class Javadoc so the next reader does not have to re-derive it. Verified by: read every `build()` in `api/src/main/java/org/apache/gravitino/semantic/` on this commit and matched each one against the corresponding branch here; confirmed the mock-only test reach at `TestSemanticModelValidator.java:186, 206-216, 245-254, 277-282, 297, 314-321, 374`. ########## core/src/test/java/org/apache/gravitino/catalog/TestManagedSemanticModelOperationsJDBC.java: ########## @@ -0,0 +1,124 @@ +/* + * 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 static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotSame; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.io.IOException; +import java.util.Map; +import java.util.concurrent.atomic.AtomicInteger; +import org.apache.commons.lang3.reflect.FieldUtils; +import org.apache.gravitino.Config; +import org.apache.gravitino.Configs; +import org.apache.gravitino.Entity; +import org.apache.gravitino.GravitinoEnv; +import org.apache.gravitino.NameIdentifier; +import org.apache.gravitino.Namespace; +import org.apache.gravitino.cache.NoOpsCache; +import org.apache.gravitino.semantic.CustomExtension; +import org.apache.gravitino.semantic.Dataset; +import org.apache.gravitino.semantic.Metric; +import org.apache.gravitino.semantic.Relationship; +import org.apache.gravitino.semantic.SemanticModel; +import org.apache.gravitino.semantic.SemanticModelDefinition; +import org.apache.gravitino.storage.RandomIdGenerator; +import org.apache.gravitino.storage.relational.RelationalEntityStore; +import org.apache.gravitino.storage.relational.TestJDBCBackend; +import org.apache.gravitino.utils.NamespaceUtil; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.TestTemplate; + +/** Verifies managed create and load through a real relational persistence backend. */ +public class TestManagedSemanticModelOperationsJDBC extends TestJDBCBackend { + + private final AtomicInteger writeValidationCount = new AtomicInteger(); + + private Config previousConfig; + private NameIdentifier modelIdent; + private ManagedSemanticModelOperations operations; + + @BeforeAll + public void captureEnvironmentConfig() { + previousConfig = GravitinoEnv.getInstance().config(); + } + + @BeforeEach + public void prepareManagedOperations() throws IOException, IllegalAccessException { + writeValidationCount.set(0); + String metalake = "managed_semantic_model_metalake"; + String catalog = "managed_semantic_model_catalog"; + String schema = "managed_semantic_model_schema"; + createAndInsertMakeLake(metalake); + createAndInsertCatalog(metalake, catalog); + createAndInsertSchema(metalake, catalog, schema); + + Namespace namespace = NamespaceUtil.ofSemanticModel(metalake, catalog, schema); + modelIdent = NameIdentifier.of(namespace, "sales_model"); + + Config config = new Config(false) {}; + config.set(Configs.CACHE_ENABLED, false); + RelationalEntityStore store = new RelationalEntityStore(); + FieldUtils.writeField(store, "backend", backend, true); + FieldUtils.writeField(store, "cache", new NoOpsCache(config), true); + operations = + new ManagedSemanticModelOperations( + store, + RandomIdGenerator.INSTANCE, + (ident, definition) -> writeValidationCount.incrementAndGet()); + } + + @AfterEach + public void restoreEnvironmentConfig() throws IllegalAccessException { + FieldUtils.writeField(GravitinoEnv.getInstance(), "config", previousConfig, true); + } Review Comment: [Nit] This capture/restore pair does not do anything. `previousConfig` is read at line 62 and written straight back here; nothing in the test or the fixture ever changes `GravitinoEnv`'s `config` field — the `Config` built at lines 79-81 is local and only feeds `new NoOpsCache(config)`. `grep -n GravitinoEnv` on `TestJDBCBackend` returns no matches, so the base class does not touch it either. A reflective write to a global singleton reads like real state management, so removing the field, `@BeforeAll` and `@AfterEach` would make the fixture easier to trust. (If it is here because something in the store path *does* read `GravitinoEnv.getInstance().config()`, a one-line comment saying so would be just as good.) Verified by: read this file end to end and grepped `TestJDBCBackend` for `GravitinoEnv`. ########## core/build.gradle.kts: ########## @@ -43,6 +43,7 @@ dependencies { implementation(libs.concurrent.trees) implementation(libs.guava) implementation(libs.h2db) + implementation(libs.jackson.databind) Review Comment: [Question] Is this dependency needed by this PR? Nothing added here uses Jackson — `git diff origin/main...HEAD | grep -i jackson` matches only this line, and the source-validation code removed in `846d48f` did not use it either (checked `git show c8ec054:.../SemanticModelValidator.java`). `core` main *does* already use `com.fasterxml.jackson.databind` in `LocalJobExecutor`, `JsonAuditFormatter` and `ViewPO`, all pre-existing, so if this is a deliberate fix for a dependency that was previously resolving transitively, that is a good change — it is just unrelated to Semantic Models and worth a line in the PR description so it is not read as a leftover of the reverted source-validation work. Verified by: grepped the full PR diff for Jackson usage and `grep -rl com.fasterxml.jackson.databind core/src/main/java`. ########## core/src/test/java/org/apache/gravitino/catalog/TestManagedSemanticModelOperationsJDBC.java: ########## @@ -0,0 +1,124 @@ +/* + * 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 static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotSame; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.io.IOException; +import java.util.Map; +import java.util.concurrent.atomic.AtomicInteger; +import org.apache.commons.lang3.reflect.FieldUtils; +import org.apache.gravitino.Config; +import org.apache.gravitino.Configs; +import org.apache.gravitino.Entity; +import org.apache.gravitino.GravitinoEnv; +import org.apache.gravitino.NameIdentifier; +import org.apache.gravitino.Namespace; +import org.apache.gravitino.cache.NoOpsCache; +import org.apache.gravitino.semantic.CustomExtension; +import org.apache.gravitino.semantic.Dataset; +import org.apache.gravitino.semantic.Metric; +import org.apache.gravitino.semantic.Relationship; +import org.apache.gravitino.semantic.SemanticModel; +import org.apache.gravitino.semantic.SemanticModelDefinition; +import org.apache.gravitino.storage.RandomIdGenerator; +import org.apache.gravitino.storage.relational.RelationalEntityStore; +import org.apache.gravitino.storage.relational.TestJDBCBackend; +import org.apache.gravitino.utils.NamespaceUtil; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.TestTemplate; + +/** Verifies managed create and load through a real relational persistence backend. */ +public class TestManagedSemanticModelOperationsJDBC extends TestJDBCBackend { + + private final AtomicInteger writeValidationCount = new AtomicInteger(); + + private Config previousConfig; + private NameIdentifier modelIdent; + private ManagedSemanticModelOperations operations; + + @BeforeAll + public void captureEnvironmentConfig() { + previousConfig = GravitinoEnv.getInstance().config(); + } + + @BeforeEach + public void prepareManagedOperations() throws IOException, IllegalAccessException { + writeValidationCount.set(0); + String metalake = "managed_semantic_model_metalake"; + String catalog = "managed_semantic_model_catalog"; + String schema = "managed_semantic_model_schema"; + createAndInsertMakeLake(metalake); + createAndInsertCatalog(metalake, catalog); + createAndInsertSchema(metalake, catalog, schema); + + Namespace namespace = NamespaceUtil.ofSemanticModel(metalake, catalog, schema); + modelIdent = NameIdentifier.of(namespace, "sales_model"); + + Config config = new Config(false) {}; + config.set(Configs.CACHE_ENABLED, false); + RelationalEntityStore store = new RelationalEntityStore(); + FieldUtils.writeField(store, "backend", backend, true); + FieldUtils.writeField(store, "cache", new NoOpsCache(config), true); + operations = + new ManagedSemanticModelOperations( + store, + RandomIdGenerator.INSTANCE, + (ident, definition) -> writeValidationCount.incrementAndGet()); + } + + @AfterEach + public void restoreEnvironmentConfig() throws IllegalAccessException { + FieldUtils.writeField(GravitinoEnv.getInstance(), "config", previousConfig, true); + } + + @TestTemplate + public void testCreateThenLoadRoundTrip() throws IOException { + Dataset dataset = + Dataset.builder() + .withName("orders") + .withSource(NameIdentifier.of("source_catalog", "source_schema", "orders")) + .withPrimaryKey(new String[0]) + .withUniqueKeys(new String[0][]) + .withCustomExtensions(new CustomExtension[0]) + .build(); + SemanticModelDefinition definition = + SemanticModelDefinition.builder() + .withDatasets(new Dataset[] {dataset}) + .withRelationships(new Relationship[0]) + .withMetrics((Metric[]) null) + .withCustomExtensions(new CustomExtension[0]) + .build(); + + SemanticModel created = + operations.createSemanticModel( + modelIdent, "Persisted model", definition, Map.of("domain", "sales")); + SemanticModel loaded = operations.loadSemanticModel(modelIdent); + + assertNotSame(created, loaded); + assertEquals(created, loaded); + assertEquals(definition, loaded.definition()); + assertEquals(1, writeValidationCount.get()); + assertTrue(backend.exists(modelIdent, Entity.EntityType.SEMANTIC_MODEL)); + } +} Review Comment: [Important] This is the only test that runs against a real relational backend, and it covers just the happy path — so the two user-facing error mappings are verified against a mocked `EntityStore` only, and a mock cannot prove the backend produces the exception being mapped. `ManagedSemanticModelOperations.createSemanticModel` maps `EntityAlreadyExistsException` to `SemanticModelAlreadyExistsException` and `NoSuchEntityException` to `NoSuchSchemaException`. In the real path neither is thrown directly: a duplicate name reaches `SemanticModelMetaService.insertSemanticModel` (`core/src/main/java/org/apache/gravitino/storage/relational/service/SemanticModelMetaService.java:151-155`), whose `catch (RuntimeException re)` hands the wrapped `SQLException` to `ExceptionUtils.checkSQLException` (`.../utils/ExceptionUtils.java:32-38`), which delegates to the per-backend `SQLExceptionConverterFactory` converter. Whether that converter yields `EntityAlreadyExistsException` for the `semantic_model_meta` unique constraint is exactly what `TestManagedSemanticModelOperations.testCreateExceptionMapping` cannot tell you, because it stubs the store. Two more `@TestTemplate`s here would close it: create the same model twice and assert `SemanticModelAlreadyExistsException`; create under a schema that was never inserted and assert `NoSuchSchemaException`. Both run on the same fixture and cost almost nothing. Verified by: traced `store.put` -> `JDBCBackend.insert` (`case SEMANTIC_MODEL`, line 286) -> `SemanticModelMetaService.insertSemanticModel` -> `ExceptionUtils.checkSQLException`, and confirmed this file's only `@TestTemplate` is `testCreateThenLoadRoundTrip`. ########## core/src/main/java/org/apache/gravitino/catalog/SemanticModelValidator.java: ########## @@ -0,0 +1,350 @@ +/* + * 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.HashMap; +import java.util.Map; +import javax.annotation.Nullable; +import org.apache.gravitino.NameIdentifier; +import org.apache.gravitino.exceptions.IllegalSemanticModelException; +import org.apache.gravitino.semantic.AIContext; +import org.apache.gravitino.semantic.AIContextObject; +import org.apache.gravitino.semantic.CustomExtension; +import org.apache.gravitino.semantic.Dataset; +import org.apache.gravitino.semantic.DialectExpression; +import org.apache.gravitino.semantic.Expression; +import org.apache.gravitino.semantic.Field; +import org.apache.gravitino.semantic.Metric; +import org.apache.gravitino.semantic.Relationship; +import org.apache.gravitino.semantic.SemanticModelDefinition; + +/** + * Validates Semantic Model writes using Gravitino's Java value model. + * + * <p>At implementation time, Apache Ossie commit {@code 88e0011148283302c9a04cd0287e00e0b9d87354}, + * whose core specification version is {@code 0.2.0.dev0}, did not publish a reusable Java SDK or + * general-purpose Java validator artifact. The Java schema validation in that upstream tree was + * converter-specific. Gravitino therefore implements the applicable structural and model-local + * rules directly in Java. If a future Ossie release publishes a compatible Java SDK or validator, + * Gravitino should evaluate replacing this implementation with that upstream library. + * + * <p>Validation is deterministic and performs no catalog I/O. Catalog-backed source validation, + * including existence, columns, and authorization, must be completed by the caller before this + * validator is invoked. SQL expression semantics, transitive View semantics, and query engine + * compatibility are outside this validator's scope. + */ +final class SemanticModelValidator { + + private SemanticModelValidator() {} + + static void validateDefinition(@Nullable SemanticModelDefinition definition) { + if (definition == null) { + throw invalid("$", "definition must not be null"); + } + + validateAIContext(definition.aiContext(), "aiContext"); + + Dataset[] datasets = definition.datasets(); + if (datasets == null || datasets.length == 0) { + throw invalid("datasets", "must not be null or empty"); + } + + Map<String, String> datasetNames = new HashMap<>(); + for (int index = 0; index < datasets.length; index++) { + validateDataset(datasets[index], "datasets[" + index + "]", datasetNames); + } + + validateRelationships(definition.relationships(), datasetNames); + validateMetrics(definition.metrics()); + validateCustomExtensions(definition.customExtensions(), "customExtensions"); + } + + // TODO(#12594): Validate source existence, columns, and authorization in the caller before + // invoking this definition-only validator. + static void validateForWrite( + NameIdentifier semanticModelIdent, @Nullable SemanticModelDefinition definition) { + if (semanticModelIdent == null || semanticModelIdent.namespace().length() != 3) { + throw invalid("$", "Semantic Model identifier must use metalake.catalog.schema.name"); + } + validateDefinition(definition); + } + + private static void validateDataset( + @Nullable Dataset dataset, String path, Map<String, String> datasetNames) { + if (dataset == null) { + throw invalid(path, "must not be null"); + } + + String namePath = path + ".name"; + validateRequiredString(dataset.name(), namePath); + validateUniqueName(dataset.name(), namePath, "dataset", datasetNames); + validateSource(dataset.source(), path + ".source"); + validateOptionalColumnNames(dataset.primaryKey(), path + ".primaryKey"); + validateUniqueKeys(dataset.uniqueKeys(), path + ".uniqueKeys"); + validateAIContext(dataset.aiContext(), path + ".aiContext"); + validateFields(dataset.fields(), path + ".fields"); + validateCustomExtensions(dataset.customExtensions(), path + ".customExtensions"); + } + + private static void validateSource(@Nullable NameIdentifier source, String path) { + if (source == null) { + throw invalid(path, "must not be null"); + } + if (source.namespace().length() != 2) { + throw invalid( + path, "must contain exactly catalog.schema.name, but was '" + source.toString() + "'"); + } + } + + private static void validateUniqueKeys(@Nullable String[][] uniqueKeys, String path) { + if (uniqueKeys == null) { + return; + } + + for (int index = 0; index < uniqueKeys.length; index++) { + String keyPath = path + "[" + index + "]"; + String[] uniqueKey = uniqueKeys[index]; + if (uniqueKey == null || uniqueKey.length == 0) { + throw invalid(keyPath, "must not be null or empty"); + } + validateColumnNames(uniqueKey, keyPath, false); + } + } + + private static void validateFields(@Nullable Field[] fields, String path) { + if (fields == null) { + return; + } + + Map<String, String> fieldNames = new HashMap<>(); + for (int index = 0; index < fields.length; index++) { + String fieldPath = path + "[" + index + "]"; + Field field = fields[index]; + if (field == null) { + throw invalid(fieldPath, "must not be null"); + } + + String namePath = fieldPath + ".name"; + validateRequiredString(field.name(), namePath); + validateUniqueName(field.name(), namePath, "field", fieldNames); + validateExpression(field.expression(), fieldPath + ".expression"); + validateAIContext(field.aiContext(), fieldPath + ".aiContext"); + validateCustomExtensions(field.customExtensions(), fieldPath + ".customExtensions"); + } + } + + private static void validateRelationships( + @Nullable Relationship[] relationships, Map<String, String> datasetNames) { + if (relationships == null) { + return; + } + + Map<String, String> relationshipNames = new HashMap<>(); + for (int index = 0; index < relationships.length; index++) { + String path = "relationships[" + index + "]"; + Relationship relationship = relationships[index]; + if (relationship == null) { + throw invalid(path, "must not be null"); + } + + String namePath = path + ".name"; + validateRequiredString(relationship.name(), namePath); + validateUniqueName(relationship.name(), namePath, "relationship", relationshipNames); + validateEndpoint(relationship.from(), path + ".from", datasetNames); + validateEndpoint(relationship.to(), path + ".to", datasetNames); + + String[] fromColumns = relationship.fromColumns(); + String[] toColumns = relationship.toColumns(); + validateColumnNames(fromColumns, path + ".fromColumns", false); + validateColumnNames(toColumns, path + ".toColumns", false); + if (fromColumns.length != toColumns.length) { + throw invalid( + path + ".toColumns", + "must contain " + + fromColumns.length + + " columns to match " + + path + + ".fromColumns, but contained " + + toColumns.length); + } + validateAIContext(relationship.aiContext(), path + ".aiContext"); + validateCustomExtensions(relationship.customExtensions(), path + ".customExtensions"); + } + } + + private static void validateEndpoint( + @Nullable String endpoint, String path, Map<String, String> datasetNames) { + validateRequiredString(endpoint, path); + if (!datasetNames.containsKey(endpoint)) { + throw invalid( + path, + "unknown dataset '" + + endpoint + + "'; relationship endpoints must reference datasets in the same model"); + } + } + + private static void validateMetrics(@Nullable Metric[] metrics) { + if (metrics == null) { + return; + } + + Map<String, String> metricNames = new HashMap<>(); + for (int index = 0; index < metrics.length; index++) { + String path = "metrics[" + index + "]"; + Metric metric = metrics[index]; + if (metric == null) { + throw invalid(path, "must not be null"); + } + + String namePath = path + ".name"; + validateRequiredString(metric.name(), namePath); + validateUniqueName(metric.name(), namePath, "metric", metricNames); + validateExpression(metric.expression(), path + ".expression"); + validateAIContext(metric.aiContext(), path + ".aiContext"); + validateCustomExtensions(metric.customExtensions(), path + ".customExtensions"); + } + } + + private static void validateExpression(@Nullable Expression expression, String path) { + if (expression == null) { + throw invalid(path, "must not be null"); + } + + DialectExpression[] dialectExpressions = expression.dialects(); + if (dialectExpressions == null || dialectExpressions.length == 0) { + throw invalid(path + ".dialects", "must not be null or empty"); + } + + Map<String, String> dialectPaths = new HashMap<>(); + for (int index = 0; index < dialectExpressions.length; index++) { + String dialectExpressionPath = path + ".dialects[" + index + "]"; + DialectExpression dialectExpression = dialectExpressions[index]; + if (dialectExpression == null) { + throw invalid(dialectExpressionPath, "must not be null"); + } + + String dialect = dialectExpression.dialect(); + String dialectPath = dialectExpressionPath + ".dialect"; + validateRequiredString(dialect, dialectPath); + String firstPath = dialectPaths.putIfAbsent(dialect, dialectPath); + if (firstPath != null) { + throw invalid( + dialectPath, "duplicate dialect '" + dialect + "'; first declared at " + firstPath); + } + validateRequiredString(dialectExpression.expression(), dialectExpressionPath + ".expression"); + } + } + + private static void validateAIContext(@Nullable AIContext aiContext, String path) { + if (aiContext == null) { + return; + } + + String text = aiContext.text(); + AIContextObject object = aiContext.object(); + if ((text == null) == (object == null)) { + throw invalid(path, "must contain exactly one string or object value"); + } + if (object == null) { + return; + } + + validateStringElements(object.synonyms(), path + ".synonyms"); + validateStringElements(object.examples(), path + ".examples"); + if (object.additionalProperties() == null) { + throw invalid(path + ".additionalProperties", "must not be null"); + } + for (String name : object.additionalProperties().keySet()) { + if (name == null) { + throw invalid(path + ".additionalProperties", "property name must not be null"); + } + } + } + + private static void validateColumnNames( + @Nullable String[] columns, String path, boolean allowEmpty) { + if (columns == null || (!allowEmpty && columns.length == 0)) { + throw invalid(path, allowEmpty ? "must not be null" : "must not be null or empty"); + } + for (int index = 0; index < columns.length; index++) { + validateRequiredString(columns[index], path + "[" + index + "]"); + } + } Review Comment: [Nit] `allowEmpty` is dead. All three call sites pass `false` — line 126 (`validateUniqueKeys`) and lines 174-175 (`fromColumns`/`toColumns`) — so the ternary at line 284 can never produce its must-not-be-null message, and `columns == null || (!allowEmpty && ...)` collapses to the unconditional form. Inlining it would leave this identical to `validateOptionalColumnNames` plus one emptiness check. Verified by: `grep -n validateColumnNames` on this file — lines 126, 174, 175 are the only callers, all with `false`. -- 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]
