This is an automated email from the ASF dual-hosted git repository.
yuqi1129 pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/gravitino.git
The following commit(s) were added to refs/heads/main by this push:
new 436a6b592a [#14606] fix(core): normalize table alter changes exactly
once (#14607)
436a6b592a is described below
commit 436a6b592a4e62e7108ea46ac05ad0609f45c666
Author: Qi Yu <[email protected]>
AuthorDate: Wed Nov 4 22:53:09 2026 +0800
[#14606] fix(core): normalize table alter changes exactly once (#14607)
### What changes were proposed in this pull request?
Remove duplicate alter-change normalization from the operation
dispatcher. Keep normalization in TableNormalizeDispatcher and use the
same normalized changes for provider dispatch and stored column name
changes.
### Why are the changes needed?
Quote-aware normalization is not idempotent: the second pass folds
already unquoted names or rejects valid names containing spaces.
Fixes #14606.
### Does this PR introduce _any_ user-facing change?
Quoted alter names retain their intended spelling through provider
dispatch.
### How was this patch tested?
TestTableNormalizeDispatcher and TestTableOperationDispatcher pass (61
tests). Reverting the production change reproduces the new regression
failure.
Co-authored-by: Jerry Shao <[email protected]>
---
.../catalog/TableOperationDispatcher.java | 10 ++--
.../catalog/TestTableNormalizeDispatcher.java | 53 ++++++++++++++++++++++
2 files changed, 57 insertions(+), 6 deletions(-)
diff --git
a/core/src/main/java/org/apache/gravitino/catalog/TableOperationDispatcher.java
b/core/src/main/java/org/apache/gravitino/catalog/TableOperationDispatcher.java
index 4439b31145..b63df43356 100644
---
a/core/src/main/java/org/apache/gravitino/catalog/TableOperationDispatcher.java
+++
b/core/src/main/java/org/apache/gravitino/catalog/TableOperationDispatcher.java
@@ -20,7 +20,6 @@ package org.apache.gravitino.catalog;
import static org.apache.gravitino.Entity.EntityType.COLUMN;
import static org.apache.gravitino.Entity.EntityType.TABLE;
-import static org.apache.gravitino.catalog.CapabilityHelpers.applyCapabilities;
import static
org.apache.gravitino.catalog.PropertiesMetadataHelpers.validatePropertyForCreate;
import static
org.apache.gravitino.rel.expressions.transforms.Transforms.EMPTY_TRANSFORM;
import static
org.apache.gravitino.utils.NameIdentifierUtil.getCatalogIdentifier;
@@ -339,15 +338,14 @@ public class TableOperationDispatcher extends
OperationDispatcher implements Tab
isRenameTable && !managed
? getTableEntityBeforeRename(ident)
: Optional.empty();
- TableChange[] normalizedChanges =
- applyCapabilities(catalog.capabilities(), changes);
+ // TableNormalizeDispatcher already normalized the
changes. Normalization can
+ // depend on quoting and must not be repeated on canonical
physical names.
Table table =
- catalog.doWithTableOps(
- tableOps -> tableOps.alterTable(ident,
normalizedChanges));
+ catalog.doWithTableOps(tableOps ->
tableOps.alterTable(ident, changes));
return new AlterTableCatalogResult(
snapshotTable(catalog, table, managed),
tableEntityBeforeRename,
- resolveColumnNameChanges(normalizedChanges));
+ resolveColumnNameChanges(changes));
},
NoSuchTableException.class,
IllegalArgumentException.class);
diff --git
a/core/src/test/java/org/apache/gravitino/catalog/TestTableNormalizeDispatcher.java
b/core/src/test/java/org/apache/gravitino/catalog/TestTableNormalizeDispatcher.java
index 879c448d1c..6bd257e2cb 100644
---
a/core/src/test/java/org/apache/gravitino/catalog/TestTableNormalizeDispatcher.java
+++
b/core/src/test/java/org/apache/gravitino/catalog/TestTableNormalizeDispatcher.java
@@ -37,6 +37,7 @@ import org.apache.gravitino.exceptions.NoSuchCatalogException;
import org.apache.gravitino.exceptions.TableAlreadyExistsException;
import org.apache.gravitino.rel.Column;
import org.apache.gravitino.rel.Table;
+import org.apache.gravitino.rel.TableCatalog;
import org.apache.gravitino.rel.TableChange;
import org.apache.gravitino.rel.expressions.NamedReference;
import org.apache.gravitino.rel.expressions.distributions.Distribution;
@@ -52,6 +53,7 @@ import org.apache.gravitino.rel.indexes.Indexes;
import org.apache.gravitino.rel.partitions.Partitions;
import org.apache.gravitino.rel.partitions.RangePartition;
import org.apache.gravitino.rel.types.Types;
+import org.apache.gravitino.utils.ThrowableFunction;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.Test;
@@ -73,6 +75,57 @@ public class TestTableNormalizeDispatcher extends
TestOperationDispatcher {
TestTableOperationDispatcher.schemaOperationDispatcher,
catalogManager);
}
+ @Test
+ void testAlterChangesAreNormalizedOnlyOnceBeforeReachingCatalog() throws
Exception {
+ CatalogManager manager = Mockito.mock(CatalogManager.class);
+ BaseCatalog<?> catalog = Mockito.mock(BaseCatalog.class);
+
Mockito.when(catalog.capability()).thenReturn(TestCapabilityHelpers.QUOTE_AWARE_CAPABILITY);
+ CatalogTestUtils.mockDoWithCatalog(manager, catalog);
+ CatalogManager.CatalogWrapper wrapper =
Mockito.mock(CatalogManager.CatalogWrapper.class);
+
Mockito.when(wrapper.capabilities()).thenReturn(TestCapabilityHelpers.QUOTE_AWARE_CAPABILITY);
+ Mockito.when(wrapper.doWithCatalog(Mockito.any())).thenReturn(true);
+ Mockito.doAnswer(
+ invocation -> {
+ ThrowableFunction<CatalogManager.CatalogWrapper, Object>
operation =
+ invocation.getArgument(1);
+ return operation.apply(wrapper);
+ })
+ .when(manager)
+ .doWithCatalogWrapper(Mockito.any(), Mockito.any());
+ TableCatalog tables = Mockito.mock(TableCatalog.class);
+ IllegalArgumentException stop =
+ new IllegalArgumentException("Stop after observing provider changes");
+ Mockito.doAnswer(
+ invocation -> {
+ ThrowableFunction<TableCatalog, Object> operation =
invocation.getArgument(0);
+ return operation.apply(tables);
+ })
+ .when(wrapper)
+ .doWithTableOps(Mockito.any());
+ Mockito.when(tables.alterTable(Mockito.any(),
Mockito.<TableChange[]>any())).thenThrow(stop);
+ TableNormalizeDispatcher dispatcher =
+ new TableNormalizeDispatcher(
+ new TableOperationDispatcher(manager, entityStore, idGenerator,
secretManager),
+ manager);
+ NameIdentifier ident = NameIdentifier.of("metalake", "catalog", "schema",
"table");
+ TableChange[] requests = {
+ TableChange.renameColumn(new String[] {"\"MixedOld\""}, "\"MixedNew\""),
+ TableChange.addColumn(new String[] {"\"Added Col\""},
Types.IntegerType.get()),
+ TableChange.rename("\"RenamedTable\"")
+ };
+ Assertions.assertSame(
+ stop,
+ Assertions.assertThrows(
+ IllegalArgumentException.class, () -> dispatcher.alterTable(ident,
requests)));
+ ArgumentCaptor<TableChange[]> captor =
ArgumentCaptor.forClass(TableChange[].class);
+ Mockito.verify(tables).alterTable(Mockito.any(), captor.capture());
+ TableChange[] actual = captor.getValue();
+ Assertions.assertEquals("MixedOld", ((TableChange.RenameColumn)
actual[0]).fieldName()[0]);
+ Assertions.assertEquals("MixedNew", ((TableChange.RenameColumn)
actual[0]).getNewName());
+ Assertions.assertEquals("Added Col", ((TableChange.AddColumn)
actual[1]).fieldName()[0]);
+ Assertions.assertEquals("RenamedTable", ((TableChange.RenameTable)
actual[2]).getNewName());
+ }
+
@Test
void testDeletionCanUseProviderValidatedNativeNamesWithoutRelaxingCreation()
throws Exception {
TableDispatcher delegate = Mockito.mock(TableDispatcher.class);
