This is an automated email from the ASF dual-hosted git repository.
jshao pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/gravitino.git
The following commit(s) were added to refs/heads/main by this push:
new 8bd045d3a2 [#9727] fix(model): fix UnsupportedOperationException when
updating aliases for model version created without aliases (#10071)
8bd045d3a2 is described below
commit 8bd045d3a29a60451a92a7c129d5dd5456588d43
Author: Jerry Shao <[email protected]>
AuthorDate: Sat Feb 28 20:48:17 2026 +0800
[#9727] fix(model): fix UnsupportedOperationException when updating aliases
for model version created without aliases (#10071)
### What changes were proposed in this pull request?
When a model version is created with **no aliases** and aliases are
later added via `alterModelVersion`, the server throws an
`UnsupportedOperationException` with the message "Model version entity
does not have an ID.".
The fix adds a `modelId` parameter to
`POConverters.updateModelVersionAliasRelPO()` and uses it directly in
the empty-old-aliases branch to construct alias relation POs, instead of
calling `entity.id()` which always throws on `ModelVersionEntity`.
### Why are the changes needed?
`POConverters.updateModelVersionAliasRelPO` had two code paths:
1. **Non-empty old aliases** — copy `modelId`/`version` from the
existing `ModelVersionAliasRelPO` ✅
2. **Empty old aliases** — call `createAliasRelPO(ModelVersionEntity,
String)` which called `entity.id()` → always throws
`UnsupportedOperationException` since `ModelVersionEntity` intentionally
has no DB entity ID ❌
Fix: #9727
### Does this PR introduce _any_ user-facing change?
No. This is a bug fix for an existing API. The `alterModelVersion` REST
API now correctly handles the case where a model version was linked
without aliases.
### How was this patch tested?
- **Unit test** (`TestPOConverters#testUpdateModelVersionAliasRelPO`):
covers both the non-empty and empty old-aliases paths of
`updateModelVersionAliasRelPO`.
- **Service-level test**
(`TestModelVersionMetaService#testUpdateModelVersionAliasesFromEmpty`):
verifies the full update flow in the JDBC storage layer — create a model
version with no aliases, then alter it to add aliases.
- **Java integration test**
(`ModelCatalogOperationsIT#testLinkAndUpdateModelVersionAliasesFromEmpty`):
end-to-end test against a live Gravitino server.
- **Python integration test**
(`test_model_catalog.py#test_link_update_model_version_aliases_from_empty`):
end-to-end test via the Python client.
---------
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Qi Yu <[email protected]>
---
.../integration/test/ModelCatalogOperationsIT.java | 42 ++++++++++++++++
.../gravitino/dto/function/function_param_dto.py | 2 +-
.../tests/integration/test_function_catalog.py | 16 ++++--
.../tests/integration/test_model_catalog.py | 44 +++++++++++++++++
.../service/ModelVersionMetaService.java | 3 +-
.../storage/relational/utils/POConverters.java | 37 +++++++-------
.../service/TestModelVersionMetaService.java | 57 ++++++++++++++++++++++
.../storage/relational/utils/TestPOConverters.java | 48 ++++++++++++++++++
8 files changed, 225 insertions(+), 24 deletions(-)
diff --git
a/catalogs/catalog-model/src/test/java/org/apache/gravtitino/catalog/model/integration/test/ModelCatalogOperationsIT.java
b/catalogs/catalog-model/src/test/java/org/apache/gravtitino/catalog/model/integration/test/ModelCatalogOperationsIT.java
index f16cec8e9c..9a53e3cdc8 100644
---
a/catalogs/catalog-model/src/test/java/org/apache/gravtitino/catalog/model/integration/test/ModelCatalogOperationsIT.java
+++
b/catalogs/catalog-model/src/test/java/org/apache/gravtitino/catalog/model/integration/test/ModelCatalogOperationsIT.java
@@ -1072,6 +1072,48 @@ public class ModelCatalogOperationsIT extends BaseIT {
Assertions.assertEquals(modelVersion.properties(),
reloadedModelVersion.properties());
}
+ @Test
+ void testLinkAndUpdateModelVersionAliasesFromEmpty() {
+ // Regression test for https://github.com/apache/gravitino/issues/9727:
+ // updating aliases on a model version that was linked without any aliases
must not throw.
+ String modelName = RandomNameUtils.genRandomName("model1");
+ Map<String, String> properties = ImmutableMap.of("key1", "val1");
+ NameIdentifier modelIdent = NameIdentifier.of(schemaName, modelName);
+
+ gravitinoCatalog.asModelCatalog().registerModel(modelIdent, null, null);
+
+ // Link a model version with NO aliases
+ gravitinoCatalog
+ .asModelCatalog()
+ .linkModelVersion(modelIdent, "uri", new String[] {}, "comment",
properties);
+
+ ModelVersion modelVersion =
gravitinoCatalog.asModelCatalog().getModelVersion(modelIdent, 0);
+ Assertions.assertEquals(0, modelVersion.version());
+ Assertions.assertEquals(0, modelVersion.aliases().length);
+
+ // Now add aliases to the version that previously had none — must not throw
+ ModelVersionChange change =
+ ModelVersionChange.updateAliases(new String[] {"alias1", "alias2"},
new String[] {});
+ ModelVersion updatedModelVersion =
+ Assertions.assertDoesNotThrow(
+ () ->
gravitinoCatalog.asModelCatalog().alterModelVersion(modelIdent, 0, change));
+
+ String[] expectedAliases = {"alias1", "alias2"};
+ Assertions.assertEquals(0, updatedModelVersion.version());
+ Assertions.assertArrayEquals(expectedAliases,
updatedModelVersion.aliases());
+
+ // Reload and verify aliases are persisted
+ ModelVersion reloadedModelVersion =
+ gravitinoCatalog.asModelCatalog().getModelVersion(modelIdent, 0);
+ Assertions.assertArrayEquals(expectedAliases,
reloadedModelVersion.aliases());
+
+ // Verify lookups by alias now work
+ Assertions.assertDoesNotThrow(
+ () -> gravitinoCatalog.asModelCatalog().getModelVersion(modelIdent,
"alias1"));
+ Assertions.assertDoesNotThrow(
+ () -> gravitinoCatalog.asModelCatalog().getModelVersion(modelIdent,
"alias2"));
+ }
+
@Test
public void testGetModelVersionUri() {
// Test get model version without default uri name
diff --git a/clients/client-python/gravitino/dto/function/function_param_dto.py
b/clients/client-python/gravitino/dto/function/function_param_dto.py
index c56b902145..13c390021b 100644
--- a/clients/client-python/gravitino/dto/function/function_param_dto.py
+++ b/clients/client-python/gravitino/dto/function/function_param_dto.py
@@ -42,7 +42,7 @@ def _encode_default_value(
def _decode_default_value(
- default_value_dict: Optional[Dict[str, Any]]
+ default_value_dict: Optional[Dict[str, Any]],
) -> Optional[FunctionArg]:
if default_value_dict is None:
return None
diff --git a/clients/client-python/tests/integration/test_function_catalog.py
b/clients/client-python/tests/integration/test_function_catalog.py
index ec515785ea..dec050216a 100644
--- a/clients/client-python/tests/integration/test_function_catalog.py
+++ b/clients/client-python/tests/integration/test_function_catalog.py
@@ -235,16 +235,26 @@ class TestFunctionCatalog(IntegrationTestEnv):
updated_trino_impl = self._create_trino_impl("SELECT 2")
function = self._catalog.as_function_catalog().alter_function(
function_ident,
- FunctionChange.update_impl([], FunctionImpl.RuntimeType.TRINO,
updated_trino_impl),
+ FunctionChange.update_impl(
+ [], FunctionImpl.RuntimeType.TRINO, updated_trino_impl
+ ),
)
no_arg_def = next(d for d in function.definitions() if not
d.parameters())
- trino_impls = [i for i in no_arg_def.impls() if i.runtime() ==
FunctionImpl.RuntimeType.TRINO]
+ trino_impls = [
+ i
+ for i in no_arg_def.impls()
+ if i.runtime() == FunctionImpl.RuntimeType.TRINO
+ ]
self.assertEqual(1, len(trino_impls))
self.assertEqual("SELECT 2", trino_impls[0].sql())
function =
self._catalog.as_function_catalog().get_function(function_ident)
no_arg_def = next(d for d in function.definitions() if not
d.parameters())
- trino_impls = [i for i in no_arg_def.impls() if i.runtime() ==
FunctionImpl.RuntimeType.TRINO]
+ trino_impls = [
+ i
+ for i in no_arg_def.impls()
+ if i.runtime() == FunctionImpl.RuntimeType.TRINO
+ ]
self.assertEqual("SELECT 2", trino_impls[0].sql())
# Test remove_impl: remove the Trino implementation from the no-arg
definition
diff --git a/clients/client-python/tests/integration/test_model_catalog.py
b/clients/client-python/tests/integration/test_model_catalog.py
index a73c6e7efc..488b2a0d3d 100644
--- a/clients/client-python/tests/integration/test_model_catalog.py
+++ b/clients/client-python/tests/integration/test_model_catalog.py
@@ -564,6 +564,50 @@ class TestModelCatalog(IntegrationTestEnv):
self.assertEqual("comment", updated_model_version.comment())
self.assertEqual({"k1": "v1", "k2": "v2"},
updated_model_version.properties())
+ def test_link_update_model_version_aliases_from_empty(self):
+ # Regression test for https://github.com/apache/gravitino/issues/9727:
+ # updating aliases on a model version that was linked without any
aliases must not throw.
+ model_name = f"model_it_model{str(randint(0, 1000))}"
+ model_ident = NameIdentifier.of(self._schema_name, model_name)
+ properties = {"k1": "v1"}
+ self._catalog.as_model_catalog().register_model(
+ model_ident, "comment", properties
+ )
+
+ # Link a model version with NO aliases
+ self._catalog.as_model_catalog().link_model_version(
+ model_ident,
+ uri="uri",
+ aliases=[],
+ comment="comment",
+ properties=properties,
+ )
+
+ original_model_version =
self._catalog.as_model_catalog().get_model_version(
+ model_ident, 0
+ )
+ self.assertEqual(0, original_model_version.version())
+ self.assertEqual([], original_model_version.aliases())
+
+ # Add aliases to the version that previously had none — must not raise
+ changes = [ModelVersionChange.update_aliases(["alias1", "alias2"], [])]
+ updated_model_version =
self._catalog.as_model_catalog().alter_model_version(
+ model_ident, 0, *changes
+ )
+
+ self.assertEqual(0, updated_model_version.version())
+ self.assertCountEqual(["alias1", "alias2"],
updated_model_version.aliases())
+
+ # Reload and verify aliases are persisted
+ reloaded =
self._catalog.as_model_catalog().get_model_version(model_ident, 0)
+ self.assertCountEqual(["alias1", "alias2"], reloaded.aliases())
+
+ # Verify lookup by alias works
+ by_alias = self._catalog.as_model_catalog().get_model_version_by_alias(
+ model_ident, "alias1"
+ )
+ self.assertEqual(0, by_alias.version())
+
def test_link_get_model_version(self):
model_name = "model_it_model" + str(randint(0, 1000))
model_ident = NameIdentifier.of(self._schema_name, model_name)
diff --git
a/core/src/main/java/org/apache/gravitino/storage/relational/service/ModelVersionMetaService.java
b/core/src/main/java/org/apache/gravitino/storage/relational/service/ModelVersionMetaService.java
index 82f92a7443..38dd40576a 100644
---
a/core/src/main/java/org/apache/gravitino/storage/relational/service/ModelVersionMetaService.java
+++
b/core/src/main/java/org/apache/gravitino/storage/relational/service/ModelVersionMetaService.java
@@ -339,7 +339,8 @@ public class ModelVersionMetaService {
boolean isAliasChanged =
isModelVersionAliasUpdated(oldModelVersionEntity,
newModelVersionEntity);
List<ModelVersionAliasRelPO> newAliasRelPOs =
- POConverters.updateModelVersionAliasRelPO(oldAliasRelPOs,
newModelVersionEntity);
+ POConverters.updateModelVersionAliasRelPO(
+ oldAliasRelPOs, newModelVersionEntity, modelEntity.id());
boolean isModelVersionUriUpdated =
isModelVersionUriUpdated(oldModelVersionEntity, newModelVersionEntity);
diff --git
a/core/src/main/java/org/apache/gravitino/storage/relational/utils/POConverters.java
b/core/src/main/java/org/apache/gravitino/storage/relational/utils/POConverters.java
index a94f9b3cba..95344a8f89 100644
---
a/core/src/main/java/org/apache/gravitino/storage/relational/utils/POConverters.java
+++
b/core/src/main/java/org/apache/gravitino/storage/relational/utils/POConverters.java
@@ -1708,23 +1708,32 @@ public class POConverters {
}
/**
- * Construct a new ModelVersionAliasRelPO object with the given alias.
+ * Construct a list of new {@link ModelVersionAliasRelPO} objects for the
updated model version,
+ * one entry per alias in the new model version.
*
- * @param oldModelVersionAliasRelPOs The old ModelVersionAliasRelPOs object
+ * @param oldModelVersionAliasRelPOs The old ModelVersionAliasRelPOs list
* @param newModelVersion The new {@link ModelVersionEntity} object
- * @return The new ModelVersionAliasRelPO object
+ * @param modelId The DB ID of the model entity
+ * @return A list of new {@link ModelVersionAliasRelPO} objects, one per
alias
*/
public static List<ModelVersionAliasRelPO> updateModelVersionAliasRelPO(
- List<ModelVersionAliasRelPO> oldModelVersionAliasRelPOs,
ModelVersionEntity newModelVersion) {
+ List<ModelVersionAliasRelPO> oldModelVersionAliasRelPOs,
+ ModelVersionEntity newModelVersion,
+ Long modelId) {
if (!oldModelVersionAliasRelPOs.isEmpty()) {
ModelVersionAliasRelPO oldModelVersionAliasRelPO =
oldModelVersionAliasRelPOs.get(0);
return newModelVersion.aliases().stream()
- .map(alias -> createAliasRelPO(oldModelVersionAliasRelPO, alias))
+ .map(
+ alias ->
+ createAliasRelPO(
+ oldModelVersionAliasRelPO.getModelId(),
+ oldModelVersionAliasRelPO.getModelVersion(),
+ alias))
.collect(Collectors.toList());
} else {
return newModelVersion.aliases().stream()
- .map(alias -> createAliasRelPO(newModelVersion, alias))
+ .map(alias -> createAliasRelPO(modelId, newModelVersion.version(),
alias))
.collect(Collectors.toList());
}
}
@@ -1775,21 +1784,11 @@ public class POConverters {
.collect(Collectors.toList());
}
- private static ModelVersionAliasRelPO createAliasRelPO(
- ModelVersionAliasRelPO oldModelVersionAliasRelPO, String alias) {
- return ModelVersionAliasRelPO.builder()
- .withModelVersion(oldModelVersionAliasRelPO.getModelVersion())
- .withModelVersionAlias(alias)
- .withModelId(oldModelVersionAliasRelPO.getModelId())
- .withDeletedAt(DEFAULT_DELETED_AT)
- .build();
- }
-
- private static ModelVersionAliasRelPO createAliasRelPO(ModelVersionEntity
entity, String alias) {
+ private static ModelVersionAliasRelPO createAliasRelPO(Long modelId, int
version, String alias) {
return ModelVersionAliasRelPO.builder()
- .withModelVersion(entity.version())
+ .withModelVersion(version)
.withModelVersionAlias(alias)
- .withModelId(entity.id())
+ .withModelId(modelId)
.withDeletedAt(DEFAULT_DELETED_AT)
.build();
}
diff --git
a/core/src/test/java/org/apache/gravitino/storage/relational/service/TestModelVersionMetaService.java
b/core/src/test/java/org/apache/gravitino/storage/relational/service/TestModelVersionMetaService.java
index c3fe11c98c..2ab5b484b3 100644
---
a/core/src/test/java/org/apache/gravitino/storage/relational/service/TestModelVersionMetaService.java
+++
b/core/src/test/java/org/apache/gravitino/storage/relational/service/TestModelVersionMetaService.java
@@ -23,6 +23,7 @@ import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Lists;
import java.io.IOException;
import java.sql.SQLException;
+import java.util.Collections;
import java.util.List;
import java.util.Map;
import java.util.UUID;
@@ -913,6 +914,62 @@ public class TestModelVersionMetaService extends
TestJDBCBackend {
updatePropertiesUpdater));
}
+ @TestTemplate
+ void testUpdateModelVersionAliasesFromEmpty() throws IOException {
+ createParentEntities(METALAKE_NAME, CATALOG_NAME, SCHEMA_NAME, AUDIT_INFO);
+
+ Map<String, String> properties = ImmutableMap.of("k1", "v1");
+ String modelName = randomModelName();
+ List<String> updatedVersionAliases = ImmutableList.of("alias1", "alias2");
+
+ ModelEntity modelEntity =
+ createModelEntity(
+ RandomIdGenerator.INSTANCE.nextId(),
+ MODEL_NS,
+ modelName,
+ "model comment",
+ 0,
+ properties,
+ AUDIT_INFO);
+
+ // Create model version with NO aliases
+ ModelVersionEntity modelVersionEntity =
+ createModelVersionEntity(
+ modelEntity.nameIdentifier(),
+ 0,
+ ImmutableMap.of(ModelVersion.URI_NAME_UNKNOWN, "S3://test/path"),
+ Collections.emptyList(),
+ "version comment",
+ properties,
+ AUDIT_INFO);
+
+ ModelVersionEntity updatedModelVersionEntity =
+ createModelVersionEntity(
+ modelVersionEntity.modelIdentifier(),
+ modelVersionEntity.version(),
+ modelVersionEntity.uris(),
+ updatedVersionAliases,
+ modelVersionEntity.comment(),
+ modelVersionEntity.properties(),
+ modelVersionEntity.auditInfo());
+
+ Assertions.assertDoesNotThrow(
+ () -> ModelMetaService.getInstance().insertModel(modelEntity, false));
+ Assertions.assertDoesNotThrow(
+ () ->
ModelVersionMetaService.getInstance().insertModelVersion(modelVersionEntity));
+
+ // Updating aliases on a version that had no aliases must not throw
+ Function<ModelVersionEntity, ModelVersionEntity> updater = old ->
updatedModelVersionEntity;
+
+ ModelVersionEntity altered =
+ Assertions.assertDoesNotThrow(
+ () ->
+ ModelVersionMetaService.getInstance()
+ .updateModelVersion(modelVersionEntity.nameIdentifier(),
updater));
+
+ Assertions.assertEquals(updatedVersionAliases, altered.aliases());
+ }
+
private NameIdentifier getModelVersionIdent(NameIdentifier modelIdent, int
version) {
List<String> parts = Lists.newArrayList(modelIdent.namespace().levels());
parts.add(modelIdent.name());
diff --git
a/core/src/test/java/org/apache/gravitino/storage/relational/utils/TestPOConverters.java
b/core/src/test/java/org/apache/gravitino/storage/relational/utils/TestPOConverters.java
index 2582b459c6..87da71493f 100644
---
a/core/src/test/java/org/apache/gravitino/storage/relational/utils/TestPOConverters.java
+++
b/core/src/test/java/org/apache/gravitino/storage/relational/utils/TestPOConverters.java
@@ -1224,6 +1224,54 @@ public class TestPOConverters {
assertEquals(expectedModelVersionWithNull, convertedModelVersionWithNull);
}
+ @Test
+ public void testUpdateModelVersionAliasRelPO() {
+ NameIdentifier modelIdent = NameIdentifierUtil.ofModel("m", "c", "s",
"model1");
+ AuditInfo auditInfo =
+
AuditInfo.builder().withCreator("creator").withCreateTime(FIX_INSTANT).build();
+ List<String> newAliases = ImmutableList.of("alias3", "alias4");
+ Long modelId = 1L;
+
+ ModelVersionEntity newModelVersion =
+ ModelVersionEntity.builder()
+ .withModelIdentifier(modelIdent)
+ .withVersion(1)
+ .withAliases(newAliases)
+ .withComment("test")
+ .withProperties(null)
+ .withUris(ImmutableMap.of("uri", "hdfs://localhost/test"))
+ .withAuditInfo(auditInfo)
+ .build();
+
+ // Case 1: old alias list is non-empty — modelId/version copied from old PO
+ List<ModelVersionAliasRelPO> oldAliasPOs =
+ ImmutableList.of(
+ ModelVersionAliasRelPO.builder()
+ .withModelVersionAlias("alias1")
+ .withModelVersion(1)
+ .withModelId(modelId)
+ .withDeletedAt(0L)
+ .build());
+
+ List<ModelVersionAliasRelPO> result =
+ POConverters.updateModelVersionAliasRelPO(oldAliasPOs,
newModelVersion, modelId);
+ assertEquals(2, result.size());
+ assertEquals("alias3", result.get(0).getModelVersionAlias());
+ assertEquals("alias4", result.get(1).getModelVersionAlias());
+ assertEquals(1, result.get(0).getModelVersion());
+ assertEquals(modelId, result.get(0).getModelId());
+
+ // Case 2: old alias list is empty (model version created without aliases)
— should not throw
+ List<ModelVersionAliasRelPO> resultFromEmpty =
+ POConverters.updateModelVersionAliasRelPO(
+ Collections.emptyList(), newModelVersion, modelId);
+ assertEquals(2, resultFromEmpty.size());
+ assertEquals("alias3", resultFromEmpty.get(0).getModelVersionAlias());
+ assertEquals("alias4", resultFromEmpty.get(1).getModelVersionAlias());
+ assertEquals(1, resultFromEmpty.get(0).getModelVersion());
+ assertEquals(modelId, resultFromEmpty.get(0).getModelId());
+ }
+
@Test
public void testStatisticPO() throws JsonProcessingException {
List<StatisticEntity> statisticEntities = Lists.newArrayList();
