Copilot commented on code in PR #14040:
URL: https://github.com/apache/gravitino/pull/14040#discussion_r4332172568
##########
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 conflicting `newIdent` is derived by mapping each `RenameTable` against
the original `ident` and then taking the last mapped result. This produces an
incorrect final identifier for multi-step renames where an earlier rename
changes schema and a later rename changes only the name (schema should carry
forward, but this code resets it back to `ident.namespace()` when
`getNewSchemaName()` is empty). Compute the final target identifier by folding
over `changes` in order, updating the current namespace/name as each
`RenameTable` is applied, so the thrown `TableAlreadyExistsException` reliably
reports the true final target (including schema) as stated by the PR contract.
##########
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:
This expected-target computation only inspects the *last* `RenameTable` to
decide whether the final schema should be `other_schema`. For change sequences
that first move schema and then perform a name-only rename (e.g.,
`{rename(\"intermediate\", \"other_schema\"), rename(\"target\")}`), the final
schema should remain `other_schema`, but this logic treats it as the original
schema. To align with the documented contract (“report the final target name,
including the target schema for table renames”), derive `expectedTarget` by
applying the rename changes sequentially (carry forward schema when a later
rename omits it).
##########
api/src/main/java/org/apache/gravitino/rel/TableCatalog.java:
##########
@@ -288,10 +288,11 @@ Table createTable(
* @param changes Table changes to apply to the table.
* @return The updated table metadata.
* @throws NoSuchTableException If the table does not exist.
+ * @throws TableAlreadyExistsException If the new name is already used by
another table.
* @throws IllegalArgumentException If the change is rejected by the
implementation.
*/
Table alterTable(NameIdentifier ident, TableChange... changes)
- throws NoSuchTableException, IllegalArgumentException;
+ throws NoSuchTableException, TableAlreadyExistsException,
IllegalArgumentException;
Review Comment:
Adding `*AlreadyExistsException` to multiple public API interfaces (and
corresponding manager/ops implementations) can be a source-incompatible change
if these exceptions are checked (callers must now catch/declare; third-party
implementers must update their method signatures). If these exceptions are
intended to be part of the stable API contract, this should be explicitly
called out in release notes/versioning; otherwise consider keeping the
interface signature unchanged and still throwing the same exceptions as
unchecked (or a common base) to avoid forcing downstream recompilation/handling
changes.
--
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]
