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 c17fc03b01 [#13447] fix(core): re-insert updated columns at their
current index in bundled alters (#13448)
c17fc03b01 is described below
commit c17fc03b01730cbdd5727d9964a1b7caa7b32e04
Author: YangJie <[email protected]>
AuthorDate: Sat Oct 10 07:53:22 2026 -0400
[#13447] fix(core): re-insert updated columns at their current index in
bundled alters (#13448)
### What changes were proposed in this pull request?
When an alter updates a column without an explicit
`UpdateColumnPosition`, `ManagedTableOperations` now puts the column
back at the list index it was removed from instead of at its stored
position. The explicit-position path is unchanged.
### Why are the changes needed?
The stored position goes stale as soon as an earlier change in the same
alter adds, removes or moves a column. Lance tables in the
lakehouse-generic catalog accept `deleteColumn` and `renameColumn`
together, and web-v2's Edit Table sends them in one request. Deleting a
lower column and renaming a later one then either threw
`IndexOutOfBoundsException` (renaming the last column) or stored the
columns in a different order from the Lance dataset (renaming one in the
middle). In both cases the dataset had already been changed.
Fix: #13447
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
Added
`TestManagedTableOperations.testAlterTableDeleteAndRenameInOneRequest`,
which deletes `col1` and renames the last column, then deletes `col1`
and renames a middle column, and checks the resulting column order. Both
cases fail against the code before this change.
---
.../gravitino/catalog/ManagedTableOperations.java | 15 +++++--
.../catalog/TestManagedTableOperations.java | 48 ++++++++++++++++++++++
2 files changed, 59 insertions(+), 4 deletions(-)
diff --git
a/core/src/main/java/org/apache/gravitino/catalog/ManagedTableOperations.java
b/core/src/main/java/org/apache/gravitino/catalog/ManagedTableOperations.java
index 0e226abcbb..64c9af8b9c 100644
---
a/core/src/main/java/org/apache/gravitino/catalog/ManagedTableOperations.java
+++
b/core/src/main/java/org/apache/gravitino/catalog/ManagedTableOperations.java
@@ -335,10 +335,10 @@ public abstract class ManagedTableOperations implements
TableCatalog {
|| change instanceof TableChange.UpdateColumnPosition
|| change instanceof TableChange.UpdateColumnNullability
|| change instanceof TableChange.UpdateColumnAutoIncrement) {
- int i;
+ int removedIndex;
ColumnEntity oldColumn = null;
- for (i = 0; i < newColumns.size(); i++) {
- ColumnEntity col = newColumns.get(i);
+ for (removedIndex = 0; removedIndex < newColumns.size();
removedIndex++) {
+ ColumnEntity col = newColumns.get(removedIndex);
if (col.name().equals(DOT.join(change.fieldName()))) {
oldColumn = col;
break;
@@ -407,7 +407,14 @@ public abstract class ManagedTableOperations implements
TableCatalog {
newPosition,
newNullable,
newAutoIncrement);
- newColumns.add(newColumn.position(), newColumn);
+ if (newPosition.isPresent()) {
+ newColumns.add(newColumn.position(), newColumn);
+ } else {
+ // Stored positions go stale as sibling changes in the same alter
add,
+ // remove or move columns, so without an explicit position change the
+ // column goes back to the list index it was removed from.
+ newColumns.add(removedIndex, newColumn);
+ }
} else if (change instanceof TableChange.DeleteColumn deleteColumn) {
boolean removed =
diff --git
a/core/src/test/java/org/apache/gravitino/catalog/TestManagedTableOperations.java
b/core/src/test/java/org/apache/gravitino/catalog/TestManagedTableOperations.java
index c660a40b67..cd0ce98ff8 100644
---
a/core/src/test/java/org/apache/gravitino/catalog/TestManagedTableOperations.java
+++
b/core/src/test/java/org/apache/gravitino/catalog/TestManagedTableOperations.java
@@ -336,6 +336,54 @@ public class TestManagedTableOperations {
.build();
}
+ @Test
+ public void testAlterTableDeleteAndRenameInOneRequest() {
+ // Deleting a lower-position column shifts the columns after it, so
renaming the last one in
+ // the same request must not re-insert it past the end of the list.
+ NameIdentifier lastIdent = createThreeColumnTable("rename_last");
+ Table renamedLast =
+ tableOperations.alterTable(
+ lastIdent,
+ TableChange.deleteColumn(new String[] {"col1"}, false),
+ TableChange.renameColumn(new String[] {"col3"}, "col3_renamed"));
+ Assertions.assertArrayEquals(
+ new String[] {"col2", "col3_renamed"},
+
Arrays.stream(renamedLast.columns()).map(Column::name).toArray(String[]::new));
+
+ // After deleting col1 in the same request, renaming col2 must not move it
behind col3.
+ NameIdentifier middleIdent = createThreeColumnTable("rename_middle");
+ Table renamedMiddle =
+ tableOperations.alterTable(
+ middleIdent,
+ TableChange.deleteColumn(new String[] {"col1"}, false),
+ TableChange.renameColumn(new String[] {"col2"}, "col2_renamed"));
+ Assertions.assertArrayEquals(
+ new String[] {"col2_renamed", "col3"},
+
Arrays.stream(renamedMiddle.columns()).map(Column::name).toArray(String[]::new));
+ }
+
+ private NameIdentifier createThreeColumnTable(String tableName) {
+ NameIdentifier tableIdent =
+ NameIdentifierUtil.ofTable(METALAKE_NAME, CATALOG_NAME, SCHEMA_NAME,
tableName);
+ Column[] columns =
+ new Column[] {
+ createColumn("col1", Types.StringType.get(), null),
+ createColumn("col2", Types.IntegerType.get(),
Literals.integerLiteral(1)),
+ createColumn("col3", Types.StringType.get(), null)
+ };
+ tableOperations.createTable(
+ tableIdent,
+ columns,
+ "Test bundled alters",
+ StringIdentifier.newPropertiesWithId(
+ StringIdentifier.fromId(idGenerator.nextId()),
Collections.emptyMap()),
+ new Transform[0],
+ Distributions.NONE,
+ new SortOrder[0],
+ Indexes.EMPTY_INDEXES);
+ return tableIdent;
+ }
+
@Test
public void testAlterTable() {
NameIdentifier table1Ident =
