mchades commented on code in PR #10879: URL: https://github.com/apache/gravitino/pull/10879#discussion_r3151942647
########## 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: Checked this against the current runtime rather than changing it speculatively. `TestViewMetaService` still passes for insert/get/list/update on the supported relational backends, so this mapping is working as-is in the environments this PR targets. Since this PR is scoped to the ViewEntity storage changes, I am leaving the mapper shape unchanged here. ########## 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: This observation is correct in isolation, but the same delete-budget pattern is already used in the other relational meta services as well. I do not want to special-case view cleanup in this PR and make it diverge from table/fileset/function behavior. If we want a single total budget plus one transaction, that should be handled as a broader follow-up across the shared pattern. ########## 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: The type-safety concern is valid, but this updater signature currently follows the same pattern used by the other relational meta services too. For this PR I fixed the concrete runtime mismatch on the view rename path instead of tightening only the view service API and leaving the rest of the pattern inconsistent. I prefer to keep that signature change as a broader follow-up if we decide to apply it across table/fileset/function/view together. -- 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]
