This is an automated email from the ASF dual-hosted git repository.
jerryshao pushed a commit to branch branch-1.3
in repository https://gitbox.apache.org/repos/asf/gravitino.git
The following commit(s) were added to refs/heads/branch-1.3 by this push:
new 7e78024546 [Cherry-pick to branch-1.3] [#13309][#13310] fix(core):
Keep column ids stable and pass the new schema on cross-schema table rename
(#13316) (#13320)
7e78024546 is described below
commit 7e78024546f3a5e9eb85b8f704473e647393e6f9
Author: Jerry Shao <[email protected]>
AuthorDate: Fri Sep 18 22:16:04 2026 +0800
[Cherry-pick to branch-1.3] [#13309][#13310] fix(core): Keep column ids
stable and pass the new schema on cross-schema table rename (#13316) (#13320)
> Backport of #13316 to `branch-1.3`.
### What changes were proposed in this pull request?
Backport of #13316 to `branch-1.3`, **except for the re-import fix**
(see below).
- **Cross-schema table move with a column change (#13309):**
`TableColumnMetaService.updateColumnPOsFromTableDiff` moves all of the
table's column rows to the new `schema_id` whenever the table moves.
Before, it did this only when no column changed, so a later cascade drop
of the old schema deleted the unchanged columns and their relations.
- **Lance `VERSION_CHECK` schema refresh (#13309):**
`LanceTableOperations.replaceColumnsFromDataset` reuses the id, comment
and audit info of each column still in the dataset, so column tags and
comments survive a dataset version change.
- **Cross-schema table rename and authorization (#13310):**
`TableHookDispatcher.alterTable` passes the new schema from
`RenameTable.getNewSchemaName()` to the authorization plugins, through a
new `AuthorizationUtils.authorizationPluginRenamePrivileges` overload
that takes the new `NameIdentifier`.
- **Ranger (Hive) plugin on a cross-schema table rename (#13310):**
`RangerAuthorizationHadoopSQLPlugin` now skips the schema-level step of
a table rename, which would otherwise move every policy of the old
schema to the new one, and moves only the table's own policies to the
new schema and name.
**Not backported: keeping column ids when a table is re-imported** (e.g.
loaded after an out-of-band rename). On `branch-1.3`,
`insertTable(overwrite = true)` resets the table version and
`RelationalEntityStore.put(overwrite)` caches the incoming entity.
Reusing column ids there would also need cache and column-query changes,
which is too risky for this release. That fix stays on `main` only.
The Lance change is a clean cherry-pick. So is the authorization change.
The schema-move change is the corresponding part of the first commit of
#13316, ported by hand.
### Why are the changes needed?
- Moving a table to another schema and changing a column in one
`alterTable`, then dropping the old schema, deleted the unchanged
columns and their tags, owners and privileges.
- On a Lance catalog with `lance.schema-refresh-mode=VERSION_CHECK`,
every dataset version change gave all columns new ids and cleared their
comments.
- On a cross-schema rename, authorization plugins were told the table
moved to `<old schema>.<new name>`.
Fix: #13309, #13310
Part of #13303
### Does this PR introduce _any_ user-facing change?
No API or configuration change. Column tags, owners and privileges
survive a table move with a column change and a Lance schema refresh.
Lance columns keep their comments. Authorization plugins get the correct
schema on a cross-schema table rename.
### How was this patch tested?
- **Unit tests** (H2 locally; each fails without its fix):
-
`TestTableColumnMetaService.testMoveTableWithColumnChangeMovesAllColumns`
-
`TestLanceTableOperations.testVersionCheckRefreshKeepsExistingColumnIdsAndComments`
- `TestTableHookDispatcher`:
`testRenameAcrossSchemasPassesNewSchemaToAuthorization`,
`testRenameTwiceKeepsSchemaFromEarlierRename`
-
`TestAuthorizationUtils.testRenameTableAcrossSchemasNotifiesAuthorizationPluginWithNewSchema`
- `TestRangerAuthorizationHadoopSQLPlugin`: a table rename across
schemas and within one schema
- **Integration tests**:
-
`CatalogGenericCatalogLanceIT.testVersionCheckRefreshKeepsColumnTagsAndComments`
passes locally.
- `TagIT.testMovedTableKeepsColumnTagsAfterOldSchemaIsDropped` needs
Docker.
- `./gradlew :core:test -PskipITs` and the `catalog-lakehouse-generic`
tests pass locally on this branch. The MySQL/PostgreSQL backends and the
Docker-based ITs still need to run in CI.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
---------
Co-authored-by: Claude Opus 5 <[email protected]>
---
.../ranger/RangerAuthorizationHadoopSQLPlugin.java | 27 ++--
.../TestRangerAuthorizationHadoopSQLPlugin.java | 147 +++++++++++++++++++++
.../lakehouse/lance/LanceTableOperations.java | 30 ++++-
.../lakehouse/lance/TestLanceTableOperations.java | 72 ++++++++++
.../test/CatalogGenericCatalogLanceIT.java | 90 +++++++++++++
.../gravitino/client/integration/test/TagIT.java | 48 +++++++
.../authorization/AuthorizationUtils.java | 24 +++-
.../apache/gravitino/hook/TableHookDispatcher.java | 15 ++-
.../relational/service/TableColumnMetaService.java | 17 ++-
.../authorization/TestAuthorizationUtils.java | 36 +++++
.../gravitino/hook/TestTableHookDispatcher.java | 52 +++++++-
.../service/TestTableColumnMetaService.java | 59 +++++++++
12 files changed, 593 insertions(+), 24 deletions(-)
diff --git
a/authorizations/authorization-ranger/src/main/java/org/apache/gravitino/authorization/ranger/RangerAuthorizationHadoopSQLPlugin.java
b/authorizations/authorization-ranger/src/main/java/org/apache/gravitino/authorization/ranger/RangerAuthorizationHadoopSQLPlugin.java
index 3f1ca387f5..35296e4611 100644
---
a/authorizations/authorization-ranger/src/main/java/org/apache/gravitino/authorization/ranger/RangerAuthorizationHadoopSQLPlugin.java
+++
b/authorizations/authorization-ranger/src/main/java/org/apache/gravitino/authorization/ranger/RangerAuthorizationHadoopSQLPlugin.java
@@ -170,6 +170,12 @@ public class RangerAuthorizationHadoopSQLPlugin extends
RangerAuthorizationPlugi
for (int index = 0; index < mappingOldAndNewMetadata.size(); index++) {
oldMetadataNames.add(mappingOldAndNewMetadata.get(index).getKey());
newMetadataNames.add(mappingOldAndNewMetadata.get(index).getValue());
+ // A table rename never changes the schema-level policies. When the
table also moves to
+ // another schema, updating them would move every policy of the old
schema, so the table's
+ // own policies are updated from the table level down instead.
+ if (index == 0 && newAuthzMetadataObject.type().equals(TABLE)) {
+ continue;
+ }
AuthorizationMetadataObject.Type type;
if (index == 0) {
@@ -235,16 +241,21 @@ public class RangerAuthorizationHadoopSQLPlugin extends
RangerAuthorizationPlugi
// Doesn't need to rename the policy `*`
return;
}
- policyNames.set(index,
newAuthzMetaObject.names().get(index));
+ // Every level up to the renamed one takes its new name,
because a renamed
+ // table may also have moved to another schema.
+ for (int i = 0; i <= index; i++) {
+ policyNames.set(i, newAuthzMetaObject.names().get(i));
+ }
policy.setName(AuthorizationSecurableObject.DOT_JOINER.join(policyNames));
}
- // Update the policy resource name to new name
- policy
- .getResources()
- .put(
- policyResourceDefinesRule().get(index),
- new RangerPolicy.RangerPolicyResource(
- newAuthzMetaObject.names().get(index)));
+ // Update the policy resource names, up to the renamed level,
to the new names
+ for (int i = 0; i <= index; i++) {
+ policy
+ .getResources()
+ .put(
+ policyResourceDefinesRule().get(i),
+ new
RangerPolicy.RangerPolicyResource(newAuthzMetaObject.names().get(i)));
+ }
boolean alreadyExist =
existNewPolicies.stream()
diff --git
a/authorizations/authorization-ranger/src/test/java/org/apache/gravitino/authorization/ranger/TestRangerAuthorizationHadoopSQLPlugin.java
b/authorizations/authorization-ranger/src/test/java/org/apache/gravitino/authorization/ranger/TestRangerAuthorizationHadoopSQLPlugin.java
new file mode 100644
index 0000000000..5b268bd12d
--- /dev/null
+++
b/authorizations/authorization-ranger/src/test/java/org/apache/gravitino/authorization/ranger/TestRangerAuthorizationHadoopSQLPlugin.java
@@ -0,0 +1,147 @@
+/*
+ * 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.authorization.ranger;
+
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyLong;
+
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Objects;
+import java.util.stream.Collectors;
+import org.apache.gravitino.MetadataObject;
+import org.apache.gravitino.MetadataObjects;
+import org.apache.gravitino.authorization.MetadataObjectChange;
+import
org.apache.gravitino.authorization.ranger.reference.RangerDefines.PolicyResource;
+import org.apache.ranger.plugin.model.RangerPolicy;
+import org.apache.ranger.plugin.util.SearchFilter;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.mockito.Mockito;
+
+public class TestRangerAuthorizationHadoopSQLPlugin {
+
+ private final List<RangerPolicy> policies = new ArrayList<>();
+ private RangerAuthorizationHadoopSQLPlugin plugin;
+
+ @BeforeEach
+ public void setUp() throws Exception {
+ policies.clear();
+ // The constructor connects to Ranger, so only the tested methods run for
real.
+ plugin = Mockito.mock(RangerAuthorizationHadoopSQLPlugin.class,
Mockito.CALLS_REAL_METHODS);
+ RangerClientExtension rangerClient =
Mockito.mock(RangerClientExtension.class);
+ // An in-memory Ranger: a search returns the policies whose resources
match every filter.
+ Mockito.when(rangerClient.findPolicies(any()))
+ .thenAnswer(invocation -> findPolicies(invocation.getArgument(0)));
+ Mockito.when(rangerClient.updatePolicy(anyLong(), any()))
+ .thenAnswer(invocation -> invocation.getArgument(1));
+ plugin.setRangerClient(rangerClient);
+ }
+
+ @Test
+ public void testRenameTableAcrossSchemasMovesOnlyThatTablesPolicies() {
+ RangerPolicy schemaPolicy = addPolicy(1, "s1", "s1", null, null);
+ RangerPolicy tablePolicy = addPolicy(2, "s1.t1", "s1", "t1", null);
+ RangerPolicy columnPolicy = addPolicy(3, "s1.t1.*", "s1", "t1", "*");
+ RangerPolicy otherTablePolicy = addPolicy(4, "s1.t9", "s1", "t9", null);
+
+ plugin.onMetadataUpdated(
+ MetadataObjectChange.rename(
+ MetadataObjects.parse("catalog.s1.t1", MetadataObject.Type.TABLE),
+ MetadataObjects.parse("catalog.s2.t2", MetadataObject.Type.TABLE),
+ null));
+
+ // The renamed table's policies follow it to the new schema and name.
+ Assertions.assertEquals("s2.t2", tablePolicy.getName());
+ Assertions.assertEquals(resources("s2", "t2", null),
resourcesOf(tablePolicy));
+ Assertions.assertEquals(resources("s2", "t2", "*"),
resourcesOf(columnPolicy));
+ // The old schema and its other tables keep their policies.
+ Assertions.assertEquals("s1", schemaPolicy.getName());
+ Assertions.assertEquals(resources("s1", null, null),
resourcesOf(schemaPolicy));
+ Assertions.assertEquals("s1.t9", otherTablePolicy.getName());
+ Assertions.assertEquals(resources("s1", "t9", null),
resourcesOf(otherTablePolicy));
+ }
+
+ @Test
+ public void testRenameTableInSameSchemaRenamesOnlyThatTablesPolicies() {
+ RangerPolicy schemaPolicy = addPolicy(1, "s1", "s1", null, null);
+ RangerPolicy tablePolicy = addPolicy(2, "s1.t1", "s1", "t1", null);
+ RangerPolicy otherTablePolicy = addPolicy(3, "s1.t9", "s1", "t9", null);
+
+ plugin.onMetadataUpdated(
+ MetadataObjectChange.rename(
+ MetadataObjects.parse("catalog.s1.t1", MetadataObject.Type.TABLE),
+ MetadataObjects.parse("catalog.s1.t2", MetadataObject.Type.TABLE),
+ null));
+
+ Assertions.assertEquals("s1.t2", tablePolicy.getName());
+ Assertions.assertEquals(resources("s1", "t2", null),
resourcesOf(tablePolicy));
+ Assertions.assertEquals(resources("s1", null, null),
resourcesOf(schemaPolicy));
+ Assertions.assertEquals(resources("s1", "t9", null),
resourcesOf(otherTablePolicy));
+ }
+
+ private RangerPolicy addPolicy(long id, String name, String db, String
table, String column) {
+ RangerPolicy policy = new RangerPolicy();
+ policy.setId(id);
+ policy.setName(name);
+ Map<String, RangerPolicy.RangerPolicyResource> policyResources = new
HashMap<>();
+ resources(db, table, column)
+ .forEach((k, v) -> policyResources.put(k, new
RangerPolicy.RangerPolicyResource(v)));
+ policy.setResources(policyResources);
+ policies.add(policy);
+ return policy;
+ }
+
+ private List<RangerPolicy> findPolicies(Map<String, String> filters) {
+ return policies.stream()
+ .filter(
+ policy ->
+ filters.entrySet().stream()
+ .filter(e ->
e.getKey().startsWith(SearchFilter.RESOURCE_PREFIX))
+ .allMatch(
+ e ->
+ Objects.equals(
+ resourcesOf(policy)
+ .get(
+ e.getKey()
+
.substring(SearchFilter.RESOURCE_PREFIX.length())),
+ e.getValue())))
+ .collect(Collectors.toList());
+ }
+
+ private static Map<String, String> resources(String db, String table, String
column) {
+ Map<String, String> resources = new HashMap<>();
+ resources.put(PolicyResource.DATABASE.getName(), db);
+ if (table != null) {
+ resources.put(PolicyResource.TABLE.getName(), table);
+ }
+ if (column != null) {
+ resources.put(PolicyResource.COLUMN.getName(), column);
+ }
+ return resources;
+ }
+
+ private static Map<String, String> resourcesOf(RangerPolicy policy) {
+ return policy.getResources().entrySet().stream()
+ .collect(Collectors.toMap(Map.Entry::getKey, e ->
e.getValue().getValues().get(0)));
+ }
+}
diff --git
a/catalogs/catalog-lakehouse-generic/src/main/java/org/apache/gravitino/catalog/lakehouse/lance/LanceTableOperations.java
b/catalogs/catalog-lakehouse-generic/src/main/java/org/apache/gravitino/catalog/lakehouse/lance/LanceTableOperations.java
index fb2bfaba1f..86debc4f03 100644
---
a/catalogs/catalog-lakehouse-generic/src/main/java/org/apache/gravitino/catalog/lakehouse/lance/LanceTableOperations.java
+++
b/catalogs/catalog-lakehouse-generic/src/main/java/org/apache/gravitino/catalog/lakehouse/lance/LanceTableOperations.java
@@ -715,12 +715,36 @@ public class LanceTableOperations extends
ManagedTableOperations {
.withCreator(PrincipalUtils.getCurrentPrincipal().getName())
.withCreateTime(Instant.now())
.build();
+ // Tags, owners and privileges are attached to a column by id, so a column
that is still in the
+ // dataset keeps its id. It also keeps its comment and audit info, which
the dataset doesn't
+ // carry.
+ Map<String, ColumnEntity> existingColumns =
+ tableEntity.columns().stream()
+ .collect(
+ Collectors.toMap(
+ ColumnEntity::name, Function.identity(), (first, second)
-> first));
List<ColumnEntity> columnEntities =
IntStream.range(0, columns.length)
.mapToObj(
- i ->
- ColumnEntity.toColumnEntity(
- columns[i], i, idGenerator.nextId(), columnAuditInfo))
+ i -> {
+ ColumnEntity existing =
existingColumns.get(columns[i].name());
+ if (existing == null) {
+ return ColumnEntity.toColumnEntity(
+ columns[i], i, idGenerator.nextId(), columnAuditInfo);
+ }
+ return ColumnEntity.builder()
+ .withId(existing.id())
+ .withName(columns[i].name())
+ .withPosition(i)
+ .withDataType(columns[i].dataType())
+ .withComment(
+ columns[i].comment() != null ? columns[i].comment()
: existing.comment())
+ .withNullable(columns[i].nullable())
+ .withAutoIncrement(columns[i].autoIncrement())
+ .withDefaultValue(columns[i].defaultValue())
+ .withAuditInfo((AuditInfo) existing.auditInfo())
+ .build();
+ })
.collect(Collectors.toList());
return TableEntity.builder()
diff --git
a/catalogs/catalog-lakehouse-generic/src/test/java/org/apache/gravitino/catalog/lakehouse/lance/TestLanceTableOperations.java
b/catalogs/catalog-lakehouse-generic/src/test/java/org/apache/gravitino/catalog/lakehouse/lance/TestLanceTableOperations.java
index f5b092fbf5..3545e2beda 100644
---
a/catalogs/catalog-lakehouse-generic/src/test/java/org/apache/gravitino/catalog/lakehouse/lance/TestLanceTableOperations.java
+++
b/catalogs/catalog-lakehouse-generic/src/test/java/org/apache/gravitino/catalog/lakehouse/lance/TestLanceTableOperations.java
@@ -331,6 +331,78 @@ public class TestLanceTableOperations {
Assertions.assertEquals("9",
loadedTable.properties().get(LANCE_TABLE_VERSION));
}
+ @Test
+ public void testVersionCheckRefreshKeepsExistingColumnIdsAndComments()
throws Exception {
+ lanceTableOps.setCatalogProperties(Map.of(LANCE_SCHEMA_REFRESH_MODE,
"version-check"));
+ NameIdentifier ident = NameIdentifier.of("schema", "table");
+ String location = tempDir.resolve("version-check-keep-ids").toString();
+ AuditInfo columnAudit =
+
AuditInfo.builder().withCreator("column_creator").withCreateTime(Instant.EPOCH).build();
+ TableEntity tableEntity =
+ tableEntity(
+ ident,
+ List.of(
+ ColumnEntity.builder()
+ .withId(10L)
+ .withName("id")
+ .withComment("the id")
+ .withDataType(Types.IntegerType.get())
+ .withPosition(0)
+ .withAuditInfo(columnAudit)
+ .build(),
+ ColumnEntity.builder()
+ .withId(11L)
+ .withName("dropped")
+ .withDataType(Types.StringType.get())
+ .withPosition(1)
+ .withAuditInfo(columnAudit)
+ .build()),
+ Map.of(Table.PROPERTY_LOCATION, location, LANCE_TABLE_VERSION,
"8"));
+ when(store.get(eq(ident), eq(Entity.EntityType.TABLE),
eq(TableEntity.class)))
+ .thenReturn(tableEntity);
+ when(idGenerator.nextId()).thenReturn(12L);
+ AtomicReference<TableEntity> updated = new AtomicReference<>();
+ when(store.update(eq(ident), eq(TableEntity.class),
eq(Entity.EntityType.TABLE), any()))
+ .thenAnswer(
+ invocation -> {
+ @SuppressWarnings("unchecked")
+ Function<TableEntity, TableEntity> updater =
invocation.getArgument(3);
+ updated.set(updater.apply(tableEntity));
+ return updated.get();
+ });
+
+ // The dataset moved to a new version: "id" is still there, "dropped" is
gone and "name" is new.
+ Dataset dataset = mock(Dataset.class);
+ when(dataset.getSchema())
+ .thenReturn(
+ new Schema(
+ List.of(
+ Field.nullable("name", new ArrowType.Utf8()),
+ Field.nullable("id", new ArrowType.Int(32, true)))));
+ when(dataset.version()).thenReturn(9L);
+ Mockito.doReturn(dataset).when(lanceTableOps).openDataset(location,
Map.of());
+
+ Table loadedTable =
+ PrincipalUtils.doAs(new UserPrincipal("tester"), () ->
lanceTableOps.loadTable(ident));
+
+ Assertions.assertEquals("9",
loadedTable.properties().get(LANCE_TABLE_VERSION));
+ List<ColumnEntity> columns = updated.get().columns();
+ Assertions.assertEquals(2, columns.size());
+
+ ColumnEntity name = columns.get(0);
+ Assertions.assertEquals("name", name.name());
+ Assertions.assertEquals(12L, name.id());
+ Assertions.assertEquals(0, name.position());
+
+ ColumnEntity id = columns.get(1);
+ Assertions.assertEquals("id", id.name());
+ Assertions.assertEquals(10L, id.id());
+ Assertions.assertEquals(1, id.position());
+ Assertions.assertEquals("the id", id.comment());
+ Assertions.assertEquals(columnAudit, id.auditInfo());
+ Assertions.assertEquals("the id", loadedTable.columns()[1].comment());
+ }
+
@Test
public void testVersionCheckSkipsRefreshWhenVersionIsCurrent() throws
Exception {
lanceTableOps.setCatalogProperties(Map.of(LANCE_SCHEMA_REFRESH_MODE,
"version-check"));
diff --git
a/catalogs/catalog-lakehouse-generic/src/test/java/org/apache/gravitino/catalog/lakehouse/lance/integration/test/CatalogGenericCatalogLanceIT.java
b/catalogs/catalog-lakehouse-generic/src/test/java/org/apache/gravitino/catalog/lakehouse/lance/integration/test/CatalogGenericCatalogLanceIT.java
index 2654bec7a5..277ed1ec6b 100644
---
a/catalogs/catalog-lakehouse-generic/src/test/java/org/apache/gravitino/catalog/lakehouse/lance/integration/test/CatalogGenericCatalogLanceIT.java
+++
b/catalogs/catalog-lakehouse-generic/src/test/java/org/apache/gravitino/catalog/lakehouse/lance/integration/test/CatalogGenericCatalogLanceIT.java
@@ -19,6 +19,7 @@
package org.apache.gravitino.catalog.lakehouse.lance.integration.test;
import static
org.apache.gravitino.lance.common.utils.LanceConstants.LANCE_CREATION_MODE;
+import static
org.apache.gravitino.lance.common.utils.LanceConstants.LANCE_SCHEMA_REFRESH_MODE;
import static
org.apache.gravitino.lance.common.utils.LanceConstants.LANCE_TABLE_DECLARED;
import static
org.apache.gravitino.lance.common.utils.LanceConstants.LANCE_TABLE_FORMAT;
import static
org.apache.gravitino.lance.common.utils.LanceConstants.LANCE_TABLE_REGISTER;
@@ -48,6 +49,7 @@ import org.apache.arrow.vector.types.pojo.ArrowType;
import org.apache.arrow.vector.types.pojo.Field;
import org.apache.commons.io.FileUtils;
import org.apache.gravitino.Catalog;
+import org.apache.gravitino.MetadataObject;
import org.apache.gravitino.NameIdentifier;
import org.apache.gravitino.Schema;
import org.apache.gravitino.client.GravitinoMetalake;
@@ -56,6 +58,7 @@ import org.apache.gravitino.integration.test.util.BaseIT;
import org.apache.gravitino.integration.test.util.GravitinoITUtils;
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;
@@ -419,6 +422,86 @@ public class CatalogGenericCatalogLanceIT extends BaseIT {
RuntimeException.class, () ->
catalog.asTableCatalog().loadTable(newNameIdentifier));
}
+ @Test
+ void testVersionCheckRefreshKeepsColumnTagsAndComments() {
+ String refreshCatalogName =
GravitinoITUtils.genRandomName("lance_version_check_catalog");
+ Catalog refreshCatalog =
+ metalake.createCatalog(
+ refreshCatalogName,
+ Catalog.Type.RELATIONAL,
+ provider,
+ "comment",
+ ImmutableMap.of(LANCE_SCHEMA_REFRESH_MODE, "VERSION_CHECK"));
+ String refreshSchemaName = GravitinoITUtils.genRandomName(SCHEMA_PREFIX);
+ String tagName = GravitinoITUtils.genRandomName("lance_version_check_tag");
+ try {
+ refreshCatalog
+ .asSchemas()
+ .createSchema(refreshSchemaName, "comment",
createSchemaProperties());
+ NameIdentifier ident =
+ NameIdentifier.of(refreshSchemaName,
GravitinoITUtils.genRandomName(TABLE_PREFIX));
+ Map<String, String> properties = createProperties();
+ properties.put(Table.PROPERTY_TABLE_FORMAT, LANCE_TABLE_FORMAT);
+ properties.put(Table.PROPERTY_LOCATION, tempDirectory + "/" +
ident.name());
+ TableCatalog tableCatalog = refreshCatalog.asTableCatalog();
+ Table created =
+ tableCatalog.createTable(
+ ident,
+ new Column[] {
+ Column.of(LANCE_COL_NAME1, Types.IntegerType.get(),
"col_1_comment"),
+ Column.of(LANCE_COL_NAME2, Types.StringType.get(),
"col_2_comment")
+ },
+ TABLE_COMMENT,
+ properties,
+ Transforms.EMPTY_TRANSFORM,
+ Distributions.NONE,
+ new SortOrder[0]);
+
+ metalake.createTag(tagName, "comment", Collections.emptyMap());
+ findColumn(tableCatalog.loadTable(ident), LANCE_COL_NAME1)
+ .supportsTags()
+ .associateTags(new String[] {tagName}, null);
+
+ // Change the dataset outside Gravitino: keep column 1, drop column 2
and add column 3. This
+ // writes a new dataset version, so the next load refreshes the columns
from the dataset.
+ org.apache.arrow.vector.types.pojo.Schema newSchema =
+ new org.apache.arrow.vector.types.pojo.Schema(
+ Arrays.asList(
+ Field.nullable(LANCE_COL_NAME1, new ArrowType.Int(32, true)),
+ Field.nullable(LANCE_COL_NAME3, new ArrowType.Utf8())));
+ try (RootAllocator allocator = new RootAllocator();
+ Dataset ignored =
+ Dataset.write()
+ .allocator(allocator)
+ .schema(newSchema)
+ .uri(created.properties().get(Table.PROPERTY_LOCATION))
+ .mode(WriteParams.WriteMode.OVERWRITE)
+ .execute()) {
+ // The new dataset version is written.
+ }
+
+ Table refreshed = tableCatalog.loadTable(ident);
+ Assertions.assertArrayEquals(
+ new String[] {LANCE_COL_NAME1, LANCE_COL_NAME3},
+
Arrays.stream(refreshed.columns()).map(Column::name).toArray(String[]::new));
+
+ // Column 1 keeps its id, so it keeps its tag and its comment.
+ Column column1 = findColumn(refreshed, LANCE_COL_NAME1);
+ Assertions.assertEquals("col_1_comment", column1.comment());
+ Assertions.assertArrayEquals(new String[] {tagName},
column1.supportsTags().listTags());
+ MetadataObject[] objects =
metalake.getTag(tagName).associatedObjects().objects();
+ Assertions.assertEquals(1, objects.length);
+ Assertions.assertEquals(
+ String.join(".", refreshCatalogName, refreshSchemaName,
ident.name(), LANCE_COL_NAME1),
+ objects[0].fullName());
+ Assertions.assertEquals(
+ 0, findColumn(refreshed,
LANCE_COL_NAME3).supportsTags().listTags().length);
+ } finally {
+ metalake.deleteTag(tagName);
+ metalake.dropCatalog(refreshCatalogName, true);
+ }
+ }
+
@Test
void testLanceTableFormat() {
String tableName = GravitinoITUtils.genRandomName(TABLE_PREFIX);
@@ -1193,4 +1276,11 @@ public class CatalogGenericCatalogLanceIT extends BaseIT
{
LOG.warn("Failed to delete external table directory: {}",
externalTableLocation, e);
}
}
+
+ private static Column findColumn(Table table, String columnName) {
+ return Arrays.stream(table.columns())
+ .filter(c -> c.name().equals(columnName))
+ .findFirst()
+ .get();
+ }
}
diff --git
a/clients/client-java/src/test/java/org/apache/gravitino/client/integration/test/TagIT.java
b/clients/client-java/src/test/java/org/apache/gravitino/client/integration/test/TagIT.java
index 412bd6329a..9a04f0b2af 100644
---
a/clients/client-java/src/test/java/org/apache/gravitino/client/integration/test/TagIT.java
+++
b/clients/client-java/src/test/java/org/apache/gravitino/client/integration/test/TagIT.java
@@ -862,6 +862,54 @@ public class TagIT extends BaseIT {
}
}
+ @Test
+ public void testMovedTableKeepsColumnTagsAfterOldSchemaIsDropped() {
+ String sourceSchema =
GravitinoITUtils.genRandomName("tag_it_move_source_schema");
+ relationalCatalog.asSchemas().createSchema(sourceSchema, "comment",
Collections.emptyMap());
+ NameIdentifier sourceIdent =
+ NameIdentifier.of(sourceSchema,
GravitinoITUtils.genRandomName("tag_it_move_table"));
+ relationalCatalog
+ .asTableCatalog()
+ .createTable(
+ sourceIdent,
+ new Column[] {
+ Column.of("c1", Types.IntegerType.get()), Column.of("c2",
Types.StringType.get())
+ },
+ "comment",
+ Collections.emptyMap());
+ NameIdentifier movedIdent = NameIdentifier.of(schema.name(),
sourceIdent.name());
+ try {
+ Tag tag =
+ metalake.createTag(
+ GravitinoITUtils.genRandomName("tag_it_move_table_tag"),
+ "comment",
+ Collections.emptyMap());
+ loadColumn(sourceIdent, "c1").supportsTags().associateTags(new String[]
{tag.name()}, null);
+
+ // Move the table to another schema and change another column in the
same request.
+ relationalCatalog
+ .asTableCatalog()
+ .alterTable(
+ sourceIdent,
+ TableChange.rename(sourceIdent.name(), schema.name()),
+ TableChange.updateColumnComment(new String[] {"c2"}, "new
comment"));
+
+ // Dropping the old schema must not take the moved table's unchanged
column with it.
+
Assertions.assertTrue(relationalCatalog.asSchemas().dropSchema(sourceSchema,
true));
+
+ Column c1 = loadColumn(movedIdent, "c1");
+ Assertions.assertArrayEquals(new String[] {tag.name()},
c1.supportsTags().listTags());
+ MetadataObject[] objects =
metalake.getTag(tag.name()).associatedObjects().objects();
+ Assertions.assertEquals(1, objects.length);
+ Assertions.assertEquals(
+ String.join(".", relationalCatalog.name(), schema.name(),
movedIdent.name(), "c1"),
+ objects[0].fullName());
+ } finally {
+ relationalCatalog.asTableCatalog().dropTable(movedIdent);
+ relationalCatalog.asSchemas().dropSchema(sourceSchema, true);
+ }
+ }
+
private NameIdentifier createColumnTestTable(String prefix) {
NameIdentifier tableIdent =
NameIdentifier.of(schema.name(),
GravitinoITUtils.genRandomName(prefix));
diff --git
a/core/src/main/java/org/apache/gravitino/authorization/AuthorizationUtils.java
b/core/src/main/java/org/apache/gravitino/authorization/AuthorizationUtils.java
index 11c380d669..0545baf3fa 100644
---
a/core/src/main/java/org/apache/gravitino/authorization/AuthorizationUtils.java
+++
b/core/src/main/java/org/apache/gravitino/authorization/AuthorizationUtils.java
@@ -367,18 +367,36 @@ public class AuthorizationUtils {
public static void authorizationPluginRenamePrivileges(
NameIdentifier ident, Entity.EntityType type, String newName,
List<String> locations) {
+ authorizationPluginRenamePrivileges(
+ ident, type, NameIdentifier.of(ident.namespace(), newName), locations);
+ }
+
+ /**
+ * Renames the privileges of an entity in the authorization plugins, when
the new identifier may
+ * be under a different parent, e.g. a table moved to another schema.
+ *
+ * @param ident the identifier of the entity before the rename
+ * @param type the entity type
+ * @param newIdent the identifier of the entity after the rename
+ * @param locations the storage locations of the entity, or {@code null}
+ */
+ public static void authorizationPluginRenamePrivileges(
+ NameIdentifier ident,
+ Entity.EntityType type,
+ NameIdentifier newIdent,
+ List<String> locations) {
// If we enable authorization, we should rename the privileges about the
entity in the
// authorization plugin.
if (GravitinoEnv.getInstance().internalAccessControlDispatcher() != null) {
notifyEntityNameIdMappingChange(ident, type);
MetadataObject oldMetadataObject =
NameIdentifierUtil.toMetadataObject(ident, type);
- MetadataObject newMetadataObject =
-
NameIdentifierUtil.toMetadataObject(NameIdentifier.of(ident.namespace(),
newName), type);
+ MetadataObject newMetadataObject =
NameIdentifierUtil.toMetadataObject(newIdent, type);
MetadataObjectChange renameChange =
MetadataObjectChange.rename(oldMetadataObject, newMetadataObject,
locations);
- String metalake = type == Entity.EntityType.METALAKE ? newName :
ident.namespace().level(0);
+ String metalake =
+ type == Entity.EntityType.METALAKE ? newIdent.name() :
ident.namespace().level(0);
// For a renamed catalog, we should pass the new name catalog, otherwise
we can't find the
// catalog in the entity store
diff --git
a/core/src/main/java/org/apache/gravitino/hook/TableHookDispatcher.java
b/core/src/main/java/org/apache/gravitino/hook/TableHookDispatcher.java
index 768c1e5410..76dc96d2ed 100644
--- a/core/src/main/java/org/apache/gravitino/hook/TableHookDispatcher.java
+++ b/core/src/main/java/org/apache/gravitino/hook/TableHookDispatcher.java
@@ -104,10 +104,15 @@ public class TableHookDispatcher implements
TableDispatcher {
public Table alterTable(NameIdentifier ident, TableChange... changes)
throws NoSuchTableException, IllegalArgumentException {
TableChange.RenameTable lastRenameChange = null;
+ // Renames apply in order, so the last one that sets a schema decides the
table's schema.
+ String newSchemaName = ident.namespace().level(2);
List<String> locations = null;
for (TableChange change : changes) {
if (change instanceof TableChange.RenameTable) {
lastRenameChange = (TableChange.RenameTable) change;
+ if (lastRenameChange.getNewSchemaName().isPresent()) {
+ newSchemaName = lastRenameChange.getNewSchemaName().get();
+ }
}
}
if (lastRenameChange != null) {
@@ -116,9 +121,15 @@ public class TableHookDispatcher implements
TableDispatcher {
Table alteredTable = dispatcher.alterTable(ident, changes);
if (lastRenameChange != null) {
- // todo: support rename across different schemas
+ // The rename may also move the table to another schema.
+ NameIdentifier newIdent =
+ NameIdentifierUtil.ofTable(
+ ident.namespace().level(0),
+ ident.namespace().level(1),
+ newSchemaName,
+ lastRenameChange.getNewName());
AuthorizationUtils.authorizationPluginRenamePrivileges(
- ident, Entity.EntityType.TABLE, lastRenameChange.getNewName(),
locations);
+ ident, Entity.EntityType.TABLE, newIdent, locations);
}
return alteredTable;
diff --git
a/core/src/main/java/org/apache/gravitino/storage/relational/service/TableColumnMetaService.java
b/core/src/main/java/org/apache/gravitino/storage/relational/service/TableColumnMetaService.java
index 62653e23f7..df79aa7513 100644
---
a/core/src/main/java/org/apache/gravitino/storage/relational/service/TableColumnMetaService.java
+++
b/core/src/main/java/org/apache/gravitino/storage/relational/service/TableColumnMetaService.java
@@ -187,15 +187,18 @@ public class TableColumnMetaService {
}
}
+ // If the table moved to another schema, move all of its existing column
rows as well, whether
+ // or not any column changed. Only the changed columns get new rows below,
so otherwise the
+ // unchanged ones would stay under the old schema and be dropped with it.
+ if (!newTable.namespace().equals(oldTable.namespace())) {
+ SessionUtils.doWithoutCommit(
+ TableColumnMapper.class,
+ mapper ->
+ mapper.updateSchemaIdByTableId(newTablePO.getTableId(),
newTablePO.getSchemaId()));
+ }
+
// If there is no change, directly return
if (columnPOsToInsert.isEmpty()) {
- // If namespace is changed, just update the schema_id of the columns.
- if (!newTable.namespace().equals(oldTable.namespace())) {
- SessionUtils.doWithoutCommit(
- TableColumnMapper.class,
- mapper ->
- mapper.updateSchemaIdByTableId(newTablePO.getTableId(),
newTablePO.getSchemaId()));
- }
return;
}
diff --git
a/core/src/test/java/org/apache/gravitino/authorization/TestAuthorizationUtils.java
b/core/src/test/java/org/apache/gravitino/authorization/TestAuthorizationUtils.java
index bef17095d0..0049a36ec2 100644
---
a/core/src/test/java/org/apache/gravitino/authorization/TestAuthorizationUtils.java
+++
b/core/src/test/java/org/apache/gravitino/authorization/TestAuthorizationUtils.java
@@ -425,6 +425,42 @@ class TestAuthorizationUtils {
Assertions.assertEquals(locations, renameChange.locations());
}
+ @Test
+ void testRenameTableAcrossSchemasNotifiesAuthorizationPluginWithNewSchema() {
+ NameIdentifier ident = NameIdentifier.of("metalake", "catalog", "schema",
"table");
+ NameIdentifier newIdent = NameIdentifier.of("metalake", "catalog",
"new_schema", "new_table");
+ List<String> locations = Lists.newArrayList("/warehouse/schema/table");
+
+ AccessControlDispatcher accessControlDispatcher =
Mockito.mock(AccessControlDispatcher.class);
+ CatalogManager catalogManager = Mockito.mock(CatalogManager.class);
+ BaseCatalog<?> baseCatalog = Mockito.mock(BaseCatalog.class);
+ AuthorizationPlugin authorizationPlugin =
Mockito.mock(AuthorizationPlugin.class);
+ CatalogTestUtils.mockDoWithCatalog(catalogManager, baseCatalog);
+
Mockito.when(baseCatalog.getAuthorizationPlugin()).thenReturn(authorizationPlugin);
+
+ GravitinoEnv envMock = Mockito.mock(GravitinoEnv.class);
+
Mockito.when(envMock.internalAccessControlDispatcher()).thenReturn(accessControlDispatcher);
+ Mockito.when(envMock.catalogManager()).thenReturn(catalogManager);
+
+ try (MockedStatic<GravitinoEnv> envStatic =
Mockito.mockStatic(GravitinoEnv.class)) {
+ envStatic.when(GravitinoEnv::getInstance).thenReturn(envMock);
+
+ AuthorizationUtils.authorizationPluginRenamePrivileges(
+ ident, Entity.EntityType.TABLE, newIdent, locations);
+ }
+
+ ArgumentCaptor<MetadataObjectChange[]> changesCaptor =
+ ArgumentCaptor.forClass(MetadataObjectChange[].class);
+
Mockito.verify(authorizationPlugin).onMetadataUpdated(changesCaptor.capture());
+ MetadataObjectChange.RenameMetadataObject renameChange =
+ Assertions.assertInstanceOf(
+ MetadataObjectChange.RenameMetadataObject.class,
changesCaptor.getValue()[0]);
+ Assertions.assertEquals("catalog.schema.table",
renameChange.metadataObject().fullName());
+ Assertions.assertEquals(
+ "catalog.new_schema.new_table",
renameChange.newMetadataObject().fullName());
+ Assertions.assertEquals(locations, renameChange.locations());
+ }
+
@Test
void
testRemoveTablePrivilegesNotifiesAuthorizationPluginWithExpectedChange() {
NameIdentifier ident = NameIdentifier.of("metalake", "catalog", "schema",
"table");
diff --git
a/core/src/test/java/org/apache/gravitino/hook/TestTableHookDispatcher.java
b/core/src/test/java/org/apache/gravitino/hook/TestTableHookDispatcher.java
index befd3a9dd4..6aff59c0e7 100644
--- a/core/src/test/java/org/apache/gravitino/hook/TestTableHookDispatcher.java
+++ b/core/src/test/java/org/apache/gravitino/hook/TestTableHookDispatcher.java
@@ -222,10 +222,60 @@ public class TestTableHookDispatcher {
.when(() -> AuthorizationUtils.getMetadataObjectLocation(ident,
Entity.EntityType.TABLE))
.thenReturn(locations);
assertSame(alteredTable, hook.alterTable(ident, renameChange));
+ NameIdentifier newIdent = NameIdentifier.of(METALAKE, CATALOG, "schema",
"newName");
authorizationUtils.verify(
() ->
AuthorizationUtils.authorizationPluginRenamePrivileges(
- ident, Entity.EntityType.TABLE, "newName", locations));
+ ident, Entity.EntityType.TABLE, newIdent, locations));
+ }
+ }
+
+ @Test
+ public void testRenameAcrossSchemasPassesNewSchemaToAuthorization() {
+ TableDispatcher dispatcher = Mockito.mock(TableDispatcher.class);
+ TableHookDispatcher hook = new TableHookDispatcher(dispatcher, () -> null);
+ NameIdentifier ident = NameIdentifier.of(METALAKE, CATALOG, "schema",
"table");
+ Table alteredTable = Mockito.mock(Table.class);
+ TableChange renameChange = TableChange.rename("newName", "newSchema");
+ List<String> locations = ImmutableList.of("/test");
+ Mockito.when(dispatcher.alterTable(ident,
renameChange)).thenReturn(alteredTable);
+
+ try (MockedStatic<AuthorizationUtils> authorizationUtils =
+ Mockito.mockStatic(AuthorizationUtils.class)) {
+ authorizationUtils
+ .when(() -> AuthorizationUtils.getMetadataObjectLocation(ident,
Entity.EntityType.TABLE))
+ .thenReturn(locations);
+ assertSame(alteredTable, hook.alterTable(ident, renameChange));
+ NameIdentifier newIdent = NameIdentifier.of(METALAKE, CATALOG,
"newSchema", "newName");
+ authorizationUtils.verify(
+ () ->
+ AuthorizationUtils.authorizationPluginRenamePrivileges(
+ ident, Entity.EntityType.TABLE, newIdent, locations));
+ }
+ }
+
+ @Test
+ public void testRenameTwiceKeepsSchemaFromEarlierRename() {
+ TableDispatcher dispatcher = Mockito.mock(TableDispatcher.class);
+ TableHookDispatcher hook = new TableHookDispatcher(dispatcher, () -> null);
+ NameIdentifier ident = NameIdentifier.of(METALAKE, CATALOG, "schema",
"table");
+ Table alteredTable = Mockito.mock(Table.class);
+ // The table dispatcher moves the table to the last schema set by any
rename.
+ TableChange moveChange = TableChange.rename("t2", "newSchema");
+ TableChange renameChange = TableChange.rename("t3");
+ Mockito.when(dispatcher.alterTable(ident, moveChange,
renameChange)).thenReturn(alteredTable);
+
+ try (MockedStatic<AuthorizationUtils> authorizationUtils =
+ Mockito.mockStatic(AuthorizationUtils.class)) {
+ authorizationUtils
+ .when(() -> AuthorizationUtils.getMetadataObjectLocation(ident,
Entity.EntityType.TABLE))
+ .thenReturn(ImmutableList.of());
+ assertSame(alteredTable, hook.alterTable(ident, moveChange,
renameChange));
+ NameIdentifier newIdent = NameIdentifier.of(METALAKE, CATALOG,
"newSchema", "t3");
+ authorizationUtils.verify(
+ () ->
+ AuthorizationUtils.authorizationPluginRenamePrivileges(
+ ident, Entity.EntityType.TABLE, newIdent,
ImmutableList.of()));
}
}
diff --git
a/core/src/test/java/org/apache/gravitino/storage/relational/service/TestTableColumnMetaService.java
b/core/src/test/java/org/apache/gravitino/storage/relational/service/TestTableColumnMetaService.java
index 377cbf8801..7a38cd485e 100644
---
a/core/src/test/java/org/apache/gravitino/storage/relational/service/TestTableColumnMetaService.java
+++
b/core/src/test/java/org/apache/gravitino/storage/relational/service/TestTableColumnMetaService.java
@@ -243,6 +243,65 @@ public class TestTableColumnMetaService extends
TestJDBCBackend {
Assertions.assertEquals(2, countActiveColumnRelations(kept));
}
+ @TestTemplate
+ public void testMoveTableWithColumnChangeMovesAllColumns() throws Exception {
+ String catalogName = "catalog1";
+ String oldSchemaName = "schema1";
+ String newSchemaName = "schema2";
+ createParentEntities(METALAKE_NAME, catalogName, oldSchemaName,
AUDIT_INFO);
+ createAndInsertSchema(METALAKE_NAME, catalogName, newSchemaName);
+
+ ColumnEntity unchanged = newIntColumn("unchanged", 0, "comment");
+ ColumnEntity changed = newIntColumn("changed", 1, "comment");
+ TableEntity table =
+ TableEntity.builder()
+ .withId(RandomIdGenerator.INSTANCE.nextId())
+ .withName("table_move")
+ .withNamespace(Namespace.of(METALAKE_NAME, catalogName,
oldSchemaName))
+ .withColumns(Lists.newArrayList(unchanged, changed))
+ .withAuditInfo(AUDIT_INFO)
+ .build();
+ TableMetaService.getInstance().insertTable(table, false);
+
+ // Move the table to the other schema and change one column in the same
update.
+ ColumnEntity changedColumn = newIntColumn("changed", 1, "new comment",
changed.id());
+ TableEntity moved =
+ TableEntity.builder()
+ .withId(table.id())
+ .withName(table.name())
+ .withNamespace(Namespace.of(METALAKE_NAME, catalogName,
newSchemaName))
+ .withColumns(Lists.newArrayList(unchanged, changedColumn))
+ .withAuditInfo(AUDIT_INFO)
+ .build();
+ TableMetaService.getInstance().updateTable(table.nameIdentifier(),
(TableEntity old) -> moved);
+
+ // Dropping the old schema must not take any of the moved table's columns
with it.
+ Assertions.assertTrue(
+ SchemaMetaService.getInstance()
+ .deleteSchema(NameIdentifier.of(METALAKE_NAME, catalogName,
oldSchemaName), true));
+
+ TableEntity retrieved =
+
TableMetaService.getInstance().getTableByIdentifier(moved.nameIdentifier());
+ compareTwoColumns(moved.columns(), retrieved.columns());
+ }
+
+ private ColumnEntity newIntColumn(String name, int position, String comment)
{
+ return newIntColumn(name, position, comment,
RandomIdGenerator.INSTANCE.nextId());
+ }
+
+ private ColumnEntity newIntColumn(String name, int position, String comment,
long id) {
+ return ColumnEntity.builder()
+ .withId(id)
+ .withName(name)
+ .withPosition(position)
+ .withComment(comment)
+ .withDataType(Types.IntegerType.get())
+ .withNullable(true)
+ .withAutoIncrement(false)
+ .withAuditInfo(AUDIT_INFO)
+ .build();
+ }
+
private void insertColumnRelations(long columnId) throws SQLException {
try (SqlSession session =
SqlSessionFactoryHelper.getInstance().getSqlSessionFactory().openSession(true);
