Copilot commented on code in PR #9319:
URL: https://github.com/apache/gravitino/pull/9319#discussion_r2583363969
##########
flink-connector/flink/src/test/java/org/apache/gravitino/flink/connector/catalog/TestBaseCatalog.java:
##########
@@ -116,4 +121,35 @@ public void testTableChangesWithoutColumnChange() {
ImmutableList.of(org.apache.gravitino.rel.TableChange.updateComment("new
comment"));
Assertions.assertArrayEquals(expected.toArray(), tableChanges);
}
+
+ @Test
+ public void testListViewsReturnsEmptyWithoutDelegation() throws Exception {
+ AbstractCatalog delegate = Mockito.mock(AbstractCatalog.class);
+ BaseCatalog catalog = new TestableBaseCatalog(delegate);
+
+ List<String> views = catalog.listViews("db");
+
+ Assertions.assertTrue(views.isEmpty());
+ Mockito.verifyNoInteractions(delegate);
+ }
Review Comment:
The test `testListViewsReturnsEmptyWithoutDelegation` only verifies that an
empty list is returned, but doesn't test the important edge case where the
database doesn't exist. According to the Flink Catalog API contract,
`listViews()` should throw `DatabaseNotExistException` when called with a
non-existent database name.
Consider adding a test case to verify this behavior:
```java
@Test
public void testListViewsThrowsExceptionForNonExistentDatabase() {
// Test that DatabaseNotExistException is thrown for non-existent database
BaseCatalog catalog = new
TestableBaseCatalog(Mockito.mock(AbstractCatalog.class));
Assertions.assertThrows(
DatabaseNotExistException.class,
() -> catalog.listViews("non_existent_db"));
}
```
##########
flink-connector/flink/src/main/java/org/apache/gravitino/flink/connector/catalog/BaseCatalog.java:
##########
@@ -202,8 +203,10 @@ public List<String> listTables(String databaseName)
}
@Override
- public List<String> listViews(String s) throws DatabaseNotExistException,
CatalogException {
- throw new UnsupportedOperationException();
+ public List<String> listViews(String databaseName)
+ throws DatabaseNotExistException, CatalogException {
Review Comment:
The `listViews()` method should validate that the database exists before
returning an empty list, similar to how `listTables()` handles database
validation. Currently, it will return an empty list even for non-existent
databases, which violates the method contract that requires throwing
`DatabaseNotExistException` when the database doesn't exist.
Consider adding database existence validation:
```java
@Override
public List<String> listViews(String databaseName)
throws DatabaseNotExistException, CatalogException {
// Validate database exists
if (!databaseExists(databaseName)) {
throw new DatabaseNotExistException(catalogName(), databaseName);
}
// Gravitino does not support views yet; return empty to keep Flink
callers happy.
return Collections.emptyList();
}
```
```suggestion
throws DatabaseNotExistException, CatalogException {
// Validate database exists
try {
// We use listTables to check for schema existence, but ignore the
result.
catalog().asTableCatalog().listTables(Namespace.of(databaseName));
} catch (NoSuchSchemaException e) {
throw new DatabaseNotExistException(catalogName(), databaseName, e);
} catch (Exception e) {
throw new CatalogException(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]
