Copilot commented on code in PR #9450:
URL: https://github.com/apache/gravitino/pull/9450#discussion_r2606588967
##########
catalogs/catalog-lakehouse-generic/src/test/java/org/apache/gravitino/catalog/lakehouse/lance/TestLanceTableOperations.java:
##########
@@ -89,4 +93,22 @@ public void testCreateTableWithInvalidMode() {
new SortOrder[0],
new Index[0]));
}
+
+ @ParameterizedTest
+ @MethodSource("pathProvider")
+ void testRegisterWithInvalidLocation(String location, boolean isValid) {
+ Assertions.assertEquals(isValid,
lanceTableOps.isValidLanceLocation(location));
+ }
+
+ static Stream<Arguments> pathProvider() {
+ return Stream.of(
+ Arguments.of("/data/lance/table1", true),
+ Arguments.of("hdfs://namenode:8020/data/lance/table2", true),
+ Arguments.of("s3a://bucket/data/lance/table3", true),
+ Arguments.of("file:///data/lance/table4", true),
+ Arguments.of("ftp://server/data/lance/table5";, true),
+ Arguments.of("invalid/path", false),
+ Arguments.of("", false),
+ Arguments.of(null, false));
Review Comment:
The test cases don't include Windows-style absolute paths (e.g.,
`C:/data/lance/table` or `C:\\data\\lance\\table`). While the implementation
should handle these correctly via `File.isAbsolute()`, adding test cases for
Windows paths would improve cross-platform test coverage. Consider adding:
- `Arguments.of("C:/data/lance/table", true)` for forward slash
- `Arguments.of("C:\\\\data\\\\lance\\\\table", true)` for backslash (note:
double escaping in Java strings)
##########
catalogs/catalog-lakehouse-generic/src/test/java/org/apache/gravitino/catalog/lakehouse/lance/TestLanceTableOperations.java:
##########
@@ -89,4 +93,22 @@ public void testCreateTableWithInvalidMode() {
new SortOrder[0],
new Index[0]));
}
+
+ @ParameterizedTest
+ @MethodSource("pathProvider")
+ void testRegisterWithInvalidLocation(String location, boolean isValid) {
+ Assertions.assertEquals(isValid,
lanceTableOps.isValidLanceLocation(location));
+ }
+
+ static Stream<Arguments> pathProvider() {
+ return Stream.of(
+ Arguments.of("/data/lance/table1", true),
+ Arguments.of("hdfs://namenode:8020/data/lance/table2", true),
+ Arguments.of("s3a://bucket/data/lance/table3", true),
+ Arguments.of("file:///data/lance/table4", true),
+ Arguments.of("ftp://server/data/lance/table5";, true),
+ Arguments.of("invalid/path", false),
+ Arguments.of("", false),
+ Arguments.of(null, false));
Review Comment:
[nitpick] Consider adding test cases for edge cases such as:
- Relative paths with special characters (e.g., `"./path"`, `"../path"`)
- Paths with only whitespace (e.g., `" "`)
- URI with scheme but invalid syntax (e.g., `"http://invalid path with
spaces"`)
These would help ensure the validation is robust against various malformed
inputs.
##########
catalogs/catalog-lakehouse-generic/src/test/java/org/apache/gravitino/catalog/lakehouse/lance/TestLanceTableOperations.java:
##########
@@ -89,4 +93,22 @@ public void testCreateTableWithInvalidMode() {
new SortOrder[0],
new Index[0]));
}
+
+ @ParameterizedTest
+ @MethodSource("pathProvider")
+ void testRegisterWithInvalidLocation(String location, boolean isValid) {
Review Comment:
The test method name `testRegisterWithInvalidLocation` is misleading because
it tests both valid and invalid locations. Consider renaming it to
`testLocationValidation` or `testIsValidLanceLocation` to better reflect what
the test is actually verifying.
```suggestion
void testLocationValidation(String location, boolean isValid) {
```
##########
catalogs/catalog-lakehouse-generic/src/main/java/org/apache/gravitino/catalog/lakehouse/lance/LanceTableOperations.java:
##########
@@ -107,9 +111,9 @@ public Table createTable(
Index[] indexes)
throws NoSuchSchemaException, TableAlreadyExistsException {
String location = properties.get(Table.PROPERTY_LOCATION);
- Preconditions.checkArgument(
- StringUtils.isNotBlank(location), "Table location must be specified");
+ Preconditions.checkArgument(
+ isValidLanceLocation(location), String.format("Table location is
invalid:'%s'", location));
Review Comment:
There's a spacing issue in the error message format. There should be a space
after "invalid:" for better readability. The format should be `"Table location
is invalid: '%s'"` instead of `"Table location is invalid:'%s'"`.
```suggestion
isValidLanceLocation(location), String.format("Table location is
invalid: '%s'", location));
```
--
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]
