Copilot commented on code in PR #9127:
URL: https://github.com/apache/gravitino/pull/9127#discussion_r2668201509
##########
lance/lance-rest-server/src/main/java/org/apache/gravitino/lance/service/rest/LanceTableOperations.java:
##########
@@ -268,4 +316,15 @@ 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) {
+ request.getAlterations();
Review Comment:
The statement 'request.getAlterations();' on line 326 has no effect and
appears to be leftover code. This line should be removed as it doesn't perform
any validation or have any side effects.
```suggestion
```
##########
lance/lance-common/src/main/java/org/apache/gravitino/lance/common/ops/gravitino/GravitinoLanceTableOperations.java:
##########
@@ -309,4 +373,28 @@ private JsonArrowSchema toJsonArrowSchema(Column[]
columns) {
return JsonArrowSchemaConverter.convertToJsonArrowSchema(
new org.apache.arrow.vector.types.pojo.Schema(fields));
}
+
+ private List<TableChange>
buildAlterColumnChanges(AlterTableAlterColumnsRequest request) {
+ List<ColumnAlteration> columns = request.getAlterations();
+
+ List<TableChange> changes = new ArrayList<>();
+ for (ColumnAlteration column : columns) {
+ // Column name will not be null according to LanceDB spec.
+ String columName = column.getColumn();
Review Comment:
Variable name 'columName' contains a typo. It should be 'columnName' for
consistency with Java naming conventions.
##########
lance/lance-rest-server/src/test/java/org/apache/gravitino/lance/integration/test/LanceRESTServiceIT.java:
##########
@@ -591,6 +597,102 @@ 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();
+ List.of(ids);
Review Comment:
Line 603 creates a list with 'List.of(ids)' but the result is not assigned
or used. This appears to be leftover debugging code that should be removed.
```suggestion
```
##########
catalogs/catalog-lakehouse-generic/src/main/java/org/apache/gravitino/catalog/lakehouse/lance/LanceTableOperations.java:
##########
@@ -303,11 +287,37 @@ private org.apache.arrow.vector.types.pojo.Schema
convertColumnsToArrowSchema(Co
return new org.apache.arrow.vector.types.pojo.Schema(fields);
}
- private void addLanceIndex(Table table, List<Index> addedIndexes) {
+ private void handleLanceTableChange(Table table, TableChange[] changes) {
+ List<String> dropColumns = Lists.newArrayList();
+ List<Index> indexToAdd = Lists.newArrayList();
+ List<ColumnAlteration> renameColumns = Lists.newArrayList();
+
+ for (TableChange change : changes) {
+ if (change instanceof TableChange.DeleteColumn deleteColumn) {
+ dropColumns.add(deleteColumn.fieldName()[0]);
+ } else if (change instanceof TableChange.AddIndex addIndex) {
+ indexToAdd.add(
+ Indexes.IndexImpl.builder()
+ .withIndexType(addIndex.getType())
+ .withName(addIndex.getName())
+ .withFieldNames(addIndex.getFieldNames())
+ .build());
+ } else if (change instanceof TableChange.RenameColumn renameColumn) {
+ // Currently, only renaming columns is supported.
+ ColumnAlteration lanceColumnAlter =
+ new ColumnAlteration.Builder(renameColumn.fieldName()[0])
+ .rename(renameColumn.getNewName())
+ .build();
+ renameColumns.add(lanceColumnAlter);
+ } else {
+ throw new UnsupportedOperationException(
+ "LanceTableOperations only supports adding indexes currently.");
Review Comment:
The error message at line 314 states "LanceTableOperations only supports
adding indexes currently", but this is now outdated since the code now also
supports dropping columns and renaming columns. The error message should be
updated to reflect the supported operations.
##########
catalogs/catalog-lakehouse-generic/src/main/java/org/apache/gravitino/catalog/lakehouse/lance/LanceTableOperations.java:
##########
@@ -320,6 +330,14 @@ private void addLanceIndex(Table table, List<Index>
addedIndexes) {
indexParams,
true);
}
+
+ if (!dropColumns.isEmpty()) {
+ dataset.dropColumns(dropColumns);
+ }
+
+ if (!renameColumns.isEmpty()) {
+ dataset.alterColumns(renameColumns);
+ }
} catch (Exception e) {
throw new RuntimeException(
"Failed to add indexes to Lance dataset at location " + location, e);
Review Comment:
The error message at line 343 mentions "Failed to add indexes" which is
misleading since this method now handles dropping columns, renaming columns,
and adding indexes. The error message should be updated to reflect all the
operations being performed.
```suggestion
"Failed to apply column and index changes (adding indexes,
dropping columns, renaming columns) to Lance dataset at location "
+ location,
e);
```
--
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]
