mchades commented on code in PR #14040:
URL: https://github.com/apache/gravitino/pull/14040#discussion_r4335358696
##########
core/src/main/java/org/apache/gravitino/catalog/ManagedTableOperations.java:
##########
@@ -181,8 +181,26 @@ public Table alterTable(NameIdentifier ident,
TableChange... changes)
} catch (NoSuchEntityException e) {
throw new NoSuchTableException(e, "Table %s does not exist", ident);
} catch (EntityAlreadyExistsException e) {
- throw new IllegalArgumentException(
- "Failed to rename table " + ident + " due to table already exists:
", e);
+ NameIdentifier newIdent =
+ Arrays.stream(changes)
+ .filter(c -> c instanceof TableChange.RenameTable)
+ .map(c -> (TableChange.RenameTable) c)
+ .map(
+ rename ->
+ NameIdentifier.of(
+ rename
+ .getNewSchemaName()
+ .map(
+ schema ->
+ NamespaceUtil.ofTable(
+ ident.namespace().level(0),
+ ident.namespace().level(1),
+ schema))
+ .orElse(ident.namespace()),
+ rename.getNewName()))
+ .reduce((first, second) -> second)
+ .orElse(ident);
+ throw new TableAlreadyExistsException(e, "Table %s already exists",
newIdent);
Review Comment:
The catch block now reuses the identifier produced by the updater.
`applyChanges` also preserves the previously specified schema when a later
rename changes only the name.
##########
core/src/test/java/org/apache/gravitino/storage/relational/TestRenameConflicts.java:
##########
@@ -0,0 +1,241 @@
+/*
+ * 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.
+ */
+package org.apache.gravitino.storage.relational;
+
+import static org.apache.gravitino.TestCatalog.PROPERTY_KEY1;
+import static org.apache.gravitino.TestCatalog.PROPERTY_KEY2;
+import static org.apache.gravitino.TestCatalog.PROPERTY_KEY5_PREFIX;
+
+import java.util.Collections;
+import java.util.Map;
+import org.apache.commons.lang3.reflect.FieldUtils;
+import org.apache.gravitino.Catalog;
+import org.apache.gravitino.CatalogChange;
+import org.apache.gravitino.Config;
+import org.apache.gravitino.Configs;
+import org.apache.gravitino.Entity.EntityType;
+import org.apache.gravitino.EntityAlreadyExistsException;
+import org.apache.gravitino.EntityStore;
+import org.apache.gravitino.GravitinoEnv;
+import org.apache.gravitino.MetalakeChange;
+import org.apache.gravitino.NameIdentifier;
+import org.apache.gravitino.Namespace;
+import org.apache.gravitino.cache.NoOpsCache;
+import org.apache.gravitino.catalog.CatalogManager;
+import org.apache.gravitino.catalog.ManagedTableOperations;
+import org.apache.gravitino.connector.SupportsSchemas;
+import org.apache.gravitino.exceptions.CatalogAlreadyExistsException;
+import org.apache.gravitino.exceptions.MetalakeAlreadyExistsException;
+import org.apache.gravitino.exceptions.TableAlreadyExistsException;
+import org.apache.gravitino.lock.LockManager;
+import org.apache.gravitino.meta.BaseMetalake;
+import org.apache.gravitino.meta.CatalogEntity;
+import org.apache.gravitino.meta.TableEntity;
+import org.apache.gravitino.metalake.MetalakeManager;
+import org.apache.gravitino.rel.TableChange;
+import org.apache.gravitino.rel.expressions.distributions.Distributions;
+import org.apache.gravitino.rel.expressions.sorts.SortOrder;
+import org.apache.gravitino.rel.expressions.transforms.Transform;
+import org.apache.gravitino.rel.indexes.Indexes;
+import org.apache.gravitino.secret.SecretManager;
+import org.apache.gravitino.storage.IdGenerator;
+import org.apache.gravitino.storage.RandomIdGenerator;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.TestTemplate;
+import org.mockito.Mockito;
+
+/** Verifies rename conflict translation against real metadata database
constraints. */
+public class TestRenameConflicts extends TestJDBCBackend {
+ private Config config;
+ private RelationalEntityStore store;
+ private Object previousConfig;
+ private Object previousLockManager;
+ private LockManager lockManager;
+ private EntityChangeLogPoller poller;
+
+ @BeforeEach
+ void setUpManagers() throws IllegalAccessException {
+ config = new Config(false) {};
+ config.set(Configs.CACHE_ENABLED, false);
+ config.set(Configs.CATALOG_LOAD_ISOLATED, false);
+ previousConfig = FieldUtils.readField(GravitinoEnv.getInstance(),
"config", true);
+ previousLockManager = FieldUtils.readField(GravitinoEnv.getInstance(),
"lockManager", true);
+ if (lockManager == null) {
+ lockManager = new LockManager(config);
+ }
+ FieldUtils.writeField(GravitinoEnv.getInstance(), "config", config, true);
+ FieldUtils.writeField(GravitinoEnv.getInstance(), "lockManager",
lockManager, true);
+ store = new RelationalEntityStore();
+ FieldUtils.writeField(store, "backend", backend, true);
+ FieldUtils.writeField(store, "cache", new NoOpsCache(config), true);
+ poller = new EntityChangeLogPoller(3600);
+ FieldUtils.writeField(store, "entityChangeLogPoller", poller, true);
+ }
+
+ @AfterEach
+ void restoreEnvironment() throws Exception {
+ poller.close();
+ FieldUtils.writeField(GravitinoEnv.getInstance(), "config",
previousConfig, true);
+ FieldUtils.writeField(GravitinoEnv.getInstance(), "lockManager",
previousLockManager, true);
+ }
+
+ @TestTemplate
+ void testRenameMetalakeToExistingName() throws Exception {
+ BaseMetalake source = createAndInsertMakeLake("source");
+ BaseMetalake target = createAndInsertMakeLake("target");
+ MetalakeManager manager = new MetalakeManager(store,
RandomIdGenerator.INSTANCE);
+ for (MetalakeChange[] changes :
+ new MetalakeChange[][] {
+ {MetalakeChange.rename("target")},
+ {MetalakeChange.rename("intermediate"),
MetalakeChange.rename("target")}
+ }) {
+ MetalakeAlreadyExistsException exception =
+ Assertions.assertThrows(
+ MetalakeAlreadyExistsException.class,
+ () -> manager.alterMetalake(source.nameIdentifier(), changes));
+ Assertions.assertEquals("Metalake target already exists",
exception.getMessage());
+ Assertions.assertInstanceOf(EntityAlreadyExistsException.class,
exception.getCause());
+ Assertions.assertEquals(source.id(),
manager.loadMetalake(source.nameIdentifier()).id());
+ Assertions.assertEquals(target.id(),
manager.loadMetalake(target.nameIdentifier()).id());
+ Assertions.assertFalse(store.exists(NameIdentifier.of("intermediate"),
EntityType.METALAKE));
+ }
+ }
+
+ @TestTemplate
+ void testRenameCatalogToExistingName() throws Exception {
+ createAndInsertMakeLake("lake");
+ NameIdentifier source = NameIdentifier.of("lake", "source");
+ NameIdentifier target = NameIdentifier.of("lake", "target");
+ Map<String, String> properties =
+ Map.of(
+ "provider",
+ "test",
+ PROPERTY_KEY1,
+ "value1",
+ PROPERTY_KEY2,
+ "value2",
+ PROPERTY_KEY5_PREFIX + "1",
+ "value3");
+ try (CatalogManager manager =
+ new CatalogManager(config, store, RandomIdGenerator.INSTANCE, new
SecretManager(config))) {
+ manager.createCatalog(source, Catalog.Type.RELATIONAL, "test", "source
comment", properties);
+ manager.createCatalog(target, Catalog.Type.RELATIONAL, "test", "target
comment", properties);
+ CatalogEntity originalSource = store.get(source, EntityType.CATALOG,
CatalogEntity.class);
+ CatalogEntity originalTarget = store.get(target, EntityType.CATALOG,
CatalogEntity.class);
+ for (CatalogChange[] changes :
+ new CatalogChange[][] {
+ {CatalogChange.rename("target")},
+ {CatalogChange.rename("intermediate"),
CatalogChange.rename("target")}
+ }) {
+ CatalogAlreadyExistsException exception =
+ Assertions.assertThrows(
+ CatalogAlreadyExistsException.class, () ->
manager.alterCatalog(source, changes));
+ Assertions.assertEquals("Catalog " + target + " already exists",
exception.getMessage());
+ Assertions.assertInstanceOf(EntityAlreadyExistsException.class,
exception.getCause());
+ Assertions.assertEquals(
+ originalSource.id(), store.get(source, EntityType.CATALOG,
CatalogEntity.class).id());
+ Assertions.assertEquals(
+ originalTarget.id(), store.get(target, EntityType.CATALOG,
CatalogEntity.class).id());
+ Assertions.assertEquals("source comment",
manager.loadCatalog(source).comment());
+ Assertions.assertEquals("target comment",
manager.loadCatalog(target).comment());
+ Assertions.assertFalse(
+ store.exists(NameIdentifier.of("lake", "intermediate"),
EntityType.CATALOG));
+ }
+ }
+ }
+
+ @TestTemplate
+ void testRenameManagedTableToExistingName() throws Exception {
+ createParentEntities("lake", "catalog", "schema", AUDIT_INFO);
+ createAndInsertSchema("lake", "catalog", "other_schema");
+ TableEntity source = table(Namespace.of("lake", "catalog", "schema"),
"source");
+ TableEntity target = table(source.namespace(), "target");
+ TableEntity crossSchemaTarget =
+ table(Namespace.of("lake", "catalog", "other_schema"), "target");
+ backend.insert(source, false);
+ backend.insert(target, false);
+ backend.insert(crossSchemaTarget, false);
+ ManagedTableOperations operations =
+ new ManagedTableOperations() {
+ @Override
+ protected EntityStore store() {
+ return store;
+ }
+
+ @Override
+ protected SupportsSchemas schemas() {
+ return Mockito.mock(SupportsSchemas.class);
+ }
+
+ @Override
+ protected IdGenerator idGenerator() {
+ return RandomIdGenerator.INSTANCE;
+ }
+ };
+ for (TableChange[] changes :
+ new TableChange[][] {
+ {TableChange.rename("target")},
+ {TableChange.rename("intermediate"), TableChange.rename("target")},
+ {TableChange.rename("target", "other_schema")},
+ {TableChange.rename("intermediate", "other_schema"),
TableChange.rename("target")}
+ }) {
+ TableChange.RenameTable lastRename = (TableChange.RenameTable)
changes[changes.length - 1];
+ TableEntity expectedTarget =
+ lastRename.getNewSchemaName().isPresent() ? crossSchemaTarget :
target;
Review Comment:
Replaced the derived expectation with explicit expected targets. The tests
now cover preserving an earlier schema and explicitly moving back to the
original schema, for both successful renames and conflicts.
--
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]
