Copilot commented on code in PR #10879:
URL: https://github.com/apache/gravitino/pull/10879#discussion_r3151721099
##########
core/src/main/java/org/apache/gravitino/storage/relational/service/ViewMetaService.java:
##########
@@ -158,104 +151,81 @@ public GenericEntity getViewByIdentifier(NameIdentifier
identifier) {
metricsSource = GRAVITINO_RELATIONAL_STORE_METRIC_NAME,
baseMetricName = "insertViewFromEntity")
public void insertView(GenericEntity entity, boolean overwrite) throws
IOException {
- Namespace namespace = entity.namespace();
- NamespaceUtil.checkView(namespace);
-
- ViewPO.Builder builder = createViewPOBuilder(namespace);
- ViewPO viewPO =
- builder
- .withViewId(entity.id())
- .withViewName(entity.name())
- .withCurrentVersion(1L)
- .withLastVersion(1L)
- .withDeletedAt(0L)
+ NamespaceUtil.checkView(entity.namespace());
+
+ ViewEntity viewEntity =
+ ViewEntity.builder()
+ .withId(entity.id())
+ .withName(entity.name())
+ .withNamespace(entity.namespace())
+ .withColumns(new Column[0])
+ .withRepresentations(
+ new Representation[] {
+
SQLRepresentation.builder().withDialect("unknown").withSql("placeholder").build()
+ })
+ .withAuditInfo(AuditInfo.EMPTY)
.build();
-
- insertView(viewPO, overwrite);
+ insertView(viewEntity, overwrite);
}
Review Comment:
Persisting a synthetic placeholder representation/columns for
`GenericEntity` view inserts risks storing materially incorrect view
definitions in the metadata store, which can be hard to detect later. If this
path is only intended for tests/legacy compatibility, consider failing fast
(throw) unless sufficient view definition data is provided, or clearly
separating it into a test-only helper; alternatively, require callers to insert
`ViewEntity` for views so persisted metadata always reflects real definitions.
##########
core/src/main/java/org/apache/gravitino/storage/relational/mapper/ViewMetaMapper.java:
##########
@@ -21,26 +21,69 @@
import java.util.List;
import org.apache.gravitino.storage.relational.po.ViewPO;
+import org.apache.gravitino.storage.relational.po.ViewVersionInfoPO;
import org.apache.ibatis.annotations.DeleteProvider;
import org.apache.ibatis.annotations.InsertProvider;
+import org.apache.ibatis.annotations.One;
import org.apache.ibatis.annotations.Param;
+import org.apache.ibatis.annotations.Result;
+import org.apache.ibatis.annotations.ResultMap;
+import org.apache.ibatis.annotations.Results;
+import org.apache.ibatis.annotations.Select;
import org.apache.ibatis.annotations.SelectProvider;
import org.apache.ibatis.annotations.UpdateProvider;
-/**
- * A MyBatis Mapper for view meta operation SQLs.
- *
- * <p>This interface class is a specification defined by MyBatis. It requires
this interface class
- * to identify the corresponding SQLs for execution. We can write SQLs in an
additional XML file, or
- * write SQLs with annotations in this interface Mapper. See: <a
- * href="https://mybatis.org/mybatis-3/getting-started.html";></a>
- */
+/** A MyBatis Mapper for view meta operation SQLs. */
public interface ViewMetaMapper {
String TABLE_NAME = "view_meta";
-
+ String VERSION_TABLE_NAME = "view_version_info";
+
+ @Results(
+ id = "mapToViewVersionInfoPO",
+ value = {
+ @Result(property = "id", column = "id", id = true),
+ @Result(property = "metalakeId", column = "version_metalake_id"),
+ @Result(property = "catalogId", column = "version_catalog_id"),
+ @Result(property = "schemaId", column = "version_schema_id"),
+ @Result(property = "viewId", column = "version_view_id"),
+ @Result(property = "version", column = "version"),
+ @Result(property = "viewComment", column = "view_comment"),
+ @Result(property = "columns", column = "columns"),
+ @Result(property = "properties", column = "properties"),
+ @Result(property = "defaultCatalog", column = "default_catalog"),
+ @Result(property = "defaultSchema", column = "default_schema"),
+ @Result(property = "representations", column = "representations"),
+ @Result(property = "auditInfo", column = "version_audit_info"),
+ @Result(property = "deletedAt", column = "version_deleted_at")
+ })
+ @Select("SELECT 1") // Dummy SQL to avoid MyBatis error, never be executed
+ ViewVersionInfoPO mapToViewVersionInfoPO();
+
+ @Results(
+ id = "viewPOResultMap",
+ value = {
+ @Result(property = "viewId", column = "view_id", id = true),
+ @Result(property = "viewName", column = "view_name"),
+ @Result(property = "metalakeId", column = "metalake_id"),
+ @Result(property = "catalogId", column = "catalog_id"),
+ @Result(property = "schemaId", column = "schema_id"),
+ @Result(property = "currentVersion", column = "current_version"),
+ @Result(property = "lastVersion", column = "last_version"),
+ @Result(property = "auditInfo", column = "audit_info"),
+ @Result(property = "deletedAt", column = "deleted_at"),
+ @Result(
+ property = "viewVersionInfoPO",
+ javaType = ViewVersionInfoPO.class,
+ column =
+
"{id,version_metalake_id,version_catalog_id,version_schema_id,version_view_id,"
+ +
"version,view_comment,columns,properties,default_catalog,default_schema,"
+ + "representations,version_audit_info,version_deleted_at}",
+ one = @One(resultMap = "mapToViewVersionInfoPO"))
+ })
Review Comment:
This MyBatis annotation mapping is very likely invalid: `@One` typically
requires a `select = ...` (not a `resultMap = ...`), and the composite `column
= \"{...}\"` format is not in the expected `{prop=col,...}` form. The
`@Select(\"SELECT 1\")` dummy method is also a red flag and may break at
runtime or fail to compile depending on MyBatis version. Prefer mapping the
joined columns directly into the nested object (e.g., via dot-path properties
like `viewVersionInfoPO.id`, `viewVersionInfoPO.viewComment`, etc.), or switch
to XML mapping where nested `resultMap` associations are supported cleanly.
##########
core/src/main/java/org/apache/gravitino/storage/relational/service/ViewMetaService.java:
##########
@@ -158,104 +151,81 @@ public GenericEntity getViewByIdentifier(NameIdentifier
identifier) {
metricsSource = GRAVITINO_RELATIONAL_STORE_METRIC_NAME,
baseMetricName = "insertViewFromEntity")
public void insertView(GenericEntity entity, boolean overwrite) throws
IOException {
- Namespace namespace = entity.namespace();
- NamespaceUtil.checkView(namespace);
-
- ViewPO.Builder builder = createViewPOBuilder(namespace);
- ViewPO viewPO =
- builder
- .withViewId(entity.id())
- .withViewName(entity.name())
- .withCurrentVersion(1L)
- .withLastVersion(1L)
- .withDeletedAt(0L)
+ NamespaceUtil.checkView(entity.namespace());
+
+ ViewEntity viewEntity =
+ ViewEntity.builder()
+ .withId(entity.id())
+ .withName(entity.name())
+ .withNamespace(entity.namespace())
+ .withColumns(new Column[0])
+ .withRepresentations(
+ new Representation[] {
+
SQLRepresentation.builder().withDialect("unknown").withSql("placeholder").build()
+ })
+ .withAuditInfo(AuditInfo.EMPTY)
.build();
-
- insertView(viewPO, overwrite);
+ insertView(viewEntity, overwrite);
}
- /**
- * Update a view.
- *
- * @param ident The identifier of the view.
- * @param updater The function to update the view entity.
- * @return The updated GenericEntity.
- * @throws IOException If an I/O error occurs.
- */
@Monitored(
metricsSource = GRAVITINO_RELATIONAL_STORE_METRIC_NAME,
baseMetricName = "updateViewByIdentifier")
- public <E extends HasIdentifier> GenericEntity updateView(
+ public <E extends Entity & HasIdentifier> ViewEntity updateView(
NameIdentifier ident, Function<E, E> updater) throws IOException {
NameIdentifierUtil.checkView(ident);
ViewPO oldViewPO = viewPOFetcher().apply(ident);
+ ViewEntity oldViewEntity = fromViewPO(oldViewPO, ident.namespace());
+ ViewEntity newEntity = (ViewEntity) updater.apply((E) oldViewEntity);
+ Preconditions.checkArgument(
+ Objects.equals(oldViewEntity.id(), newEntity.id()),
+ "The updated view entity id: %s should be same with the entity id
before: %s",
+ newEntity.id(),
+ oldViewEntity.id());
- GenericEntity oldEntity =
- GenericEntity.builder()
- .withId(oldViewPO.getViewId())
- .withName(oldViewPO.getViewName())
- .withNamespace(ident.namespace())
- .withEntityType(Entity.EntityType.VIEW)
- .build();
-
- GenericEntity newEntity = (GenericEntity) updater.apply((E) oldEntity);
-
- // Check if the namespace (schema) has changed and resolve new schema ID
if needed
- boolean isSchemaChanged =
- newEntity.namespace() != null &&
!newEntity.namespace().equals(ident.namespace());
- Long schemaId =
- isSchemaChanged
- ? EntityIdService.getEntityId(
- NameIdentifier.of(newEntity.namespace().levels()),
Entity.EntityType.SCHEMA)
- : oldViewPO.getSchemaId();
-
- ViewPO newViewPO =
- ViewPO.builder()
- .withViewId(oldViewPO.getViewId())
- .withViewName(newEntity.name())
- .withMetalakeId(oldViewPO.getMetalakeId())
- .withCatalogId(oldViewPO.getCatalogId())
- .withSchemaId(schemaId)
- .withDeletedAt(oldViewPO.getDeletedAt())
- .withLastVersion(oldViewPO.getLastVersion())
- .withCurrentVersion(oldViewPO.getCurrentVersion())
- .build();
-
- updateView(oldViewPO, newViewPO);
-
- return newEntity;
+ try {
+ ViewPO newViewPO = updateViewPO(oldViewPO, newEntity);
+ SessionUtils.doMultipleWithCommit(
+ () ->
+ SessionUtils.doWithoutCommit(
+ ViewVersionInfoMapper.class,
+ mapper ->
mapper.insertViewVersionInfo(newViewPO.getViewVersionInfoPO())),
+ () ->
+ SessionUtils.doWithoutCommit(
+ ViewMetaMapper.class, mapper ->
mapper.updateViewMeta(newViewPO, oldViewPO)));
+ return newEntity;
+ } catch (RuntimeException re) {
+ ExceptionUtils.checkSQLException(
+ re, Entity.EntityType.VIEW, newEntity.nameIdentifier().toString());
+ throw re;
+ }
}
- /**
- * Delete a view by identifier.
- *
- * @param ident The identifier of the view.
- * @return true if the view was deleted, false otherwise.
- */
@Monitored(
metricsSource = GRAVITINO_RELATIONAL_STORE_METRIC_NAME,
baseMetricName = "deleteViewByIdentifier")
public boolean deleteView(NameIdentifier ident) {
NameIdentifierUtil.checkView(ident);
-
ViewPO viewPO = viewPOFetcher().apply(ident);
return deleteView(viewPO.getViewId());
}
- private ViewPO getViewPOBySchemaIdAndName(Long schemaId, String viewName) {
- ViewPO viewPO =
- SessionUtils.getWithoutCommit(
+ @Monitored(
+ metricsSource = GRAVITINO_RELATIONAL_STORE_METRIC_NAME,
+ baseMetricName = "deleteViewMetasByLegacyTimeline")
+ public int deleteViewMetasByLegacyTimeline(Long legacyTimeline, int limit) {
+ int versionDeletedCount =
+ SessionUtils.doWithCommitAndFetchResult(
+ ViewVersionInfoMapper.class,
+ mapper ->
mapper.deleteViewVersionsByLegacyTimeline(legacyTimeline, limit));
+
+ int metaDeletedCount =
+ SessionUtils.doWithCommitAndFetchResult(
ViewMetaMapper.class,
- mapper -> mapper.selectViewMetaBySchemaIdAndName(schemaId,
viewName));
+ mapper -> mapper.deleteViewMetasByLegacyTimeline(legacyTimeline,
limit));
- if (viewPO == null) {
- throw new NoSuchEntityException(
- NoSuchEntityException.NO_SUCH_ENTITY_MESSAGE,
- Entity.EntityType.VIEW.name().toLowerCase(),
- viewName);
- }
- return viewPO;
+ return versionDeletedCount + metaDeletedCount;
}
Review Comment:
The `limit` parameter is applied independently to version rows and meta
rows, so a single call can delete up to `2 * limit` rows and may delete
versions and metas unevenly when limits are hit. If callers rely on `limit` as
an upper bound on total deletions (or want to avoid orphan rows under tight
limits), consider enforcing a single overall budget (e.g., delete versions
first, then delete metas with `limit - versionDeletedCount`), and ideally run
both deletions in one transaction.
##########
core/src/main/java/org/apache/gravitino/storage/relational/service/ViewMetaService.java:
##########
@@ -158,104 +151,81 @@ public GenericEntity getViewByIdentifier(NameIdentifier
identifier) {
metricsSource = GRAVITINO_RELATIONAL_STORE_METRIC_NAME,
baseMetricName = "insertViewFromEntity")
public void insertView(GenericEntity entity, boolean overwrite) throws
IOException {
- Namespace namespace = entity.namespace();
- NamespaceUtil.checkView(namespace);
-
- ViewPO.Builder builder = createViewPOBuilder(namespace);
- ViewPO viewPO =
- builder
- .withViewId(entity.id())
- .withViewName(entity.name())
- .withCurrentVersion(1L)
- .withLastVersion(1L)
- .withDeletedAt(0L)
+ NamespaceUtil.checkView(entity.namespace());
+
+ ViewEntity viewEntity =
+ ViewEntity.builder()
+ .withId(entity.id())
+ .withName(entity.name())
+ .withNamespace(entity.namespace())
+ .withColumns(new Column[0])
+ .withRepresentations(
+ new Representation[] {
+
SQLRepresentation.builder().withDialect("unknown").withSql("placeholder").build()
+ })
+ .withAuditInfo(AuditInfo.EMPTY)
.build();
-
- insertView(viewPO, overwrite);
+ insertView(viewEntity, overwrite);
}
- /**
- * Update a view.
- *
- * @param ident The identifier of the view.
- * @param updater The function to update the view entity.
- * @return The updated GenericEntity.
- * @throws IOException If an I/O error occurs.
- */
@Monitored(
metricsSource = GRAVITINO_RELATIONAL_STORE_METRIC_NAME,
baseMetricName = "updateViewByIdentifier")
- public <E extends HasIdentifier> GenericEntity updateView(
+ public <E extends Entity & HasIdentifier> ViewEntity updateView(
NameIdentifier ident, Function<E, E> updater) throws IOException {
NameIdentifierUtil.checkView(ident);
ViewPO oldViewPO = viewPOFetcher().apply(ident);
+ ViewEntity oldViewEntity = fromViewPO(oldViewPO, ident.namespace());
+ ViewEntity newEntity = (ViewEntity) updater.apply((E) oldViewEntity);
Review Comment:
This API uses generics with unchecked casts (`(E) oldViewEntity` and
`(ViewEntity) updater.apply(...)`), which can lead to `ClassCastException` at
runtime and makes the contract unclear. Since this service now operates on
`ViewEntity`, prefer a strongly-typed signature like `Function<ViewEntity,
ViewEntity>` (or overloads) to remove unsafe casts and make misuse impossible
at compile time.
```suggestion
public ViewEntity updateView(NameIdentifier ident, Function<ViewEntity,
ViewEntity> updater)
throws IOException {
NameIdentifierUtil.checkView(ident);
ViewPO oldViewPO = viewPOFetcher().apply(ident);
ViewEntity oldViewEntity = fromViewPO(oldViewPO, ident.namespace());
ViewEntity newEntity = updater.apply(oldViewEntity);
```
--
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]
