jerryshao commented on code in PR #6861:
URL: https://github.com/apache/gravitino/pull/6861#discussion_r2036259176
##########
catalogs/catalog-model/src/main/java/org/apache/gravitino/catalog/model/ModelCatalogOperations.java:
##########
@@ -327,6 +350,81 @@ private ModelEntity updateModelEntity(
.build();
}
+ private ModelVersion internalUpdateModelVersion(
+ NameIdentifier ident, ModelVersionChange... changes) {
+ NameIdentifier modelIdent = NameIdentifierUtil.toModelIdentifier(ident);
+ try {
+ if (!store.exists(modelIdent, Entity.EntityType.MODEL)) {
+ throw new NoSuchModelException("Model %s does not exist", modelIdent);
+ }
+
+ if (!store.exists(ident, Entity.EntityType.MODEL_VERSION)) {
+ throw new NoSuchModelVersionException("Model version %s does not
exist", ident);
+ }
+ } catch (IOException ioe) {
+ throw new RuntimeException("Failed to alter model " + ident, ioe);
Review Comment:
“Failed to alter model version ”
##########
catalogs/catalog-model/src/main/java/org/apache/gravitino/catalog/model/ModelCatalogOperations.java:
##########
@@ -327,6 +350,81 @@ private ModelEntity updateModelEntity(
.build();
}
+ private ModelVersion internalUpdateModelVersion(
+ NameIdentifier ident, ModelVersionChange... changes) {
+ NameIdentifier modelIdent = NameIdentifierUtil.toModelIdentifier(ident);
+ try {
+ if (!store.exists(modelIdent, Entity.EntityType.MODEL)) {
+ throw new NoSuchModelException("Model %s does not exist", modelIdent);
+ }
+
+ if (!store.exists(ident, Entity.EntityType.MODEL_VERSION)) {
+ throw new NoSuchModelVersionException("Model version %s does not
exist", ident);
+ }
+ } catch (IOException ioe) {
+ throw new RuntimeException("Failed to alter model " + ident, ioe);
+ }
+
+ try {
+ ModelVersionEntity updatedModelVersionEntity =
+ store.update(
+ ident,
+ ModelVersionEntity.class,
+ Entity.EntityType.MODEL_VERSION,
+ e -> updateModelVersionEntity(e, changes));
+
+ return toModelVersionImpl(updatedModelVersionEntity);
+
+ } catch (IOException ioe) {
+ throw new RuntimeException("Failed to load model " + ident, ioe);
+ } catch (NoSuchEntityException nsee) {
+ throw new NoSuchModelVersionException(nsee, "Model Version %s does not
exist", ident);
+ }
+ }
+
+ private ModelVersionEntity updateModelVersionEntity(
+ ModelVersionEntity modelVersionEntity, ModelVersionChange... changes) {
+
+ NameIdentifier entityModelIdentifier =
modelVersionEntity.modelIdentifier();
+ int entityVersion = modelVersionEntity.version();
+ String entityComment = modelVersionEntity.comment();
+ List<String> entityAliases =
+ modelVersionEntity.aliases() == null
+ ? Lists.newArrayList()
+ : Lists.newArrayList(modelVersionEntity.aliases());
+ String entityUri = modelVersionEntity.uri();
+ Map<String, String> entityProperties =
+ modelVersionEntity.properties() == null
+ ? Maps.newHashMap()
+ : Maps.newHashMap(modelVersionEntity.properties());
+ AuditInfo entityAuditInfo = modelVersionEntity.auditInfo();
+
+ for (ModelVersionChange change : changes) {
+ if (change instanceof ModelVersionChange.UpdateComment) {
+ entityComment = ((ModelVersionChange.UpdateComment)
change).newComment();
+ } else {
+ throw new IllegalArgumentException(
+ "Unsupported model version change: " +
change.getClass().getSimpleName());
+ }
+ }
+
+ return ModelVersionEntity.builder()
+ .withVersion(entityVersion)
+ .withModelIdentifier(entityModelIdentifier)
+ .withAliases(entityAliases)
+ .withComment(entityComment)
+ .withUri(entityUri)
+ .withProperties(entityProperties)
+ .withAuditInfo(
+ AuditInfo.builder()
+ .withCreator(entityAuditInfo.creator())
+ .withCreateTime(entityAuditInfo.createTime())
+ .withLastModifier(entityAuditInfo.lastModifier())
+ .withLastModifiedTime(entityAuditInfo.lastModifiedTime())
Review Comment:
I think you should update the modifer and modify time here, since it is an
alteration operation. I saw you didn't do this in model alteration
https://github.com/apache/gravitino/blob/a9e93e256ef7b397af11b45ad858f771d0c6fdb7/catalogs/catalog-model/src/main/java/org/apache/gravitino/catalog/model/ModelCatalogOperations.java#L323
You can also update the code here in this PR.
##########
catalogs/catalog-model/src/main/java/org/apache/gravitino/catalog/model/ModelCatalogOperations.java:
##########
@@ -327,6 +350,81 @@ private ModelEntity updateModelEntity(
.build();
}
+ private ModelVersion internalUpdateModelVersion(
+ NameIdentifier ident, ModelVersionChange... changes) {
+ NameIdentifier modelIdent = NameIdentifierUtil.toModelIdentifier(ident);
+ try {
+ if (!store.exists(modelIdent, Entity.EntityType.MODEL)) {
+ throw new NoSuchModelException("Model %s does not exist", modelIdent);
+ }
+
+ if (!store.exists(ident, Entity.EntityType.MODEL_VERSION)) {
+ throw new NoSuchModelVersionException("Model version %s does not
exist", ident);
+ }
+ } catch (IOException ioe) {
+ throw new RuntimeException("Failed to alter model " + ident, ioe);
+ }
+
+ try {
+ ModelVersionEntity updatedModelVersionEntity =
+ store.update(
+ ident,
+ ModelVersionEntity.class,
+ Entity.EntityType.MODEL_VERSION,
+ e -> updateModelVersionEntity(e, changes));
+
+ return toModelVersionImpl(updatedModelVersionEntity);
+
+ } catch (IOException ioe) {
+ throw new RuntimeException("Failed to load model " + ident, ioe);
+ } catch (NoSuchEntityException nsee) {
+ throw new NoSuchModelVersionException(nsee, "Model Version %s does not
exist", ident);
+ }
+ }
+
+ private ModelVersionEntity updateModelVersionEntity(
+ ModelVersionEntity modelVersionEntity, ModelVersionChange... changes) {
+
Review Comment:
Please remove this blank line.
##########
core/src/main/java/org/apache/gravitino/catalog/ModelOperationDispatcher.java:
##########
@@ -236,6 +237,57 @@ public Model alterModel(NameIdentifier ident,
ModelChange... changes)
alteredModel.properties()));
}
+ /** {@inheritDoc} */
+ @Override
+ public ModelVersion alterModelVersion(
+ NameIdentifier ident, int version, ModelVersionChange... changes)
+ throws NoSuchModelVersionException, IllegalArgumentException {
+ validateAlterProperties(ident,
HasPropertyMetadata::modelPropertiesMetadata, changes);
+ NameIdentifier catalogIdent = getCatalogIdentifier(ident);
+ ModelVersion alteredModelVersion =
+ TreeLockUtils.doWithTreeLock(
+ ident,
+ LockType.WRITE,
+ () ->
+ doWithCatalog(
+ catalogIdent,
+ c -> c.doWithModelOps(f -> f.alterModelVersion(ident,
version, changes)),
+ NoSuchModelVersionException.class,
+ IllegalArgumentException.class));
+
+ return internalUpdateModelVersion(catalogIdent, alteredModelVersion);
+ }
+
+ /** {@inheritDoc} */
+ @Override
+ public ModelVersion alterModelVersion(
+ NameIdentifier ident, String alias, ModelVersionChange... changes)
+ throws NoSuchModelException, IllegalArgumentException {
+ NameIdentifier catalogIdent = getCatalogIdentifier(ident);
+ ModelVersion alteredModelVersion =
+ TreeLockUtils.doWithTreeLock(
+ ident,
+ LockType.WRITE,
+ () ->
+ doWithCatalog(
+ catalogIdent,
+ c -> c.doWithModelOps(f -> f.alterModelVersion(ident,
alias, changes)),
+ NoSuchModelVersionException.class,
+ IllegalArgumentException.class));
+
+ return internalUpdateModelVersion(catalogIdent, alteredModelVersion);
+ }
Review Comment:
The above two methods are quite the same, you can extract the common part.
##########
catalogs/catalog-model/src/test/java/org/apache/gravtitino/catalog/model/TestModelCatalogOperations.java:
##########
@@ -689,6 +690,102 @@ public void testRename() {
Assertions.assertEquals(properties, alteredModel.properties());
}
+ @Test
+ void testUpdateVersionCommentViaVersion() {
+ String schemaName = randomSchemaName();
+ createSchema(schemaName);
+
+ String modelName = "model1";
+ String modelComment = "model1 comment";
+
+ String versionComment = "version1 comment";
+ String versionNewComment = "new version1 comment";
+ String versionUri = "model_version_path";
+ String[] versionAliases = new String[] {"alias1", "alias2"};
+
+ NameIdentifier modelIdent =
+ NameIdentifierUtil.ofModel(METALAKE_NAME, CATALOG_NAME, schemaName,
modelName);
+ StringIdentifier stringId = StringIdentifier.fromId(idGenerator.nextId());
+ Map<String, String> properties =
StringIdentifier.newPropertiesWithId(stringId, null);
+
+ ops.registerModel(modelIdent, modelComment, properties);
+ StringIdentifier versionId = StringIdentifier.fromId(idGenerator.nextId());
+ Map<String, String> versionProperties =
StringIdentifier.newPropertiesWithId(versionId, null);
+
+ ops.linkModelVersion(modelIdent, versionUri, versionAliases,
versionComment, versionProperties);
+
+ // validade loaded model
Review Comment:
Typo: validate.
##########
catalogs/catalog-model/src/main/java/org/apache/gravitino/catalog/model/ModelCatalogOperations.java:
##########
@@ -327,6 +350,81 @@ private ModelEntity updateModelEntity(
.build();
}
+ private ModelVersion internalUpdateModelVersion(
+ NameIdentifier ident, ModelVersionChange... changes) {
+ NameIdentifier modelIdent = NameIdentifierUtil.toModelIdentifier(ident);
+ try {
+ if (!store.exists(modelIdent, Entity.EntityType.MODEL)) {
+ throw new NoSuchModelException("Model %s does not exist", modelIdent);
+ }
+
+ if (!store.exists(ident, Entity.EntityType.MODEL_VERSION)) {
+ throw new NoSuchModelVersionException("Model version %s does not
exist", ident);
+ }
+ } catch (IOException ioe) {
+ throw new RuntimeException("Failed to alter model " + ident, ioe);
+ }
+
+ try {
+ ModelVersionEntity updatedModelVersionEntity =
+ store.update(
+ ident,
+ ModelVersionEntity.class,
+ Entity.EntityType.MODEL_VERSION,
+ e -> updateModelVersionEntity(e, changes));
+
+ return toModelVersionImpl(updatedModelVersionEntity);
+
+ } catch (IOException ioe) {
+ throw new RuntimeException("Failed to load model " + ident, ioe);
Review Comment:
"Failed to alter model version"
##########
core/src/main/java/org/apache/gravitino/catalog/ModelOperationDispatcher.java:
##########
@@ -213,7 +214,7 @@ public boolean deleteModelVersion(NameIdentifier ident,
String alias) {
/** {@inheritDoc} */
@Override
public Model alterModel(NameIdentifier ident, ModelChange... changes)
- throws NoSuchModelException, IllegalArgumentException {
+ throws NoSuchModelVersionException, IllegalArgumentException {
Review Comment:
Why do we change to `NoSuchModelVersionException`?
--
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
