jerryshao commented on code in PR #7502:
URL: https://github.com/apache/gravitino/pull/7502#discussion_r2181521132
##########
catalogs/catalog-model/src/main/java/org/apache/gravitino/catalog/model/ModelCatalogOperations.java:
##########
@@ -503,7 +504,7 @@ private ModelVersionImpl
toModelVersionImpl(ModelVersionEntity modelVersion) {
return ModelVersionImpl.builder()
.withVersion(modelVersion.version())
.withAliases(modelVersion.aliases().toArray(new String[0]))
- .withUri(modelVersion.uri())
+ .withUri(modelVersion.uris().get(ModelVersion.URI_NAME_UNKNOWN))
Review Comment:
Where are the code changes to support multiple URIs in this class?
##########
core/src/main/java/org/apache/gravitino/meta/ModelVersionEntity.java:
##########
@@ -45,8 +46,8 @@ public class ModelVersionEntity implements Entity, Auditable,
HasIdentifier {
Field.optional("comment", String.class, "The comment or description of
the model entity.");
public static final Field ALIASES =
Field.optional("aliases", List.class, "The aliases of the model
entity.");
- public static final Field URL =
- Field.required("uri", String.class, "The URI of the model entity.");
+ public static final Field URIS =
+ Field.required("uris", Map.class, "The URI and their names of the model
entity.");
Review Comment:
The description here is not correct.
##########
core/src/main/java/org/apache/gravitino/storage/relational/mapper/provider/base/ModelVersionMetaBaseSQLProvider.java:
##########
@@ -172,8 +182,24 @@ public String updateModelVersionMeta(
+ "AND version = #{oldModelVersionMeta.modelVersion} "
+ "AND model_version_comment =
#{oldModelVersionMeta.modelVersionComment} "
+ "AND model_version_properties =
#{oldModelVersionMeta.modelVersionProperties} "
- + "AND model_version_uri = #{oldModelVersionMeta.modelVersionUri} "
Review Comment:
Why do we remove this?
##########
core/src/main/java/org/apache/gravitino/storage/relational/mapper/provider/base/ModelVersionMetaBaseSQLProvider.java:
##########
@@ -26,51 +28,60 @@
public class ModelVersionMetaBaseSQLProvider {
- public String insertModelVersionMeta(@Param("modelVersionMeta")
ModelVersionPO modelVersionPO) {
- return "INSERT INTO "
+ public String insertModelVersionMetas(
+ @Param("modelVersionMetas") List<ModelVersionPO> modelVersionPOs) {
+ return "<script>"
+ + "INSERT INTO "
+ ModelVersionMetaMapper.TABLE_NAME
- + "(metalake_id, catalog_id, schema_id, model_id, version,"
- + " model_version_comment, model_version_properties,
model_version_uri,"
- + " audit_info, deleted_at)"
- + " SELECT metalake_id, catalog_id, schema_id, model_id,
model_latest_version,"
- + " #{modelVersionMeta.modelVersionComment},
#{modelVersionMeta.modelVersionProperties},"
- + " #{modelVersionMeta.modelVersionUri},
#{modelVersionMeta.auditInfo},"
- + " #{modelVersionMeta.deletedAt}"
+ + "(metalake_id, catalog_id, schema_id, model_id, version,
model_version_comment,"
+ + " model_version_properties, model_version_uri_name,
model_version_uri, audit_info, deleted_at)"
+ + " SELECT m.metalake_id, m.catalog_id, m.schema_id, m.model_id,
m.model_latest_version, v.model_version_comment,"
+ + " v.model_version_properties, v.model_version_uri_name,
v.model_version_uri, v.audit_info, v.deleted_at"
+ + " FROM ("
+ + "<foreach collection='modelVersionMetas' item='version'
separator='UNION ALL'>"
+ + " SELECT"
+ + " #{version.modelId} AS model_id, #{version.modelVersionComment} AS
model_version_comment,"
+ + " #{version.modelVersionProperties} AS model_version_properties,
#{version.auditInfo} AS audit_info,"
+ + " #{version.deletedAt} AS deleted_at, #{version.modelVersionUriName}
AS model_version_uri_name,"
+ + " #{version.modelVersionUri} AS model_version_uri "
+ + "</foreach>"
+ + " ) v"
+ + " JOIN"
+ + " (SELECT metalake_id, catalog_id, schema_id, model_id,
model_latest_version"
+ " FROM "
+ ModelMetaMapper.TABLE_NAME
- + " WHERE model_id = #{modelVersionMeta.modelId} AND deleted_at = 0";
+ + " WHERE model_id = #{modelVersionMetas[0].modelId} AND deleted_at =
0) m"
+ + " ON v.model_id = m.model_id"
+ + "</script>";
Review Comment:
One uri will be inserted as one recode, so multiple uris of one model
version will have multiple records, am I right?
##########
core/src/test/java/org/apache/gravitino/storage/relational/service/TestModelVersionMetaService.java:
##########
@@ -752,7 +771,7 @@ void testUpdateModelVersionAliases() throws IOException {
createModelVersionEntity(
modelVersionEntity.modelIdentifier(),
modelVersionEntity.version(),
- modelVersionEntity.uri(),
+ modelVersionEntity.uris(),
Review Comment:
The test cases here is not enough to cover your changes, you only change to
make the code work, doesn't actually test your changes.
##########
core/src/main/java/org/apache/gravitino/storage/relational/mapper/provider/base/ModelVersionMetaBaseSQLProvider.java:
##########
@@ -172,8 +182,24 @@ public String updateModelVersionMeta(
+ "AND version = #{oldModelVersionMeta.modelVersion} "
+ "AND model_version_comment =
#{oldModelVersionMeta.modelVersionComment} "
+ "AND model_version_properties =
#{oldModelVersionMeta.modelVersionProperties} "
- + "AND model_version_uri = #{oldModelVersionMeta.modelVersionUri} "
+ "AND audit_info = #{oldModelVersionMeta.auditInfo} "
+ "AND deleted_at = 0";
}
+
+ public String updateModelVersionUris(
+ @Param("modelId") Long modelId,
+ @Param("modelVersion") Integer modelVersion,
+ @Param("uris") Map<String, String> uris) {
+ return "<script>"
+ + "UPDATE "
+ + ModelVersionMetaMapper.TABLE_NAME
+ + " SET"
+ + " model_version_uri = CASE"
+ + "<foreach collection='uris' index='k' item='v' separator=''>"
+ + " WHEN model_version_uri_name = #{k} THEN #{v}"
+ + "</foreach>"
+ + " ELSE model_version_uri END"
+ + " WHERE model_id = #{modelId} AND version = #{modelVersion} AND
deleted_at = 0"
+ + "</script>";
Review Comment:
How do you handle the scenario where the number of URIs is different before
and after? For example, if you have 2 old URIs, now you update the URIs to have
3 new URIs, and vice versa?
--
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]
