Copilot commented on code in PR #9589:
URL: https://github.com/apache/gravitino/pull/9589#discussion_r2661354488
##########
lance/lance-rest-server/src/main/java/org/apache/gravitino/lance/service/rest/LanceTableOperations.java:
##########
@@ -141,6 +146,7 @@ public Response createEmptyTable(
? Maps.newHashMap()
: Maps.newHashMap(request.getProperties());
+ props.put(LANCE_TABLE_CREATE_EMTPY, "true");
Review Comment:
The constant name contains a typo: "EMTPY" should be "EMPTY". This should
match the corrected spelling in the constant definition.
##########
lance/lance-rest-server/src/main/java/org/apache/gravitino/lance/service/rest/LanceTableOperations.java:
##########
@@ -141,6 +146,7 @@ public Response createEmptyTable(
? Maps.newHashMap()
: Maps.newHashMap(request.getProperties());
+ props.put(LANCE_TABLE_CREATE_EMTPY, "true");
Review Comment:
The internal property `LANCE_TABLE_CREATE_EMTPY` (note the typo - should be
EMPTY) is being added to the table properties but is not declared in the
`LanceTableDelegator.tablePropertyEntries()` method. Similar internal
properties like `LANCE_TABLE_REGISTER` are properly declared as hidden or
reserved property entries. This property should either be: 1) Declared in
tablePropertyEntries() as a hidden/reserved property, or 2) Filtered out before
persisting to table metadata to prevent internal implementation details from
leaking to users.
##########
lance/lance-rest-server/src/main/java/org/apache/gravitino/lance/service/rest/LanceTableOperations.java:
##########
@@ -20,6 +20,7 @@
import static
org.apache.gravitino.lance.common.ops.NamespaceWrapper.NAMESPACE_DELIMITER_DEFAULT;
import static
org.apache.gravitino.lance.common.utils.LanceConstants.LANCE_LOCATION;
+import static
org.apache.gravitino.lance.common.utils.LanceConstants.LANCE_TABLE_CREATE_EMTPY;
Review Comment:
The constant name contains a typo: "EMTPY" should be "EMPTY". This should
match the corrected spelling in the constant definition.
```suggestion
import static
org.apache.gravitino.lance.common.utils.LanceConstants.LANCE_TABLE_CREATE_EMPTY;
```
##########
lance/lance-rest-server/src/main/java/org/apache/gravitino/lance/service/rest/LanceTableOperations.java:
##########
@@ -122,6 +123,10 @@ public Response createTable(
}
}
+ /**
+ * According to the latest spec, `createEmptyTable` only store the metadata
and store the location
+ * of lance table, and will never touch lance storage.
Review Comment:
The comment has a grammatical issue with repeated "store": "only store the
metadata and store the location". Consider revising to: "only stores the
metadata and location of the lance table, and will never touch lance storage"
or "only stores the table metadata including its location, and will never touch
lance storage".
```suggestion
* According to the latest spec, {@code createEmptyTable} only stores the
table metadata,
* including its location, and will never touch lance storage.
```
##########
lance/lance-common/src/main/java/org/apache/gravitino/lance/common/ops/gravitino/GravitinoLanceTableOperations.java:
##########
@@ -154,6 +154,7 @@ public CreateTableResponse createTable(
@Override
public CreateEmptyTableResponse createEmptyTable(
String tableId, String delimiter, String tableLocation, Map<String,
String> tableProperties) {
+ // Create tables only support mode `create`.
Review Comment:
The comment "Create tables only support mode `create`." is unclear and
potentially misleading. It's not obvious whether this refers to empty tables
specifically or all table creations. Consider clarifying this to: "Empty table
creation only supports CREATE mode (not EXIST_OK or OVERWRITE)" if that's the
intended meaning, or updating it to better explain why this constraint exists.
```suggestion
// Empty table creation only supports CREATE mode (not EXIST_OK or
OVERWRITE).
```
##########
catalogs/catalog-lakehouse-generic/src/main/java/org/apache/gravitino/catalog/lakehouse/lance/LanceTableOperations.java:
##########
@@ -263,6 +264,18 @@ Table createTableInternal(
ident, columns, comment, properties, partitions, distribution,
sortOrders, indexes);
}
+ // Check whether it's a create empty table operation.
+ boolean createEmpty =
+
Optional.ofNullable(properties.get(LanceConstants.LANCE_TABLE_CREATE_EMTPY))
Review Comment:
The constant name contains a typo: "EMTPY" should be "EMPTY". This should
match the corrected spelling in the constant definition.
```suggestion
Optional.ofNullable(properties.get(LanceConstants.LANCE_TABLE_CREATE_EMPTY))
```
##########
lance/lance-rest-server/src/test/java/org/apache/gravitino/lance/integration/test/LanceRESTServiceIT.java:
##########
@@ -446,19 +445,20 @@ void testCreateEmptyTable() throws ApiException {
// Try to create a table with wrong location should fail
Review Comment:
The comment "Try to create a table with wrong location should fail" is now
misleading. The test was updated to verify that creating an empty table with a
non-existent location succeeds (which is correct for the new empty table
behavior), but the comment still suggests this should fail. The comment should
be updated to reflect the new expected behavior, such as "Create an empty table
with non-existent location should succeed since storage is not touched".
```suggestion
// Create an empty table with non-existent location should succeed since
storage is not touched
```
##########
lance/lance-rest-server/src/test/java/org/apache/gravitino/lance/integration/test/LanceRESTServiceIT.java:
##########
@@ -789,12 +789,7 @@ void testDropTable() {
// Drop the table
DropTableRequest dropTableRequest = new DropTableRequest();
dropTableRequest.setId(ids);
- DropTableResponse dropTableResponse =
- Assertions.assertDoesNotThrow(() -> ns.dropTable(dropTableRequest));
- Assertions.assertNotNull(dropTableResponse);
- Assertions.assertEquals(location, dropTableResponse.getLocation());
- Assertions.assertFalse(
- new File(location).exists(), "Data should be deleted after dropping
the table.");
+ Assertions.assertThrows(Exception.class, () ->
ns.dropTable(dropTableRequest));
Review Comment:
The test now expects `dropTable` to throw a generic `Exception.class`
instead of validating the specific response. This is too broad and reduces test
precision. If the new behavior is that dropping an empty table (metadata-only)
should fail, the test should verify the specific exception type and error code.
Additionally, the comment at line 794 "Describe the dropped table should fail"
is now misleading since the table was never actually dropped due to the
exception.
##########
lance/lance-rest-server/src/test/java/org/apache/gravitino/lance/integration/test/LanceRESTServiceIT.java:
##########
@@ -789,12 +789,7 @@ void testDropTable() {
// Drop the table
DropTableRequest dropTableRequest = new DropTableRequest();
dropTableRequest.setId(ids);
- DropTableResponse dropTableResponse =
- Assertions.assertDoesNotThrow(() -> ns.dropTable(dropTableRequest));
- Assertions.assertNotNull(dropTableResponse);
- Assertions.assertEquals(location, dropTableResponse.getLocation());
- Assertions.assertFalse(
- new File(location).exists(), "Data should be deleted after dropping
the table.");
+ Assertions.assertThrows(Exception.class, () ->
ns.dropTable(dropTableRequest));
Review Comment:
After the dropTable call is expected to throw an exception, the test
continues to verify "Describe the dropped table should fail" (line 794) and
"Drop a non-existing table should fail" (line 802). However, since the first
dropTable call at line 792 is expected to throw an exception, the table was
never actually dropped. The subsequent assertions at lines 794-800 and 802-807
are testing the behavior of attempting to describe/drop a table that still
exists (but is an empty table), not a dropped table. The test logic and
comments should be updated to reflect the actual behavior being tested.
```suggestion
Assertions.assertDoesNotThrow(() -> ns.dropTable(dropTableRequest));
```
##########
lance/lance-common/src/main/java/org/apache/gravitino/lance/common/utils/LanceConstants.java:
##########
@@ -37,5 +37,7 @@ public class LanceConstants {
public static final String LANCE_TABLE_REGISTER = "lance.register";
+ public static final String LANCE_TABLE_CREATE_EMTPY = "lance.create-empty";
+
Review Comment:
The constant name contains a typo: "EMTPY" should be "EMPTY". This
misspelling appears in the constant definition and is used throughout the
codebase.
```suggestion
public static final String LANCE_TABLE_CREATE_EMPTY = "lance.create-empty";
/**
* @deprecated Use {@link #LANCE_TABLE_CREATE_EMPTY} instead.
*/
@Deprecated
public static final String LANCE_TABLE_CREATE_EMTPY =
LANCE_TABLE_CREATE_EMPTY;
```
##########
lance/lance-rest-server/src/test/java/org/apache/gravitino/lance/integration/test/LanceRESTServiceIT.java:
##########
@@ -446,19 +445,20 @@ void testCreateEmptyTable() throws ApiException {
// Try to create a table with wrong location should fail
CreateEmptyTableRequest wrongLocationRequest = new
CreateEmptyTableRequest();
- wrongLocationRequest.setId(List.of(CATALOG_NAME, SCHEMA_NAME,
"wrong_location_table"));
- wrongLocationRequest.setLocation("hdfs://localhost:9000/invalid_path/");
- LanceNamespaceException apiException =
- Assertions.assertThrows(
- LanceNamespaceException.class,
- () -> {
- ns.createEmptyTable(wrongLocationRequest);
- });
- Assertions.assertTrue(apiException.getMessage().contains("Invalid user
input"));
+ wrongLocationRequest.setId(List.of(CATALOG_NAME, SCHEMA_NAME,
"another_table"));
+ String another_location = tempDir + "/" + "another_location/";
+ Assertions.assertFalse(new File(another_location).exists());
+ wrongLocationRequest.setLocation(another_location);
+ response = ns.createEmptyTable(wrongLocationRequest);
+ Assertions.assertNotNull(response);
+ Assertions.assertEquals(another_location, response.getLocation());
+ // Will not touch storage, so the path should not be created.
+ Assertions.assertFalse(new File(another_location).exists());
// Correct the location and try again
Review Comment:
The comment "Correct the location and try again" is misleading. The previous
test at lines 448-456 already demonstrated that creating an empty table with a
non-existent location succeeds (as expected for the new behavior). This comment
suggests the location needed correction, but that's no longer applicable.
Consider updating the comment to clarify what this test case is actually
verifying, or remove it if it's now redundant.
```suggestion
// Create another empty table at a new location and verify it succeeds
```
--
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]
