jerryshao commented on code in PR #6765:
URL: https://github.com/apache/gravitino/pull/6765#discussion_r2017238081
##########
catalogs/catalog-model/src/main/java/org/apache/gravitino/catalog/model/ModelCatalogOperations.java:
##########
@@ -96,9 +99,10 @@ public NameIdentifier[] listModels(Namespace namespace)
throws NoSuchSchemaExcep
.toArray(NameIdentifier[]::new);
} catch (NoSuchEntityException e) {
- throw new NoSuchSchemaException(e, "Schema %s does not exist",
namespace);
+ throw new NoSuchSchemaException(e, ErrorMessages.SCHEMA_DOES_NOT_EXIST,
namespace);
} catch (IOException ioe) {
- throw new RuntimeException("Failed to list models under namespace " +
namespace, ioe);
+ throw new RuntimeException(
+ String.format(ErrorMessages.FAILED_TO_LIST_MODELS, namespace), ioe);
Review Comment:
Things like this to refactor the error message is unrelated to this PR and
not so worthy to change, please remove all such changes.
##########
core/src/main/java/org/apache/gravitino/listener/ModelEventDispatcher.java:
##########
@@ -275,6 +276,20 @@ public boolean deleteModelVersion(NameIdentifier ident,
String alias) {
}
}
+ /** {@inheritDoc} */
+ @Override
+ public Model alterModel(NameIdentifier ident, ModelChange... changes)
+ throws NoSuchModelException, IllegalArgumentException {
+ // TODO add model pre event
+ try {
+ // TODO add model event
+ return dispatcher.alterModel(ident, changes);
+ } catch (Exception e) {
+ // TODO add model failure event
+ throw e;
+ }
+ }
Review Comment:
In case to forget this, I would suggest you to add event support in this PR.
##########
catalogs/catalog-model/src/main/java/org/apache/gravitino/catalog/model/ModelCatalogOperations.java:
##########
@@ -259,6 +266,73 @@ public boolean deleteModelVersion(NameIdentifier ident,
String alias) {
return internalDeleteModelVersion(modelVersionIdent);
}
+ /** {@inheritDoc} */
+ @Override
+ public Model alterModel(NameIdentifier ident, ModelChange... changes)
+ throws NoSuchModelException, IllegalArgumentException {
+ try {
+ if (!store.exists(ident, Entity.EntityType.MODEL)) {
+ throw new NoSuchModelException(ErrorMessages.MODEL_DOES_NOT_EXIST,
ident);
+ }
+ } catch (IOException ioe) {
+ throw new
RuntimeException(String.format(ErrorMessages.FAILED_TO_LOAD_MODEL, ident), ioe);
+ }
+
+ try {
+ ModelEntity updatedModelEntity =
+ store.update(
+ ident,
+ ModelEntity.class,
+ Entity.EntityType.MODEL,
+ e -> updateModelEntity(ident, e, changes));
+
+ return toModelImpl(updatedModelEntity);
+
+ } catch (IOException ioe) {
+ throw new
RuntimeException(String.format(ErrorMessages.FAILED_TO_LOAD_MODEL, ident), ioe);
+ } catch (NoSuchEntityException nsee) {
+ throw new NoSuchModelException(nsee, ErrorMessages.MODEL_DOES_NOT_EXIST,
ident);
+ } catch (EntityAlreadyExistsException eaee) {
+ // This is happened when renaming a model to an existing model name.
+ throw new RuntimeException(
+ String.format(ErrorMessages.MODEL_DOES_NOT_EXIST, ident.name()),
eaee);
Review Comment:
It is not model does not exist error, it is model already exists error.
##########
api/src/main/java/org/apache/gravitino/model/ModelChange.java:
##########
@@ -0,0 +1,135 @@
+/*
+ * 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.model;
+
+import com.google.common.annotations.VisibleForTesting;
+import org.apache.gravitino.annotation.Evolving;
+
+/**
+ * A model change is a change to a model. It can be used to rename a model,
update the comment of a
+ * model, set a property and value pair for a model, or remove a property from
a model.
+ */
+@Evolving
+public interface ModelChange {
+ /**
+ * Create a ModelChange for renaming a model.
+ *
+ * @param newName The new model name.
+ * @return A ModelChange for the rename.
+ */
+ static ModelChange rename(String newName) {
+ return new ModelChange.RenameModel(newName);
+ }
+
+ /**
+ * Create a unsupported ModelChange for testing.
+ *
+ * @param message the error message.
+ * @return A ModelChange for the unsupported operation.
+ */
+ static ModelChange unSupport(String message) {
+ return new UnsupportModelChange(message);
+ }
Review Comment:
I think it is improper to add a `UnsupportModelChange` here only for
testing. API module is exposed to the users, we should not add any unneeded
things. Please remove it.
##########
core/src/main/java/org/apache/gravitino/storage/relational/service/ModelMetaService.java:
##########
@@ -196,4 +200,38 @@ ModelPO getModelPOByIdentifier(NameIdentifier ident) {
}
return modelPO;
}
+
+ public <E extends Entity & HasIdentifier> ModelEntity updateModel(
+ NameIdentifier identifier, Function<E, E> updater) throws IOException {
+ NameIdentifierUtil.checkModel(identifier);
+
+ ModelPO oldModelPO = getModelPOByIdentifier(identifier);
+ ModelEntity oldModelEntity = POConverters.fromModelPO(oldModelPO,
identifier.namespace());
+ ModelEntity newEntity = (ModelEntity) updater.apply((E) oldModelEntity);
+ Preconditions.checkArgument(
+ Objects.equals(oldModelEntity.id(), newEntity.id()),
+ "The updated model entity id: %s should be same with the table entity
id before: %s",
+ newEntity.id(),
+ oldModelEntity.id());
+
+ Integer updateResult;
+ try {
+ updateResult =
+ SessionUtils.doWithCommitAndFetchResult(
+ ModelMetaMapper.class,
+ mapper ->
+ mapper.updateModelMeta(
+ POConverters.updateModelPO(oldModelPO, newEntity),
oldModelPO));
+ } catch (RuntimeException re) {
+ ExceptionUtils.checkSQLException(
+ re, Entity.EntityType.CATALOG,
newEntity.nameIdentifier().toString());
+ throw re;
+ }
+
+ if (updateResult > 0) {
+ return newEntity;
+ } else {
+ throw new IOException("Failed to update the entity: " + identifier);
+ }
+ }
Review Comment:
Can you please add a test for this change?
##########
api/src/main/java/org/apache/gravitino/ErrorMessages.java:
##########
@@ -0,0 +1,67 @@
+/*
+ * 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;
+
+/**
+ * User friendly error messages for model operations. This class provides
standardized error message
+ * templates for model-related operations.
+ */
+public class ErrorMessages {
+ /** Error message when a schema does not exist. */
+ public static final String SCHEMA_DOES_NOT_EXIST = "Schema %s does not
exist";
+
+ /** Error message when a model does not exist. */
+ public static final String MODEL_DOES_NOT_EXIST = "Model %s does not exist";
+
+ /** Error message when attempting to create a model that already exists. */
+ public static final String MODEL_ALREADY_EXISTS = "Model %s already exists";
+
+ /** Error message when attempting to create a model version with aliases
that already exist. */
+ public static final String MODEL_VERSION_ALREADY_EXISTS =
+ "Model version aliases %s already exist";
+
+ /** Error message when loading a model fails. */
+ public static final String FAILED_TO_LOAD_MODEL = "Failed to load model %s";
+
+ /** Error message when deleting a model fails. */
+ public static final String FAILED_TO_DELETE_MODEL = "Failed to delete model
%s";
+
+ /** Error message when listing model versions fails. */
+ public static final String FAILED_TO_LIST_MODEL_VERSIONS =
+ "Failed to list model versions for model %s";
+
+ /** Error message when listing models fails. */
+ public static final String FAILED_TO_LIST_MODELS = "Failed to list models
under namespace %s";
+
+ /** Error message when linking a model version fails. */
+ public static final String FAILED_TO_LINK_MODEL_VERSION = "Failed to link
model version %s";
+
+ /** Error message when getting a model version fails. */
+ public static final String FAILED_TO_GET_MODEL = "Failed to get model
version %s";
+
+ /** Error message when registering a model fails. */
+ public static final String FAILED_TO_REGISTER_MODEL = "Failed to register
model %s";
+
+ /** Error message when a property string identifier is null. */
+ public static final String PROPERTY_NOT_NULL = "Property string identifier
should not be null";
+
+ /** Error message when deleting a model version fails. */
+ public static final String FAILED_TO_DELETE_MODEL_VERSION = "Failed to
delete model version %s";
+}
Review Comment:
I don't think we need this change mixed with model rename PR. Please revert
such unrelated changes.
--
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: commits-unsubscr...@gravitino.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
