Copilot commented on code in PR #9513:
URL: https://github.com/apache/gravitino/pull/9513#discussion_r2631005191
##########
lance/lance-rest-server/src/test/java/org/apache/gravitino/lance/integration/test/LanceRESTServiceIT.java:
##########
@@ -672,6 +672,15 @@ void testRegisterTable() {
Assertions.assertNotNull(response);
Assertions.assertEquals(nonExistingLocation, response.getLocation());
Assertions.assertFalse(new File(nonExistingLocation).exists());
+
+ // Test create again after deregister should fail
Review Comment:
The comment is misleading and does not accurately describe what the test is
doing. It says "Test create again after deregister should fail" but the test
actually verifies that registerTable works correctly when mode is null (it
should not fail). Update the comment to accurately reflect the test purpose.
```suggestion
// Test registerTable with null mode after deregister should succeed
```
##########
lance/lance-rest-server/src/test/java/org/apache/gravitino/lance/integration/test/LanceRESTServiceIT.java:
##########
@@ -672,6 +672,15 @@ void testRegisterTable() {
Assertions.assertNotNull(response);
Assertions.assertEquals(nonExistingLocation, response.getLocation());
Assertions.assertFalse(new File(nonExistingLocation).exists());
+
+ // Test create again after deregister should fail
+ registerTableRequest.setMode(null);
+ String verifyNullModeLocation = tempDir + "/" +
"verify_null_mode_location/";
+ registerTableRequest.setLocation(verifyNullModeLocation);
+ registerTableRequest.setId(List.of(CATALOG_NAME, SCHEMA_NAME,
"verify_null_mode_location"));
+ response = Assertions.assertDoesNotThrow(() ->
ns.registerTable(registerTableRequest));
+ Assertions.assertNotNull(response);
+ Assertions.assertEquals(verifyNullModeLocation, response.getLocation());
Review Comment:
The test should verify the actual behavior when mode is null. Currently, it
only checks that no exception is thrown and the response is not null. Consider
adding an assertion to verify that the table was registered with the expected
default mode behavior (e.g., verify that registering with null mode behaves
like CREATE or REGISTER mode based on the fix).
##########
lance/lance-rest-server/src/main/java/org/apache/gravitino/lance/service/rest/LanceTableOperations.java:
##########
@@ -167,7 +167,8 @@ public Response registerTable(
: Maps.newHashMap(registerTableRequest.getProperties());
props.put(LANCE_LOCATION, registerTableRequest.getLocation());
props.put(LanceConstants.LANCE_TABLE_REGISTER, "true");
- ModeEnum mode = registerTableRequest.getMode();
+ ModeEnum mode =
+ registerTableRequest.getMode() == null ? ModeEnum.CREATE :
registerTableRequest.getMode();
Review Comment:
The default value for mode should not be CREATE. According to the API
documentation in LanceTableOperations interface (line 79), the mode for
registerTable can only be REGISTER or OVERWRITE, not CREATE. Using CREATE as a
default may cause unexpected behavior or errors. Consider using
ModeEnum.REGISTER as the default value instead, or validate that the mode is
not null and return an appropriate error if it is.
```suggestion
registerTableRequest.getMode() == null ? ModeEnum.REGISTER :
registerTableRequest.getMode();
```
--
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]
