[#9113] feat(lance-rest): Support drop and rename column for Lance table#9127
[#9113] feat(lance-rest): Support drop and rename column for Lance table#9127yuqi1129 wants to merge 15 commits intoapache:mainfrom
Conversation
|
There are some things we need to dicuss before doing the add column API as the parameter differs from that in Gravitino, and more important, the transition betwwen them are not so easy. |
There was a problem hiding this comment.
Pull request overview
This PR adds support for dropping columns and altering columns (rename) in Lance tables, addressing issue #9113. The implementation provides both REST API endpoints and underlying catalog operations.
Key Changes
- Added REST endpoints
/drop_columnsand/alter_columnsfor table column operations - Implemented drop columns and alter columns functionality across REST server, Gravitino operations, and catalog layers
- Comprehensive test coverage with both unit tests and integration tests
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
lance/lance-common/src/main/java/org/apache/gravitino/lance/common/ops/LanceTableOperations.java |
Added interface methods for drop columns, alter columns, and add columns operations |
lance/lance-common/src/main/java/org/apache/gravitino/lance/common/ops/gravitino/GravitinoLanceTableOperations.java |
Implemented drop/alter column operations by translating Lance requests to Gravitino TableChange operations |
lance/lance-rest-server/src/main/java/org/apache/gravitino/lance/service/rest/LanceTableOperations.java |
Added REST endpoints for drop_columns and alter_columns with input validation |
catalogs/catalog-lakehouse-generic/src/main/java/org/apache/gravitino/catalog/lakehouse/lance/LanceTableOperations.java |
Refactored alterTable to handle drop columns and rename columns alongside existing add index support |
lance/lance-rest-server/src/test/java/org/apache/gravitino/lance/service/rest/TestLanceNamespaceOperations.java |
Added comprehensive unit tests for drop columns and alter columns operations including error scenarios |
lance/lance-rest-server/src/test/java/org/apache/gravitino/lance/integration/test/LanceRESTServiceIT.java |
Added integration tests validating drop columns and alter columns functionality end-to-end |
...main/java/org/apache/gravitino/lance/common/ops/gravitino/GravitinoLanceTableOperations.java
Outdated
Show resolved
Hide resolved
...-rest-server/src/main/java/org/apache/gravitino/lance/service/rest/LanceTableOperations.java
Outdated
Show resolved
Hide resolved
...generic/src/main/java/org/apache/gravitino/catalog/lakehouse/lance/LanceTableOperations.java
Outdated
Show resolved
Hide resolved
...est-server/src/test/java/org/apache/gravitino/lance/integration/test/LanceRESTServiceIT.java
Outdated
Show resolved
Hide resolved
...generic/src/main/java/org/apache/gravitino/catalog/lakehouse/lance/LanceTableOperations.java
Outdated
Show resolved
Hide resolved
...-rest-server/src/main/java/org/apache/gravitino/lance/service/rest/LanceTableOperations.java
Show resolved
Hide resolved
...-rest-server/src/main/java/org/apache/gravitino/lance/service/rest/LanceTableOperations.java
Show resolved
Hide resolved
...generic/src/main/java/org/apache/gravitino/catalog/lakehouse/lance/LanceTableOperations.java
Outdated
Show resolved
Hide resolved
...est-server/src/test/java/org/apache/gravitino/lance/integration/test/LanceRESTServiceIT.java
Show resolved
Hide resolved
...est-server/src/test/java/org/apache/gravitino/lance/integration/test/LanceRESTServiceIT.java
Show resolved
Hide resolved
...main/java/org/apache/gravitino/lance/common/ops/gravitino/GravitinoLanceTableOperations.java
Show resolved
Hide resolved
...generic/src/main/java/org/apache/gravitino/catalog/lakehouse/lance/LanceTableOperations.java
Outdated
Show resolved
Hide resolved
...generic/src/main/java/org/apache/gravitino/catalog/lakehouse/lance/LanceTableOperations.java
Show resolved
Hide resolved
...generic/src/main/java/org/apache/gravitino/catalog/lakehouse/lance/LanceTableOperations.java
Outdated
Show resolved
Hide resolved
...main/java/org/apache/gravitino/lance/common/ops/gravitino/GravitinoLanceTableOperations.java
Outdated
Show resolved
Hide resolved
|
@mchades can you please help to review? |
...generic/src/main/java/org/apache/gravitino/catalog/lakehouse/lance/LanceTableOperations.java
Outdated
Show resolved
Hide resolved
...generic/src/main/java/org/apache/gravitino/catalog/lakehouse/lance/LanceTableOperations.java
Outdated
Show resolved
Hide resolved
...main/java/org/apache/gravitino/lance/common/ops/gravitino/GravitinoLanceTableOperations.java
Outdated
Show resolved
Hide resolved
...main/java/org/apache/gravitino/lance/common/ops/gravitino/GravitinoLanceTableOperations.java
Outdated
Show resolved
Hide resolved
...main/java/org/apache/gravitino/lance/common/ops/gravitino/GravitinoLanceTableOperations.java
Outdated
Show resolved
Hide resolved
...main/java/org/apache/gravitino/lance/common/ops/gravitino/GravitinoLanceTableOperations.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/apache/gravitino/connector/GenericTable.java
Outdated
Show resolved
Hide resolved
| // More please see: https://docs.lancedb.com/api-reference/data/add-columns | ||
| throw new UnsupportedOperationException("Adding columns is not supported yet."); | ||
| } | ||
|
|
There was a problem hiding this comment.
The implementation of these 3 methods are basically the same; can they be merged together?
There was a problem hiding this comment.
Do you mean we can define the following interface to replace them all?
@Override
public AlterTableAlterColumnsResponse alterTableOperations(
String tableId, String delimiter, Object request) {
if (request instanceof AlterTableAddColumnsRequest addColumnsRequest) {
// add column
} else if (request instanceof AlterTableAlterColumnsRequest alterColumnsRequest) {
return alterTableAlterColumns(tableId, delimiter, alterColumnsRequest);
} else if (request instanceof AlterTableDropColumnsRequest dropColumnsRequest) {
// drop column
return alterTableDropColumns(tableId, delimiter, dropColumnsRequest);
}
}There was a problem hiding this comment.
Yes, something like this. Since most of the logic are similar, if we continue to add more alter-related operations, there will be lots of duplicated codes.
What changes were proposed in this pull request?
Support drop columns for the Lance table
Why are the changes needed?
It's a common need for Lance tables.
Fix: #9113
Does this PR introduce any user-facing change?
N/A
How was this patch tested?
UTs and ITs