jerryshao commented on code in PR #12625:
URL: https://github.com/apache/gravitino/pull/12625#discussion_r4118393397
##########
server/src/main/java/org/apache/gravitino/server/web/rest/ExceptionHandlers.java:
##########
@@ -990,6 +1005,46 @@ public Response handle(OperationType op, String function,
String schema, Excepti
}
}
+ private static class SemanticModelExceptionHandler extends
BaseExceptionHandler {
+ private static final ExceptionHandler INSTANCE = new
SemanticModelExceptionHandler();
+
+ private static String getSemanticModelErrorMsg(
+ String semanticModel, String operation, String schema, String reason) {
+ return String.format(
+ "Failed to operate Semantic Model(s)%s operation [%s] under schema
[%s], reason [%s]",
+ semanticModel, operation, schema, reason);
+ }
+
+ @Override
+ public Response handle(OperationType op, String semanticModel, String
schema, Exception e) {
+ String formatted = StringUtil.isBlank(semanticModel) ? "" : " [" +
semanticModel + "]";
+ String errorMsg = getSemanticModelErrorMsg(formatted, op.name(), schema,
getErrorMsg(e));
+ LOG.warn(errorMsg, e);
+
+ if (e instanceof IllegalArgumentException) {
+ return Utils.illegalArguments(errorMsg, e);
+
+ } else if (e instanceof NotFoundException) {
+ return Utils.notFound(errorMsg, e);
+
+ } else if (e instanceof SemanticModelAlreadyExistsException) {
+ return Utils.alreadyExists(errorMsg, e);
+
+ } else if (e instanceof ForbiddenException) {
+ return Utils.forbidden(errorMsg, e);
+
+ } else if (e instanceof UnsupportedOperationException) {
+ return Utils.unsupportedOperation(errorMsg, e);
+
+ } else if (e instanceof ConnectionFailedException) {
+ return Utils.connectionFailed(errorMsg, e);
+
+ } else {
Review Comment:
[Important] `NotInUseException` is not mapped, so a disabled metalake yields
500 instead of 409.
Both new endpoints call
`SemanticModelOperationDispatcher.checkRelationalCatalog()`
(`core/src/main/java/org/apache/gravitino/catalog/SemanticModelOperationDispatcher.java:106-108`)
-> `CatalogManager.loadCatalog()`
(`core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java:786-796`)
-> `baseCatalog.checkMetalakeInUse()`
(`core/src/main/java/org/apache/gravitino/connector/BaseCatalog.java:257-265`),
which throws `MetalakeNotInUseException`.
`createSemanticModel`/`loadSemanticModel` additionally go through
`schemaDispatcher.loadSchema`/`schemaExists`, which can raise
`CatalogNotInUseException`.
`MetalakeNotInUseException extends NotInUseException extends
GravitinoRuntimeException`, so it matches none of the six branches here and
falls through to `super.handle(...)`. `BaseExceptionHandler` (lines 1216-1251)
only special-cases `ConnectionFailedException`, `OptimisticLockException`,
`UnmodifiableStatisticException` and `UnsupportedOperationException`, then
returns `Utils.internalError(...)` — HTTP 500 with `INTERNAL_ERROR_CODE` and a
stack trace. Every other resource returns 409 `NOT_IN_USE_CODE` via
`Utils.notInUse`
(`server-common/src/main/java/org/apache/gravitino/server/web/Utils.java:163-168`);
`grep -n NotInUseException ExceptionHandlers.java` shows the branch in all 22
sibling handlers (lines 242, 284, 325, 366, 410, 451, 535, 576, 611, 649, 687,
754, 783, 811, 852, 892, 924, 962, 999, 1080, 1118). A client would also fail
to reconstruct `MetalakeNotInUseException` from the error code.
Add the branch before the `else` (`NotInUseException` is already imported at
line 45, so no new import):
```java
} else if (e instanceof NotInUseException) {
return Utils.notInUse(errorMsg, e);
}
```
and a `TestSemanticModelOperations` case asserting 409 +
`ErrorConstants.NOT_IN_USE_CODE` for a `MetalakeNotInUseException` thrown by
the mock dispatcher.
Verified by: read the full handler and `BaseExceptionHandler` fallback in
this file; traced the exception to its throw site through the three core files
cited; confirmed the class hierarchy in
`api/src/main/java/org/apache/gravitino/exceptions/{MetalakeNotInUseException,NotInUseException}.java`
and the status code in `Utils.notInUse`. Not reproduced at runtime — the build
rejects JDK 21.
---
_Generated by [Claude Code](https://claude.ai/code)_
##########
common/src/main/java/org/apache/gravitino/dto/requests/SemanticModelCreateRequest.java:
##########
@@ -0,0 +1,105 @@
+/*
+ * 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.dto.requests;
+
+import com.fasterxml.jackson.annotation.JsonInclude;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.fasterxml.jackson.annotation.JsonPropertyOrder;
+import com.google.common.base.Preconditions;
+import java.util.Map;
+import javax.annotation.Nullable;
+import lombok.Builder;
+import lombok.EqualsAndHashCode;
+import lombok.Getter;
+import lombok.ToString;
+import lombok.extern.jackson.Jacksonized;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.gravitino.dto.semantic.SemanticModelDefinitionDTO;
+import org.apache.gravitino.rest.RESTRequest;
+import org.apache.gravitino.semantic.SemanticModelDefinition;
+
+/** Represents a request to create a Semantic Model. */
+@Getter
+@EqualsAndHashCode
+@ToString
+@Builder
+@Jacksonized
+@JsonInclude(JsonInclude.Include.NON_NULL)
+@JsonPropertyOrder({"name", "comment", "definition", "properties"})
+public class SemanticModelCreateRequest implements RESTRequest {
+
+ @JsonProperty("name")
+ private final String name;
+
+ @Nullable
+ @JsonProperty("comment")
+ private final String comment;
+
+ @JsonProperty("definition")
+ private final SemanticModelDefinitionDTO definition;
+
+ @JsonProperty("properties")
+ private final Map<String, String> properties;
+
+ /** Default constructor for Jackson deserialization. */
+ public SemanticModelCreateRequest() {
+ this(null, null, null, null);
+ }
+
+ /**
+ * Creates a Semantic Model create request.
+ *
+ * @param name The Semantic Model name.
+ * @param comment The comment, or {@code null} if it is not set.
+ * @param definition The required Semantic Model definition.
+ * @param properties The required Gravitino-specific properties.
+ */
+ public SemanticModelCreateRequest(
+ String name,
+ @Nullable String comment,
+ SemanticModelDefinitionDTO definition,
+ Map<String, String> properties) {
+ this.name = name;
+ this.comment = comment;
+ this.definition = definition;
+ this.properties = properties;
+ }
+
+ @Override
+ public void validate() throws IllegalArgumentException {
+ Preconditions.checkArgument(
+ StringUtils.isNotBlank(name), "\"name\" field is required and cannot
be empty");
+ Preconditions.checkArgument(
+ definition != null, "\"definition\" field is required and cannot be
null");
+ Preconditions.checkArgument(
+ properties != null, "\"properties\" field is required and cannot be
null");
Review Comment:
[Question] Is `properties` intended to be a mandatory wire field? This is
stricter than every other Gravitino create API.
Rejecting a null `properties` means an HTTP caller must always send
`"properties": {}`; omitting the key is a 400. No other create request does
this — `SchemaCreateRequest.validate()`
(`common/src/main/java/org/apache/gravitino/dto/requests/SchemaCreateRequest.java:108-111`)
checks only `name`, and `ModelRegisterRequest` likewise leaves `properties`
unvalidated. Since REST request shape is hard to relax once released, it seems
worth settling now.
`SemanticModelOperationDispatcher.createSemanticModel` also asserts non-null
(line 78), so the strictness is consistent with core — but the REST layer could
absorb it by defaulting instead, e.g. pass `properties == null ?
Collections.emptyMap() : properties` and drop this precondition. That keeps the
core contract intact while matching the rest of the API surface. If mandatory
is deliberate, could you note it in the OpenAPI spec when it lands so clients
aren't surprised?
Either way there is no test covering a create request with `properties`
absent from the JSON (the existing cases always pass a map), so the current
behaviour is unpinned.
Verified by: read `validate()` here and compared against
`SchemaCreateRequest.validate()` and `ModelRegisterRequest`; checked the core
precondition at `SemanticModelOperationDispatcher.java:78`; grepped
`TestSemanticModelCreateRequest` and `TestSemanticModelOperations` for an
omitted-`properties` case and found none.
---
_Generated by [Claude Code](https://claude.ai/code)_
##########
server/src/main/java/org/apache/gravitino/server/web/rest/ExceptionHandlers.java:
##########
@@ -990,6 +1005,46 @@ public Response handle(OperationType op, String function,
String schema, Excepti
}
}
+ private static class SemanticModelExceptionHandler extends
BaseExceptionHandler {
+ private static final ExceptionHandler INSTANCE = new
SemanticModelExceptionHandler();
+
+ private static String getSemanticModelErrorMsg(
+ String semanticModel, String operation, String schema, String reason) {
+ return String.format(
+ "Failed to operate Semantic Model(s)%s operation [%s] under schema
[%s], reason [%s]",
+ semanticModel, operation, schema, reason);
+ }
+
+ @Override
+ public Response handle(OperationType op, String semanticModel, String
schema, Exception e) {
+ String formatted = StringUtil.isBlank(semanticModel) ? "" : " [" +
semanticModel + "]";
+ String errorMsg = getSemanticModelErrorMsg(formatted, op.name(), schema,
getErrorMsg(e));
+ LOG.warn(errorMsg, e);
+
+ if (e instanceof IllegalArgumentException) {
+ return Utils.illegalArguments(errorMsg, e);
+
+ } else if (e instanceof NotFoundException) {
+ return Utils.notFound(errorMsg, e);
+
+ } else if (e instanceof SemanticModelAlreadyExistsException) {
+ return Utils.alreadyExists(errorMsg, e);
+
+ } else if (e instanceof ForbiddenException) {
+ return Utils.forbidden(errorMsg, e);
+
+ } else if (e instanceof UnsupportedOperationException) {
+ return Utils.unsupportedOperation(errorMsg, e);
+
+ } else if (e instanceof ConnectionFailedException) {
+ return Utils.connectionFailed(errorMsg, e);
Review Comment:
[Nit] These two branches are redundant — `BaseExceptionHandler` already maps
both identically.
`super.handle(...)` returns `Utils.unsupportedOperation(errorMsg, e)` for
`UnsupportedOperationException` (lines 1244-1247) and
`Utils.connectionFailed(errorMsg, e)` for `ConnectionFailedException` (lines
1226-1229), i.e. the same responses these branches produce. Dropping them
leaves behaviour unchanged and keeps this handler shaped like its siblings
(`FunctionExceptionHandler`, starting at line 971, carries neither). The
existing assertions for 501 and 502 in `TestSemanticModelOperations` pass
either way, since the fallback produces the same status and error code.
Minor, and the reason it is worth a line at all: the handler grew two
branches it does not need while missing the one it does (see the
`NotInUseException` comment below).
Verified by: read both branches here and the full
`BaseExceptionHandler.handle` body at lines 1216-1251; compared against
`FunctionExceptionHandler` in the same file.
---
_Generated by [Claude Code](https://claude.ai/code)_
##########
common/src/main/java/org/apache/gravitino/dto/responses/SemanticModelResponse.java:
##########
@@ -0,0 +1,65 @@
+/*
+ * 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.dto.responses;
+
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.google.common.base.Preconditions;
+import lombok.EqualsAndHashCode;
+import lombok.Getter;
+import lombok.ToString;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.gravitino.dto.semantic.SemanticModelDTO;
+
+/** Represents a response containing one Semantic Model. */
+@Getter
+@EqualsAndHashCode(callSuper = true)
+@ToString
+public class SemanticModelResponse extends BaseResponse {
+
+ @JsonProperty("semanticModel")
+ private final SemanticModelDTO semanticModel;
+
+ /** Default constructor for Jackson deserialization. */
+ public SemanticModelResponse() {
+ super();
+ this.semanticModel = null;
+ }
+
+ /**
+ * Creates a successful Semantic Model response.
+ *
+ * @param semanticModel The Semantic Model DTO.
+ */
+ public SemanticModelResponse(SemanticModelDTO semanticModel) {
+ super(0);
+ this.semanticModel = semanticModel;
+ }
+
+ @Override
+ public void validate() throws IllegalArgumentException {
+ super.validate();
+ Preconditions.checkArgument(semanticModel != null, "semanticModel must not
be null");
+ Preconditions.checkArgument(
+ StringUtils.isNotBlank(semanticModel.name()),
+ "semanticModel 'name' must not be null or empty");
+ semanticModel.definition();
Review Comment:
[Nit] This bare statement validates by side effect only — easy to mistake
for dead code.
`semanticModel.definition()` is called and the result thrown away. It does
do something: `SemanticModelDTO.definition()` runs
`Preconditions.checkArgument(definition != null, ...)` and then
`definition.toDefinition()`, which re-validates the whole definition
(`SemanticModelDTO.java:86-89`). But nothing on this line says so, and it sits
between two explicit `Preconditions` calls, so a future reader or an IDE
"result unused" cleanup could delete it and silently drop the check.
Worth either a one-line comment saying the call is for its validation side
effect, or making the intent explicit. Note this also means every client
`validate()` on a load/create response pays a full definition conversion; if
that is unwanted, checking the raw DTO for null would be cheaper (it would need
a getter on `SemanticModelDTO`, which currently exposes only the
`SemanticModel` interface methods).
Verified by: read `validate()` here and `SemanticModelDTO.definition()` at
`common/src/main/java/org/apache/gravitino/dto/semantic/SemanticModelDTO.java:85-89`,
plus `SemanticModelDefinitionDTO.toDefinition()` to confirm the conversion is
not memoized.
---
_Generated by [Claude Code](https://claude.ai/code)_
##########
server/src/main/java/org/apache/gravitino/server/web/rest/SemanticModelOperations.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.server.web.rest;
+
+import com.codahale.metrics.annotation.ResponseMetered;
+import com.codahale.metrics.annotation.Timed;
+import javax.inject.Inject;
+import javax.servlet.http.HttpServletRequest;
+import javax.ws.rs.GET;
+import javax.ws.rs.POST;
+import javax.ws.rs.Path;
+import javax.ws.rs.PathParam;
+import javax.ws.rs.Produces;
+import javax.ws.rs.core.Context;
+import javax.ws.rs.core.Response;
+import org.apache.gravitino.NameIdentifier;
+import org.apache.gravitino.catalog.SemanticModelDispatcher;
+import org.apache.gravitino.dto.requests.SemanticModelCreateRequest;
+import org.apache.gravitino.dto.responses.SemanticModelResponse;
+import org.apache.gravitino.dto.util.DTOConverters;
+import org.apache.gravitino.metrics.MetricNames;
+import org.apache.gravitino.semantic.SemanticModel;
+import org.apache.gravitino.server.web.Utils;
+import org.apache.gravitino.utils.NameIdentifierUtil;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/** REST create and load operations for schema-scoped Semantic Models. */
+@Path("metalakes/{metalake}/catalogs/{catalog}/schemas/{schema}/semantic-models")
+public class SemanticModelOperations {
+
+ private static final Logger LOG =
LoggerFactory.getLogger(SemanticModelOperations.class);
+
+ private final SemanticModelDispatcher dispatcher;
+
+ @Context private HttpServletRequest httpRequest;
+
+ /**
+ * Creates Semantic Model REST operations.
+ *
+ * @param dispatcher The Semantic Model dispatcher.
+ */
+ @Inject
+ public SemanticModelOperations(SemanticModelDispatcher dispatcher) {
+ this.dispatcher = dispatcher;
+ }
+
+ /**
+ * Creates a Semantic Model.
+ *
+ * @param metalake The metalake name.
+ * @param catalog The catalog name.
+ * @param schema The schema name.
+ * @param request The structured Semantic Model create request.
+ * @return A response containing the created Semantic Model.
+ */
+ @POST
+ @Produces("application/vnd.gravitino.v1+json")
+ @Timed(name = "create-semantic-model." + MetricNames.HTTP_PROCESS_DURATION,
absolute = true)
+ @ResponseMetered(name = "create-semantic-model", absolute = true)
+ public Response createSemanticModel(
+ @PathParam("metalake") String metalake,
+ @PathParam("catalog") String catalog,
+ @PathParam("schema") String schema,
+ SemanticModelCreateRequest request) {
+ String name = request == null ? "" : request.getName();
+ LOG.info(
+ "Received create Semantic Model request: {}.{}.{}.{}", metalake,
catalog, schema, name);
+ try {
+ return Utils.doAs(
+ httpRequest,
+ () -> {
+ if (request == null) {
+ throw new IllegalArgumentException("Request body must not be
null");
+ }
+ request.validate();
+ NameIdentifier ident =
+ NameIdentifierUtil.ofSemanticModel(metalake, catalog, schema,
request.getName());
+ SemanticModel semanticModel =
+ dispatcher.createSemanticModel(
+ ident, request.getComment(), request.toDefinition(),
request.getProperties());
Review Comment:
[Nit] The whole definition object graph is converted twice on every create.
`request.validate()` on line 92 already calls `toDefinition()`
(`SemanticModelCreateRequest.java:93`) purely to surface validation errors, and
discards the result. Line 97 then calls `request.toDefinition()` again, so
`SemanticModelDefinitionDTO.toDefinition()` rebuilds every dataset, field,
metric, relationship and custom extension a second time
(`SemanticModelDefinitionDTO.java:137-155`, each element via
`SemanticDTOUtils.convertArray`). For a large semantic model that is a needless
full walk of the graph plus the garbage it produces.
Converting once and reusing it reads better and avoids the duplicate work:
```java
request.validate();
SemanticModelDefinition definition = request.toDefinition();
...
dispatcher.createSemanticModel(ident, request.getComment(), definition,
request.getProperties());
```
Verified by: read `validate()` and `toDefinition()` in
`SemanticModelCreateRequest`, and `SemanticModelDefinitionDTO.toDefinition()`,
confirming both call sites perform the same full conversion with no caching.
---
_Generated by [Claude Code](https://claude.ai/code)_
--
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]
