Copilot commented on code in PR #9659:
URL: https://github.com/apache/gravitino/pull/9659#discussion_r2675213860
##########
catalogs/catalog-lakehouse-generic/src/test/java/org/apache/gravitino/catalog/lakehouse/lance/integration/test/CatalogGenericCatalogLanceIT.java:
##########
@@ -898,43 +906,75 @@ public void
testRegisterTableWithCreateModeFailsWhenExists() throws IOException
Transforms.EMPTY_TRANSFORM,
Distributions.NONE,
new SortOrder[0]));
- }
- @Test
- void testRegisterWithNonExistLocation() {
- // Now try to register a table with non-existing location with CREATE mode
- should succeed
+ // Test register table without specifying the location
String newTableName = GravitinoITUtils.genRandomName(TABLE_PREFIX);
NameIdentifier newTableIdentifier = NameIdentifier.of(schemaName,
newTableName);
Map<String, String> newCreateProperties = createProperties();
- String newLocation = String.format("%s/%s/%s/", tempDirectory, schemaName,
newTableName);
- boolean dirExists = new File(newLocation).exists();
- Assertions.assertFalse(dirExists);
-
- newCreateProperties.put(Table.PROPERTY_LOCATION, newLocation);
newCreateProperties.put(Table.PROPERTY_TABLE_FORMAT, LANCE_TABLE_FORMAT);
newCreateProperties.put(Table.PROPERTY_EXTERNAL, "true");
newCreateProperties.put(LANCE_TABLE_REGISTER, "true");
- Table nonExistingTable =
+ String finalNewLocation = String.format("%s/%s/%s/", tempDirectory,
schemaName, newTableName);
+
+ try (RootAllocator allocator = new RootAllocator();
+ Dataset dataset =
+ Dataset.create(
+ allocator, finalNewLocation, arrowSchema, new
WriteParams.Builder().build())) {
+ // Dataset created
+ }
+
+ newCreateProperties.put(Table.PROPERTY_LOCATION, finalNewLocation);
+ Table registerTableWithoutColumn =
catalog
.asTableCatalog()
.createTable(
newTableIdentifier,
new Column[0],
- "Updated comment",
- newCreateProperties,
+ TABLE_COMMENT,
+ properties,
Review Comment:
The wrong properties map is being used here. Should use
`newCreateProperties` instead of `properties` since the test is setting up
properties in `newCreateProperties` (lines 913-927).
```suggestion
newCreateProperties,
```
##########
catalogs/catalog-lakehouse-generic/src/main/java/org/apache/gravitino/catalog/lakehouse/lance/LanceTableOperations.java:
##########
@@ -258,12 +261,33 @@ Table createTableInternal(
String location)
throws NoSuchSchemaException, TableAlreadyExistsException {
+ Map<String, String> storageProps =
LancePropertiesUtils.getLanceStorageOptions(properties);
if (register) {
+ // Try to check if the Lance dataset exists at the location and then
extract the schema.
+ // Note: We will ignore the columns parameter in this case.
+ LOG.warn(
+ "Registering existing Lance table at location {}. The provided
schema will be ignored. columns: {}",
+ location,
+ Arrays.toString(columns));
Review Comment:
Consider providing more specific information in the warning message. Instead
of just logging the columns as a string array, consider logging only the number
of columns and whether they were provided, since the provided schema is being
ignored anyway. This would make the log message cleaner and more actionable.
```suggestion
boolean columnsProvided = columns != null && columns.length > 0;
int numColumns = columns == null ? 0 : columns.length;
LOG.warn(
"Registering existing Lance table at location {}. The provided
schema will be ignored. "
+ "columnsProvided: {}, numberOfColumns: {}",
location,
columnsProvided,
numColumns);
```
##########
lance/lance-rest-server/src/test/java/org/apache/gravitino/lance/integration/test/LanceRESTServiceIT.java:
##########
@@ -664,25 +675,43 @@ void testRegisterTable() {
Assertions.assertEquals(newLocation,
deregisterTableResponse.getLocation());
// Test Overwrite again after deregister
+ // We need a valid location for overwrite
String nonExistingLocation = tempDir + "/" + "non_existing_location/";
+ createLanceTable(nonExistingLocation);
+
registerTableRequest.setMode(ModeEnum.OVERWRITE);
registerTableRequest.setId(ids);
registerTableRequest.setLocation(nonExistingLocation);
response = Assertions.assertDoesNotThrow(() ->
ns.registerTable(registerTableRequest));
Assertions.assertNotNull(response);
Assertions.assertEquals(nonExistingLocation, response.getLocation());
- Assertions.assertFalse(new File(nonExistingLocation).exists());
+ Assertions.assertTrue(new File(nonExistingLocation).exists());
// Test registerTable with null mode after deregister should succeed
registerTableRequest.setMode(null);
String verifyNullModeLocation = tempDir + "/" +
"verify_null_mode_location/";
+ createLanceTable(verifyNullModeLocation);
+
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());
}
+ private void createLanceTable(String location) {
+ org.apache.arrow.vector.types.pojo.Schema schema =
+ new org.apache.arrow.vector.types.pojo.Schema(
+ Arrays.asList(
+ Field.nullable("id", new ArrowType.Int(32, true)),
+ Field.nullable("value", new ArrowType.Utf8())));
+ try (Dataset dataset =
+ Dataset.create(allocator, location, schema, new
WriteParams.Builder().build())) {
+ } catch (Exception e) {
+ throw new RuntimeException(e);
Review Comment:
The error message could be more descriptive. Consider including the
exception message or type in the RuntimeException to provide better context
about what went wrong during dataset creation.
```suggestion
throw new RuntimeException(
"Failed to create 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]
