Copilot commented on code in PR #13448:
URL: https://github.com/apache/gravitino/pull/13448#discussion_r4073238879
##########
core/src/main/java/org/apache/gravitino/catalog/ManagedTableOperations.java:
##########
@@ -407,7 +407,14 @@ private List<ColumnEntity> applyColumnChanges(
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 or
+ // remove columns, so without an explicit position change the column
goes
+ // back to the list index it was removed from.
+ newColumns.add(i, newColumn);
+ }
Review Comment:
The reinsertion relies on `i` representing “the list index the column was
removed from,” but that’s not explicit from the variable name and is easy to
break if the surrounding loop/indexing changes. Consider capturing a clearly
named `removedIndex` at the point of removal (and using that here), or using an
iterator-based approach (`ListIterator#add`) so the reinsertion index is
inherently tied to the removal location.
##########
core/src/test/java/org/apache/gravitino/catalog/TestManagedTableOperations.java:
##########
@@ -336,6 +336,52 @@ private Column createColumn(String name, Type dataType,
Expression defaultValue)
.build();
}
+ @Test
+ public void testAlterTableBundledColumnChanges() {
+ NameIdentifier tableIdent =
+ NameIdentifierUtil.ofTable(METALAKE_NAME, CATALOG_NAME, SCHEMA_NAME,
"bundled");
+ 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);
+
+ // A delete of a lower-position column followed by an update of a
+ // higher-position one must neither crash nor reorder the remaining
columns.
+ Table updated =
+ tableOperations.alterTable(
+ tableIdent,
+ TableChange.deleteColumn(new String[] {"col1"}, false),
+ TableChange.updateColumnComment(new String[] {"col3"}, "updated"));
+ Assertions.assertArrayEquals(
+ new String[] {"col2", "col3"},
+ Arrays.stream(updated.columns()).map(Column::name).toArray());
+
+ // An add at first() followed by an update of a later column must keep
order.
+ Table updated2 =
+ tableOperations.alterTable(
+ tableIdent,
+ TableChange.addColumn(
+ new String[] {"colNew"},
+ Types.StringType.get(),
+ TableChange.ColumnPosition.first()),
+ TableChange.updateColumnComment(new String[] {"col3"}, "updated
again"));
Review Comment:
The new test verifies column ordering, but it doesn’t assert that
`updateColumnComment` actually updated the comment value. Adding assertions on
the updated column’s comment (for both `"updated"` and `"updated again"`) would
ensure the “update” part of the bundled alter is validated, not just the
resulting order.
##########
core/src/test/java/org/apache/gravitino/catalog/TestManagedTableOperations.java:
##########
@@ -336,6 +336,52 @@ private Column createColumn(String name, Type dataType,
Expression defaultValue)
.build();
}
+ @Test
+ public void testAlterTableBundledColumnChanges() {
+ NameIdentifier tableIdent =
+ NameIdentifierUtil.ofTable(METALAKE_NAME, CATALOG_NAME, SCHEMA_NAME,
"bundled");
+ 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);
+
+ // A delete of a lower-position column followed by an update of a
+ // higher-position one must neither crash nor reorder the remaining
columns.
+ Table updated =
+ tableOperations.alterTable(
+ tableIdent,
+ TableChange.deleteColumn(new String[] {"col1"}, false),
+ TableChange.updateColumnComment(new String[] {"col3"}, "updated"));
+ Assertions.assertArrayEquals(
+ new String[] {"col2", "col3"},
+ Arrays.stream(updated.columns()).map(Column::name).toArray());
+
+ // An add at first() followed by an update of a later column must keep
order.
+ Table updated2 =
+ tableOperations.alterTable(
+ tableIdent,
+ TableChange.addColumn(
+ new String[] {"colNew"},
+ Types.StringType.get(),
+ TableChange.ColumnPosition.first()),
+ TableChange.updateColumnComment(new String[] {"col3"}, "updated
again"));
+ Assertions.assertArrayEquals(
+ new String[] {"colNew", "col2", "col3"},
+ Arrays.stream(updated2.columns()).map(Column::name).toArray());
Review Comment:
`Stream#toArray()` returns `Object[]`, which makes the assertion less
type-specific and can hide type issues. Prefer collecting into a `String[]`
(e.g., `toArray(String[]::new)`) so the actual array type matches the expected
`String[]` explicitly.
##########
core/src/test/java/org/apache/gravitino/catalog/TestManagedTableOperations.java:
##########
@@ -336,6 +336,52 @@ private Column createColumn(String name, Type dataType,
Expression defaultValue)
.build();
}
+ @Test
+ public void testAlterTableBundledColumnChanges() {
+ NameIdentifier tableIdent =
+ NameIdentifierUtil.ofTable(METALAKE_NAME, CATALOG_NAME, SCHEMA_NAME,
"bundled");
+ 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);
+
+ // A delete of a lower-position column followed by an update of a
+ // higher-position one must neither crash nor reorder the remaining
columns.
+ Table updated =
+ tableOperations.alterTable(
+ tableIdent,
+ TableChange.deleteColumn(new String[] {"col1"}, false),
+ TableChange.updateColumnComment(new String[] {"col3"}, "updated"));
Review Comment:
The new test verifies column ordering, but it doesn’t assert that
`updateColumnComment` actually updated the comment value. Adding assertions on
the updated column’s comment (for both `"updated"` and `"updated again"`) would
ensure the “update” part of the bundled alter is validated, not just the
resulting order.
##########
core/src/test/java/org/apache/gravitino/catalog/TestManagedTableOperations.java:
##########
@@ -336,6 +336,52 @@ private Column createColumn(String name, Type dataType,
Expression defaultValue)
.build();
}
+ @Test
+ public void testAlterTableBundledColumnChanges() {
+ NameIdentifier tableIdent =
+ NameIdentifierUtil.ofTable(METALAKE_NAME, CATALOG_NAME, SCHEMA_NAME,
"bundled");
+ 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);
+
+ // A delete of a lower-position column followed by an update of a
+ // higher-position one must neither crash nor reorder the remaining
columns.
+ Table updated =
+ tableOperations.alterTable(
+ tableIdent,
+ TableChange.deleteColumn(new String[] {"col1"}, false),
+ TableChange.updateColumnComment(new String[] {"col3"}, "updated"));
+ Assertions.assertArrayEquals(
+ new String[] {"col2", "col3"},
+ Arrays.stream(updated.columns()).map(Column::name).toArray());
Review Comment:
`Stream#toArray()` returns `Object[]`, which makes the assertion less
type-specific and can hide type issues. Prefer collecting into a `String[]`
(e.g., `toArray(String[]::new)`) so the actual array type matches the expected
`String[]` explicitly.
--
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]
