xloya commented on code in PR #5078:
URL: https://github.com/apache/gravitino/pull/5078#discussion_r1796331816
##########
core/src/main/java/org/apache/gravitino/storage/relational/service/TableMetaService.java:
##########
@@ -144,30 +153,47 @@ public <E extends Entity & HasIdentifier> TableEntity
updateTable(
CommonMetaService.getInstance().getParentEntityIdByNamespace(identifier.namespace());
TablePO oldTablePO = getTablePOBySchemaIdAndName(schemaId, tableName);
- TableEntity oldTableEntity = POConverters.fromTablePO(oldTablePO,
identifier.namespace());
- TableEntity newEntity = (TableEntity) updater.apply((E) oldTableEntity);
+ List<ColumnPO> oldTableColumns =
+ TableColumnMetaService.getInstance()
+ .getColumnsByTableIdAndVersion(oldTablePO.getTableId(),
oldTablePO.getCurrentVersion());
+ TableEntity oldTableEntity =
+ POConverters.fromTableAndColumnPOs(oldTablePO, oldTableColumns,
identifier.namespace());
+
+ TableEntity newTableEntity = (TableEntity) updater.apply((E)
oldTableEntity);
Preconditions.checkArgument(
- Objects.equals(oldTableEntity.id(), newEntity.id()),
+ Objects.equals(oldTableEntity.id(), newTableEntity.id()),
"The updated table entity id: %s should be same with the table entity
id before: %s",
- newEntity.id(),
+ newTableEntity.id(),
oldTableEntity.id());
- Integer updateResult;
+ boolean isColumnChanged =
+ TableColumnMetaService.getInstance().isColumnUpdated(oldTableEntity,
newTableEntity);
+ TablePO newTablePO =
+ POConverters.updateTablePOWithVersion(oldTablePO, newTableEntity,
isColumnChanged);
+
+ final AtomicInteger updateResult = new AtomicInteger(0);
try {
- updateResult =
- SessionUtils.doWithCommitAndFetchResult(
- TableMetaMapper.class,
- mapper ->
- mapper.updateTableMeta(
- POConverters.updateTablePOWithVersion(oldTablePO,
newEntity), oldTablePO));
+ SessionUtils.doMultipleWithCommit(
+ () ->
+ updateResult.set(
Review Comment:
Should the table `updateResult` pass to the update column operation so that
if it equals `0` can throw an IOException immediately?
##########
scripts/mysql/upgrade-0.6.0-to-0.7.0-mysql.sql:
##########
@@ -0,0 +1,41 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one
+-- or more contributor license agreements. See the NOTICE file--
+-- distributed with this work for additional information
+-- regarding copyright ownership. The ASF licenses this file
+-- to you under the Apache License, Version 2.0 (the
+-- "License"). You may not use this file except in compliance
+-- with the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing,
+-- software distributed under the License is distributed on an
+-- "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+-- KIND, either express or implied. See the License for the
+-- specific language governing permissions and limitations
+-- under the License.
+--
+CREATE TABLE IF NOT EXISTS `table_column_version_info` (
+ `id` BIGINT(20) UNSIGNED NOT NULL AUTO_INCREMENT COMMENT 'auto increment
id',
+ `metalake_id` BIGINT(20) UNSIGNED NOT NULL COMMENT 'metalake id',
+ `catalog_id` BIGINT(20) UNSIGNED NOT NULL COMMENT 'catalog id',
+ `schema_id` BIGINT(20) UNSIGNED NOT NULL COMMENT 'schema id',
+ `table_id` BIGINT(20) UNSIGNED NOT NULL COMMENT 'table id',
+ `table_version` INT UNSIGNED NOT NULL COMMENT 'table version',
+ `column_id` BIGINT(20) UNSIGNED NOT NULL COMMENT 'column id',
+ `column_name` VARCHAR(128) NOT NULL COMMENT 'column name',
+ `column_type` VARCHAR(128) NOT NULL COMMENT 'column type',
+ `column_comment` VARCHAR(256) DEFAULT '' COMMENT 'column comment',
+ `column_nullable` TINYINT(1) NOT NULL DEFAULT 1 COMMENT 'column nullable,
0 is not nullable, 1 is nullable',
+ `column_auto_increment` TINYINT(1) NOT NULL DEFAULT 0 COMMENT 'column auto
increment, 0 is not auto increment, 1 is auto increment',
+ `column_default_value` VARCHAR(256) DEFAULT NULL COMMENT 'column default
value',
+ `column_op_type` TINYINT(1) NOT NULL COMMENT 'column operation type, 1 is
create, 2 is update, 3 is delete',
Review Comment:
Can this field save the Enum type name string? Maybe it's more readable.
##########
core/src/main/java/org/apache/gravitino/storage/relational/service/TableMetaService.java:
##########
@@ -183,26 +209,28 @@ public boolean deleteTable(NameIdentifier identifier) {
Long tableId = getTableIdBySchemaIdAndName(schemaId, tableName);
+ AtomicInteger deleteResult = new AtomicInteger(0);
SessionUtils.doMultipleWithCommit(
() ->
- SessionUtils.doWithoutCommit(
- TableMetaMapper.class, mapper ->
mapper.softDeleteTableMetasByTableId(tableId)),
+ deleteResult.set(
Review Comment:
Like the update, maybe the result can pass to the following operations to
throw an exception
in advance.
##########
scripts/mysql/upgrade-0.6.0-to-0.7.0-mysql.sql:
##########
@@ -0,0 +1,41 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one
+-- or more contributor license agreements. See the NOTICE file--
+-- distributed with this work for additional information
+-- regarding copyright ownership. The ASF licenses this file
+-- to you under the Apache License, Version 2.0 (the
+-- "License"). You may not use this file except in compliance
+-- with the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing,
+-- software distributed under the License is distributed on an
+-- "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+-- KIND, either express or implied. See the License for the
+-- specific language governing permissions and limitations
+-- under the License.
+--
+CREATE TABLE IF NOT EXISTS `table_column_version_info` (
+ `id` BIGINT(20) UNSIGNED NOT NULL AUTO_INCREMENT COMMENT 'auto increment
id',
Review Comment:
Maybe the `id` is unnecessary, because we have `column_id` field here.
##########
core/src/main/java/org/apache/gravitino/storage/relational/service/TableMetaService.java:
##########
@@ -144,30 +153,47 @@ public <E extends Entity & HasIdentifier> TableEntity
updateTable(
CommonMetaService.getInstance().getParentEntityIdByNamespace(identifier.namespace());
TablePO oldTablePO = getTablePOBySchemaIdAndName(schemaId, tableName);
- TableEntity oldTableEntity = POConverters.fromTablePO(oldTablePO,
identifier.namespace());
- TableEntity newEntity = (TableEntity) updater.apply((E) oldTableEntity);
+ List<ColumnPO> oldTableColumns =
+ TableColumnMetaService.getInstance()
+ .getColumnsByTableIdAndVersion(oldTablePO.getTableId(),
oldTablePO.getCurrentVersion());
+ TableEntity oldTableEntity =
+ POConverters.fromTableAndColumnPOs(oldTablePO, oldTableColumns,
identifier.namespace());
+
+ TableEntity newTableEntity = (TableEntity) updater.apply((E)
oldTableEntity);
Preconditions.checkArgument(
- Objects.equals(oldTableEntity.id(), newEntity.id()),
+ Objects.equals(oldTableEntity.id(), newTableEntity.id()),
"The updated table entity id: %s should be same with the table entity
id before: %s",
- newEntity.id(),
+ newTableEntity.id(),
oldTableEntity.id());
- Integer updateResult;
+ boolean isColumnChanged =
+ TableColumnMetaService.getInstance().isColumnUpdated(oldTableEntity,
newTableEntity);
+ TablePO newTablePO =
+ POConverters.updateTablePOWithVersion(oldTablePO, newTableEntity,
isColumnChanged);
+
+ final AtomicInteger updateResult = new AtomicInteger(0);
try {
- updateResult =
- SessionUtils.doWithCommitAndFetchResult(
- TableMetaMapper.class,
- mapper ->
- mapper.updateTableMeta(
- POConverters.updateTablePOWithVersion(oldTablePO,
newEntity), oldTablePO));
+ SessionUtils.doMultipleWithCommit(
+ () ->
+ updateResult.set(
Review Comment:
Maybe Fileset's `update` / `delete` operation can improve like this too.
--
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]
