Copilot commented on code in PR #9127:
URL: https://github.com/apache/gravitino/pull/9127#discussion_r2768568103
##########
lance/lance-rest-server/src/test/java/org/apache/gravitino/lance/integration/test/LanceRESTServiceIT.java:
##########
@@ -599,6 +607,108 @@ void testCreateTable() throws IOException, ApiException {
Set<String> stringSet = listResponse.getTables();
Assertions.assertEquals(1, stringSet.size());
Assertions.assertTrue(stringSet.contains(Joiner.on(".").join(ids)));
+
+ // Now try to drop columns in the table
+ AlterTableDropColumnsRequest dropColumnsRequest = new
AlterTableDropColumnsRequest();
+ dropColumnsRequest.setId(List.of(CATALOG_NAME, SCHEMA_NAME, "table"));
+ dropColumnsRequest.setColumns(List.of("value"));
+
+ // No alterTableDropColumns in Namespace interface, so we need to get
TableApi via reflection
+ RestNamespace restNamespace = (RestNamespace) ns;
+ TableApi tableApi = (TableApi) FieldUtils.readField(restNamespace,
"tableApi", true);
+ String delimiter = RestNamespaceConfig.DELIMITER_DEFAULT;
+
+ AlterTableDropColumnsResponse alterTableDropColumnsResponse =
+ Assertions.assertDoesNotThrow(
+ () ->
+ tableApi.alterTableDropColumns(
+ String.join(delimiter, ids), dropColumnsRequest,
delimiter));
+ Assertions.assertNotNull(alterTableDropColumnsResponse);
+ Assertions.assertNotNull(alterTableDropColumnsResponse.getVersion());
+
+ describeTableRequest.setId(ids);
+ loadTable = ns.describeTable(describeTableRequest);
+ Assertions.assertNotNull(loadTable);
+ Assertions.assertEquals(newLocation, loadTable.getLocation());
+
+ jsonArrowFields = loadTable.getSchema().getFields();
+ Assertions.assertEquals(1, jsonArrowFields.size());
+ for (int i = 0; i < jsonArrowFields.size(); i++) {
+ JsonArrowField jsonArrowField = jsonArrowFields.get(i);
+ Field originalField = schema.getFields().get(i);
+ Assertions.assertEquals(originalField.getName(),
jsonArrowField.getName());
+
+ if (i == 0) {
+ Assertions.assertEquals("int32", jsonArrowField.getType().getType());
+ } else if (i == 1) {
+ Assertions.assertEquals("utf8", jsonArrowField.getType().getType());
+ }
+ }
+
Review Comment:
The loop iterates over `jsonArrowFields.size()` which is 1 after dropping a
column, but then accesses `schema.getFields().get(i)` where `schema` still has
2 fields (the original schema before column drop). This will cause an
IndexOutOfBoundsException when `i=0` because the first field in jsonArrowFields
corresponds to the first field in the original schema, but after dropping
"value", only "id" remains.
The comparison logic is flawed: it's comparing the remaining field(s) in the
table after dropping "value" against the original schema fields. You should
either:
1. Remove this validation loop entirely since you're checking the count
(line 635), or
2. Compare against the actual expected fields after the drop operation
```suggestion
JsonArrowField jsonArrowField = jsonArrowFields.get(0);
Assertions.assertEquals("id", jsonArrowField.getName());
Assertions.assertEquals("int32", jsonArrowField.getType().getType());
```
##########
lance/lance-rest-server/src/test/java/org/apache/gravitino/lance/service/rest/TestLanceNamespaceOperations.java:
##########
@@ -774,4 +780,106 @@ void testDropTable() {
Response.Status.INTERNAL_SERVER_ERROR.getStatusCode(),
resp.getStatus());
Assertions.assertEquals(MediaType.APPLICATION_JSON_TYPE,
resp.getMediaType());
}
+
+ @Test
+ void testDropColumns() {
+ String tableIds = "catalog.scheme.alter_table_drop_columns";
+ String delimiter = ".";
+
+ // first try to create a table and drop columns from it
+ AlterTableDropColumnsResponse dropColumnsResponse = new
AlterTableDropColumnsResponse();
+ dropColumnsResponse.setVersion(2L);
+
+ AlterTableDropColumnsRequest dropColumnsRequest = new
AlterTableDropColumnsRequest();
+ dropColumnsRequest.setColumns(List.of("id"));
+ when(tableOps.alterTableDropColumns(any(), any(),
any())).thenReturn(dropColumnsResponse);
+ Response resp =
+ target(String.format("/v1/table/%s/drop_columns", tableIds))
+ .queryParam("delimiter", delimiter)
+ .request(MediaType.APPLICATION_JSON_TYPE)
+ .post(Entity.entity(dropColumnsRequest,
MediaType.APPLICATION_JSON_TYPE));
+
+ Assertions.assertEquals(Response.Status.OK.getStatusCode(),
resp.getStatus());
+ Assertions.assertEquals(MediaType.APPLICATION_JSON_TYPE,
resp.getMediaType());
+ AlterTableDropColumnsResponse response =
resp.readEntity(AlterTableDropColumnsResponse.class);
+ Assertions.assertEquals(dropColumnsResponse.getVersion(),
response.getVersion());
+
+ // Test runtime exception
+ Mockito.reset(tableOps);
+ when(tableOps.alterTableDropColumns(any(), any(), any()))
+ .thenThrow(new RuntimeException("Runtime exception"));
+ resp =
+ target(String.format("/v1/table/%s/drop_columns", tableIds))
+ .queryParam("delimiter", delimiter)
+ .request(MediaType.APPLICATION_JSON_TYPE)
+ .post(Entity.entity(dropColumnsRequest,
MediaType.APPLICATION_JSON_TYPE));
+ Assertions.assertEquals(
+ Response.Status.INTERNAL_SERVER_ERROR.getStatusCode(),
resp.getStatus());
+ Assertions.assertEquals(MediaType.APPLICATION_JSON_TYPE,
resp.getMediaType());
+
+ // Test No such table exception
+ Mockito.reset(tableOps);
+ when(tableOps.alterTableDropColumns(any(), any(), any()))
+ .thenThrow(
+ LanceNamespaceException.notFound(
+ "Table not found", "NoSuchTableException", tableIds, ""));
+ resp =
+ target(String.format("/v1/table/%s/drop_columns", tableIds))
+ .queryParam("delimiter", delimiter)
+ .request(MediaType.APPLICATION_JSON_TYPE)
+ .post(Entity.entity(dropColumnsRequest,
MediaType.APPLICATION_JSON_TYPE));
+ Assertions.assertEquals(Response.Status.NOT_FOUND.getStatusCode(),
resp.getStatus());
+ }
Review Comment:
The test coverage is missing validation scenarios for the drop_columns
endpoint. Consider adding test cases for:
1. Empty columns list (should trigger the validation and return BAD_REQUEST)
2. Invalid column names
Similar patterns exist in other tests like testCreateTable (line 372-373)
and testRegisterTable (line 487-488) where IllegalArgumentException handling is
tested.
##########
catalogs/catalog-lakehouse-generic/src/main/java/org/apache/gravitino/catalog/lakehouse/lance/LanceTableOperations.java:
##########
@@ -161,31 +163,30 @@ public Table createTable(
@Override
public Table alterTable(NameIdentifier ident, TableChange... changes)
throws NoSuchSchemaException, TableAlreadyExistsException {
- // Lance only supports adding indexes for now.
- boolean onlyAddIndex =
- Arrays.stream(changes).allMatch(change -> change instanceof
TableChange.AddIndex);
- Preconditions.checkArgument(onlyAddIndex, "Only adding indexes is
supported for Lance tables");
-
- List<Index> addedIndexes =
- Arrays.stream(changes)
- .filter(change -> change instanceof TableChange.AddIndex)
- .map(
- change -> {
- TableChange.AddIndex addIndexChange = (TableChange.AddIndex)
change;
- return Indexes.IndexImpl.builder()
- .withIndexType(addIndexChange.getType())
- .withName(addIndexChange.getName())
- .withFieldNames(addIndexChange.getFieldNames())
- .build();
- })
- .collect(Collectors.toList());
Table loadedTable = super.loadTable(ident);
- addLanceIndex(loadedTable, addedIndexes);
- // After adding the index to the Lance dataset, we need to update the
table metadata in
+ long version = handleLanceTableChange(loadedTable, changes);
+ // After making changes to the Lance dataset, we need to update the table
metadata in
// Gravitino. If there's any failure during this process, the code will
throw an exception
// and the update won't be applied in Gravitino.
- return super.alterTable(ident, changes);
+ GenericTable table = (GenericTable) super.alterTable(ident, changes);
+ Map<String, String> updatedProperties =
+ new HashMap<>(table.properties()) {
+ {
+ put(LanceConstants.LANCE_TABLE_VERSION, String.valueOf(version));
+ }
+ };
Review Comment:
Using double-brace initialization creates an anonymous inner class, which
can lead to memory leaks (the inner class holds a reference to the outer class)
and increases the class count. This pattern is generally discouraged in favor
of explicit initialization. Consider using explicit HashMap initialization
instead, similar to the pattern seen in GenericCatalogOperations.java:242 where
`Maps.newHashMap(properties)` is used followed by explicit `put` calls.
```suggestion
Map<String, String> updatedProperties = new
HashMap<>(table.properties());
updatedProperties.put(LanceConstants.LANCE_TABLE_VERSION,
String.valueOf(version));
```
##########
lance/lance-rest-server/src/test/java/org/apache/gravitino/lance/service/rest/TestLanceNamespaceOperations.java:
##########
@@ -774,4 +780,106 @@ void testDropTable() {
Response.Status.INTERNAL_SERVER_ERROR.getStatusCode(),
resp.getStatus());
Assertions.assertEquals(MediaType.APPLICATION_JSON_TYPE,
resp.getMediaType());
}
+
+ @Test
+ void testDropColumns() {
+ String tableIds = "catalog.scheme.alter_table_drop_columns";
+ String delimiter = ".";
+
+ // first try to create a table and drop columns from it
+ AlterTableDropColumnsResponse dropColumnsResponse = new
AlterTableDropColumnsResponse();
+ dropColumnsResponse.setVersion(2L);
+
+ AlterTableDropColumnsRequest dropColumnsRequest = new
AlterTableDropColumnsRequest();
+ dropColumnsRequest.setColumns(List.of("id"));
+ when(tableOps.alterTableDropColumns(any(), any(),
any())).thenReturn(dropColumnsResponse);
+ Response resp =
+ target(String.format("/v1/table/%s/drop_columns", tableIds))
+ .queryParam("delimiter", delimiter)
+ .request(MediaType.APPLICATION_JSON_TYPE)
+ .post(Entity.entity(dropColumnsRequest,
MediaType.APPLICATION_JSON_TYPE));
+
+ Assertions.assertEquals(Response.Status.OK.getStatusCode(),
resp.getStatus());
+ Assertions.assertEquals(MediaType.APPLICATION_JSON_TYPE,
resp.getMediaType());
+ AlterTableDropColumnsResponse response =
resp.readEntity(AlterTableDropColumnsResponse.class);
+ Assertions.assertEquals(dropColumnsResponse.getVersion(),
response.getVersion());
+
+ // Test runtime exception
+ Mockito.reset(tableOps);
+ when(tableOps.alterTableDropColumns(any(), any(), any()))
+ .thenThrow(new RuntimeException("Runtime exception"));
+ resp =
+ target(String.format("/v1/table/%s/drop_columns", tableIds))
+ .queryParam("delimiter", delimiter)
+ .request(MediaType.APPLICATION_JSON_TYPE)
+ .post(Entity.entity(dropColumnsRequest,
MediaType.APPLICATION_JSON_TYPE));
+ Assertions.assertEquals(
+ Response.Status.INTERNAL_SERVER_ERROR.getStatusCode(),
resp.getStatus());
+ Assertions.assertEquals(MediaType.APPLICATION_JSON_TYPE,
resp.getMediaType());
+
+ // Test No such table exception
+ Mockito.reset(tableOps);
+ when(tableOps.alterTableDropColumns(any(), any(), any()))
+ .thenThrow(
+ LanceNamespaceException.notFound(
+ "Table not found", "NoSuchTableException", tableIds, ""));
+ resp =
+ target(String.format("/v1/table/%s/drop_columns", tableIds))
+ .queryParam("delimiter", delimiter)
+ .request(MediaType.APPLICATION_JSON_TYPE)
+ .post(Entity.entity(dropColumnsRequest,
MediaType.APPLICATION_JSON_TYPE));
+ Assertions.assertEquals(Response.Status.NOT_FOUND.getStatusCode(),
resp.getStatus());
+ }
+
+ @Test
+ void testAlterColumns() {
+ String tableIds = "catalog.scheme.alter_table_alter_columns";
+ String delimiter = ".";
+
+ AlterTableAlterColumnsResponse alterColumnsResponse = new
AlterTableAlterColumnsResponse();
+ alterColumnsResponse.setVersion(3L);
+
+ AlterTableAlterColumnsRequest alterColumnsRequest = new
AlterTableAlterColumnsRequest();
+ alterColumnsRequest.setId(List.of("catalog", "scheme",
"alter_table_alter_columns"));
+ ColumnAlteration columnAlteration = new ColumnAlteration();
+ columnAlteration.setColumn("col1");
+ columnAlteration.setRename("col1_new");
+ alterColumnsRequest.setAlterations(List.of(columnAlteration));
+
+ when(tableOps.alterTableAlterColumns(any(), any(),
any())).thenReturn(alterColumnsResponse);
+ Response resp =
+ target(String.format("/v1/table/%s/alter_columns", tableIds))
+ .queryParam("delimiter", delimiter)
+ .request(MediaType.APPLICATION_JSON_TYPE)
+ .post(Entity.entity(alterColumnsRequest,
MediaType.APPLICATION_JSON_TYPE));
+
+ Assertions.assertEquals(Response.Status.OK.getStatusCode(),
resp.getStatus());
+ Assertions.assertEquals(MediaType.APPLICATION_JSON_TYPE,
resp.getMediaType());
+ AlterTableAlterColumnsResponse response =
resp.readEntity(AlterTableAlterColumnsResponse.class);
+ Assertions.assertEquals(alterColumnsResponse.getVersion(),
response.getVersion());
+
+ Mockito.reset(tableOps);
+ when(tableOps.alterTableAlterColumns(any(), any(), any()))
+ .thenThrow(new RuntimeException("Runtime exception"));
+ resp =
+ target(String.format("/v1/table/%s/alter_columns", tableIds))
+ .queryParam("delimiter", delimiter)
+ .request(MediaType.APPLICATION_JSON_TYPE)
+ .post(Entity.entity(alterColumnsRequest,
MediaType.APPLICATION_JSON_TYPE));
+ Assertions.assertEquals(
+ Response.Status.INTERNAL_SERVER_ERROR.getStatusCode(),
resp.getStatus());
+ Assertions.assertEquals(MediaType.APPLICATION_JSON_TYPE,
resp.getMediaType());
+
+ Mockito.reset(tableOps);
+ when(tableOps.alterTableAlterColumns(any(), any(), any()))
+ .thenThrow(
+ LanceNamespaceException.notFound(
+ "Table not found", "NoSuchTableException", tableIds, ""));
+ resp =
+ target(String.format("/v1/table/%s/alter_columns", tableIds))
+ .queryParam("delimiter", delimiter)
+ .request(MediaType.APPLICATION_JSON_TYPE)
+ .post(Entity.entity(alterColumnsRequest,
MediaType.APPLICATION_JSON_TYPE));
+ Assertions.assertEquals(Response.Status.NOT_FOUND.getStatusCode(),
resp.getStatus());
+ }
Review Comment:
The test coverage is missing validation scenarios for the alter_columns
endpoint. Consider adding test cases for:
1. Empty alterations list (should trigger the validation and return
BAD_REQUEST)
2. Alteration with castTo set (should trigger the validation and return
BAD_REQUEST since only RENAME is supported)
Similar patterns exist in other tests like testCreateTable (line 372-373)
and testRegisterTable (line 487-488) where IllegalArgumentException handling is
tested.
##########
lance/lance-rest-server/src/main/java/org/apache/gravitino/lance/service/rest/LanceTableOperations.java:
##########
@@ -272,4 +322,20 @@ private void
validateDropTableRequest(@SuppressWarnings("unused") DropTableReque
// We will ignore the id in the request body since it's already provided
in the path param
// No specific fields to validate for now
}
+
+ private void validateDropColumnsRequest(AlterTableDropColumnsRequest
request) {
+ Preconditions.checkArgument(
+ !request.getColumns().isEmpty(), "Columns to drop cannot be empty.");
+ }
+
+ private void validateAlterColumnsRequest(AlterTableAlterColumnsRequest
request) {
+ Preconditions.checkArgument(
+ !request.getAlterations().isEmpty(), "Columns to alter cannot be
empty.");
+
+ for (ColumnAlteration alteration : request.getAlterations()) {
+ Preconditions.checkArgument(
+ StringUtils.isBlank(alteration.getCastTo()),
+ "Only RENAME alteration is supported currently.");
Review Comment:
The validation only checks that castTo is blank (since only RENAME is
supported) but doesn't verify that the rename field is actually populated. This
means a request with an alteration that has both blank rename and blank castTo
would pass validation but result in no actual changes, triggering the "No valid
alterations found" error at GravitinoLanceTableOperations.java:362.
Consider adding validation to ensure that when castTo is blank, the rename
field must be non-blank. For example:
```
Preconditions.checkArgument(
StringUtils.isNotBlank(alteration.getRename()),
"Rename field must be specified when castTo is not provided.");
```
```suggestion
"Only RENAME alteration is supported currently.");
Preconditions.checkArgument(
StringUtils.isNotBlank(alteration.getRename()),
"Rename field must be specified when castTo is not provided.");
```
--
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]
