Copilot commented on code in PR #11189:
URL: https://github.com/apache/gravitino/pull/11189#discussion_r3280829289
##########
flink-connector/flink-common/src/main/java/org/apache/gravitino/flink/connector/catalog/BaseCatalog.java:
##########
@@ -261,16 +327,28 @@ public void renameTable(ObjectPath tablePath, String
newTableName, boolean ignor
if (catalog().asTableCatalog().tableExists(identifier)) {
Review Comment:
The pre-check for rename conflicts only calls
`catalog().asTableCatalog().tableExists(...)`. If the rename target is an
existing *view* (in catalogs where views are not surfaced as tables), this
method will miss the conflict and proceed. Consider also checking
`catalog().asViewCatalog().viewExists(...)` (handling
`UnsupportedOperationException`) before attempting the rename, and throwing
`TableAlreadyExistException` when it’s taken.
##########
flink-connector/flink-common/src/test/java/org/apache/gravitino/flink/connector/catalog/TestBaseCatalog.java:
##########
@@ -136,33 +150,170 @@ public void testToGravitinoDistributionDefaultsToNone() {
}
@Test
- public void testListViewsReturnsEmptyWithoutDelegation() throws Exception {
- AbstractCatalog delegate = Mockito.mock(AbstractCatalog.class);
- BaseCatalog catalog = new TestableBaseCatalog(delegate);
+ public void testListViewsReturnsEmptyWhenViewCatalogUnsupported() throws
Exception {
+ Catalog gravitinoCatalog = mockUnsupportedViewCatalog();
+ BaseCatalog catalog =
+ new TestableBaseCatalog(Mockito.mock(AbstractCatalog.class),
gravitinoCatalog);
List<String> views = catalog.listViews("db");
Assertions.assertTrue(views.isEmpty());
- Mockito.verifyNoInteractions(delegate);
+ }
+
+ @Test
+ public void testListViewsDelegatesToViewCatalog() throws Exception {
+ ViewCatalog viewCatalog = Mockito.mock(ViewCatalog.class);
+ Mockito.when(viewCatalog.listViews(Namespace.of("db")))
+ .thenReturn(
+ new NameIdentifier[] {NameIdentifier.of("db", "v1"),
NameIdentifier.of("db", "v2")});
+ Catalog gravitinoCatalog = Mockito.mock(Catalog.class);
+ Mockito.when(gravitinoCatalog.asViewCatalog()).thenReturn(viewCatalog);
+
+ BaseCatalog catalog =
+ new TestableBaseCatalog(Mockito.mock(AbstractCatalog.class),
gravitinoCatalog);
+
+ List<String> views = catalog.listViews("db");
+
+ Assertions.assertEquals(ImmutableList.of("v1", "v2"), views);
+ }
+
+ @Test
+ public void testGetGravitinoViewChangesSetAndRemoveProperty() {
+ List<TableChange> tableChanges =
+ ImmutableList.of(TableChange.set("k1", "v1"), TableChange.reset("k2"));
+
+ org.apache.flink.table.api.Schema schema =
+ Schema.newBuilder().column("id", DataTypes.INT()).build();
Review Comment:
This test uses a fully qualified name `org.apache.flink.table.api.Schema`
even though `Schema` is already imported at the top of the file. Please use the
imported type to keep the code consistent and avoid unnecessary verbosity.
##########
flink-connector/flink-common/src/test/java/org/apache/gravitino/flink/connector/integration/test/hive/FlinkHiveCatalogIT.java:
##########
@@ -745,6 +748,278 @@ public void testDefaultFormatAndSerdeApplied() {
true);
}
+ @Test
+ public void testCreateView() {
+ String schemaName = "test_hive_create_view_db";
+ String viewName = "test_view_create";
+ String tableName = "test_view_base_table";
+ doWithSchema(
+ currentCatalog(),
+ schemaName,
+ catalog -> {
+ TestUtils.assertTableResult(
+ sql("CREATE TABLE %s (id INT, name STRING) WITH
('connector'='hive')", tableName),
+ ResultKind.SUCCESS);
+ TestUtils.assertTableResult(
+ sql(
+ "CREATE VIEW %s COMMENT 'view comment' AS SELECT id, name
FROM %s",
+ viewName, tableName),
+ ResultKind.SUCCESS);
+
+ // Verify via Gravitino ViewCatalog
+ ViewCatalog viewCatalog = catalog.asViewCatalog();
+ View view = viewCatalog.loadView(NameIdentifier.of(schemaName,
viewName));
+ Assertions.assertEquals(viewName, view.name());
+ Assertions.assertEquals("view comment", view.comment());
+ Assertions.assertEquals(1, view.representations().length);
+ Assertions.assertInstanceOf(SQLRepresentation.class,
view.representations()[0]);
+
+ // Verify via Flink catalog API
+ Optional<Catalog> flinkCatalog = tableEnv.getCatalog(catalog.name());
+ Assertions.assertTrue(flinkCatalog.isPresent());
+ try {
+ CatalogBaseTable flinkTable =
+ ((GravitinoHiveCatalog) flinkCatalog.get())
+ .getTable(new ObjectPath(schemaName, viewName));
+ Assertions.assertEquals(CatalogBaseTable.TableKind.VIEW,
flinkTable.getTableKind());
+ } catch (TableNotExistException e) {
+ Assertions.fail("view should exist in Flink catalog: " +
e.getMessage());
+ }
+ },
+ true);
+ }
+
+ @Test
+ public void testListViews() {
+ String schemaName = "test_hive_list_views_db";
+ String tableName = "test_list_view_base";
+ String view1 = "test_list_view_1";
+ String view2 = "test_list_view_2";
+ doWithSchema(
+ currentCatalog(),
+ schemaName,
+ catalog -> {
+ TestUtils.assertTableResult(
+ sql("CREATE TABLE %s (id INT) WITH ('connector'='hive')",
tableName),
+ ResultKind.SUCCESS);
+ TestUtils.assertTableResult(
+ sql("CREATE VIEW %s AS SELECT id FROM %s", view1, tableName),
ResultKind.SUCCESS);
+ TestUtils.assertTableResult(
+ sql("CREATE VIEW %s AS SELECT id FROM %s", view2, tableName),
ResultKind.SUCCESS);
+
+ List<String> views = Arrays.asList(tableEnv.listViews());
+ Assertions.assertTrue(views.contains(view1), "view1 not found in
SHOW VIEWS");
+ Assertions.assertTrue(views.contains(view2), "view2 not found in
SHOW VIEWS");
+
+ // Tables should not appear in listViews
+ Assertions.assertFalse(views.contains(tableName), "table should not
appear in listViews");
+
+ // Verify via Gravitino ViewCatalog
+ ViewCatalog viewCatalog = catalog.asViewCatalog();
+ NameIdentifier[] gravitinoViews =
+
viewCatalog.listViews(org.apache.gravitino.Namespace.of(schemaName));
+ List<String> gravitinoViewNames =
Review Comment:
Avoid using a fully qualified class name inside the method body
(`org.apache.gravitino.Namespace.of(...)`). Please add an import for
`org.apache.gravitino.Namespace` (or reuse an existing one) and reference
`Namespace.of(...)` directly to match the project's import style guidelines.
##########
flink-connector/flink-common/src/test/java/org/apache/gravitino/flink/connector/catalog/TestBaseCatalog.java:
##########
@@ -136,33 +150,170 @@ public void testToGravitinoDistributionDefaultsToNone() {
}
@Test
- public void testListViewsReturnsEmptyWithoutDelegation() throws Exception {
- AbstractCatalog delegate = Mockito.mock(AbstractCatalog.class);
- BaseCatalog catalog = new TestableBaseCatalog(delegate);
+ public void testListViewsReturnsEmptyWhenViewCatalogUnsupported() throws
Exception {
+ Catalog gravitinoCatalog = mockUnsupportedViewCatalog();
+ BaseCatalog catalog =
+ new TestableBaseCatalog(Mockito.mock(AbstractCatalog.class),
gravitinoCatalog);
List<String> views = catalog.listViews("db");
Assertions.assertTrue(views.isEmpty());
- Mockito.verifyNoInteractions(delegate);
+ }
+
+ @Test
+ public void testListViewsDelegatesToViewCatalog() throws Exception {
+ ViewCatalog viewCatalog = Mockito.mock(ViewCatalog.class);
+ Mockito.when(viewCatalog.listViews(Namespace.of("db")))
+ .thenReturn(
+ new NameIdentifier[] {NameIdentifier.of("db", "v1"),
NameIdentifier.of("db", "v2")});
+ Catalog gravitinoCatalog = Mockito.mock(Catalog.class);
+ Mockito.when(gravitinoCatalog.asViewCatalog()).thenReturn(viewCatalog);
+
+ BaseCatalog catalog =
+ new TestableBaseCatalog(Mockito.mock(AbstractCatalog.class),
gravitinoCatalog);
+
+ List<String> views = catalog.listViews("db");
+
+ Assertions.assertEquals(ImmutableList.of("v1", "v2"), views);
+ }
+
+ @Test
+ public void testGetGravitinoViewChangesSetAndRemoveProperty() {
+ List<TableChange> tableChanges =
+ ImmutableList.of(TableChange.set("k1", "v1"), TableChange.reset("k2"));
+
+ org.apache.flink.table.api.Schema schema =
+ Schema.newBuilder().column("id", DataTypes.INT()).build();
+
+ ViewChange[] changes =
+ BaseCatalog.getGravitinoViewChanges(
+ tableChanges, resolveView(schema, "SELECT 1", "comment"),
Dialects.FLINK);
+
+ Assertions.assertEquals(2, changes.length);
+ Assertions.assertInstanceOf(ViewChange.SetProperty.class, changes[0]);
+ Assertions.assertEquals("k1", ((ViewChange.SetProperty)
changes[0]).getProperty());
+ Assertions.assertEquals("v1", ((ViewChange.SetProperty)
changes[0]).getValue());
+
+ Assertions.assertInstanceOf(ViewChange.RemoveProperty.class, changes[1]);
+ Assertions.assertEquals("k2", ((ViewChange.RemoveProperty)
changes[1]).getProperty());
+ }
+
+ @Test
+ public void testGetGravitinoViewChangesBodyReplaceOnStructuralChange() {
+ List<TableChange> tableChanges =
+ ImmutableList.of(
+ TableChange.add(Column.physical("id", DataTypes.INT())),
TableChange.set("k1", "v1"));
+
+ org.apache.flink.table.api.Schema schema =
+ Schema.newBuilder().column("id", DataTypes.INT()).build();
Review Comment:
This test uses a fully qualified name `org.apache.flink.table.api.Schema`
even though `Schema` is already imported. Please use the imported type for
consistency.
##########
flink-connector/flink-common/src/test/java/org/apache/gravitino/flink/connector/catalog/TestBaseCatalog.java:
##########
@@ -136,33 +150,170 @@ public void testToGravitinoDistributionDefaultsToNone() {
}
@Test
- public void testListViewsReturnsEmptyWithoutDelegation() throws Exception {
- AbstractCatalog delegate = Mockito.mock(AbstractCatalog.class);
- BaseCatalog catalog = new TestableBaseCatalog(delegate);
+ public void testListViewsReturnsEmptyWhenViewCatalogUnsupported() throws
Exception {
+ Catalog gravitinoCatalog = mockUnsupportedViewCatalog();
+ BaseCatalog catalog =
+ new TestableBaseCatalog(Mockito.mock(AbstractCatalog.class),
gravitinoCatalog);
List<String> views = catalog.listViews("db");
Assertions.assertTrue(views.isEmpty());
- Mockito.verifyNoInteractions(delegate);
+ }
+
+ @Test
+ public void testListViewsDelegatesToViewCatalog() throws Exception {
+ ViewCatalog viewCatalog = Mockito.mock(ViewCatalog.class);
+ Mockito.when(viewCatalog.listViews(Namespace.of("db")))
+ .thenReturn(
+ new NameIdentifier[] {NameIdentifier.of("db", "v1"),
NameIdentifier.of("db", "v2")});
+ Catalog gravitinoCatalog = Mockito.mock(Catalog.class);
+ Mockito.when(gravitinoCatalog.asViewCatalog()).thenReturn(viewCatalog);
+
+ BaseCatalog catalog =
+ new TestableBaseCatalog(Mockito.mock(AbstractCatalog.class),
gravitinoCatalog);
+
+ List<String> views = catalog.listViews("db");
+
+ Assertions.assertEquals(ImmutableList.of("v1", "v2"), views);
+ }
+
+ @Test
+ public void testGetGravitinoViewChangesSetAndRemoveProperty() {
+ List<TableChange> tableChanges =
+ ImmutableList.of(TableChange.set("k1", "v1"), TableChange.reset("k2"));
+
+ org.apache.flink.table.api.Schema schema =
+ Schema.newBuilder().column("id", DataTypes.INT()).build();
+
+ ViewChange[] changes =
+ BaseCatalog.getGravitinoViewChanges(
+ tableChanges, resolveView(schema, "SELECT 1", "comment"),
Dialects.FLINK);
+
+ Assertions.assertEquals(2, changes.length);
+ Assertions.assertInstanceOf(ViewChange.SetProperty.class, changes[0]);
+ Assertions.assertEquals("k1", ((ViewChange.SetProperty)
changes[0]).getProperty());
+ Assertions.assertEquals("v1", ((ViewChange.SetProperty)
changes[0]).getValue());
+
+ Assertions.assertInstanceOf(ViewChange.RemoveProperty.class, changes[1]);
+ Assertions.assertEquals("k2", ((ViewChange.RemoveProperty)
changes[1]).getProperty());
+ }
+
+ @Test
+ public void testGetGravitinoViewChangesBodyReplaceOnStructuralChange() {
+ List<TableChange> tableChanges =
+ ImmutableList.of(
+ TableChange.add(Column.physical("id", DataTypes.INT())),
TableChange.set("k1", "v1"));
+
+ org.apache.flink.table.api.Schema schema =
+ Schema.newBuilder().column("id", DataTypes.INT()).build();
+
+ ViewChange[] changes =
+ BaseCatalog.getGravitinoViewChanges(
+ tableChanges, resolveView(schema, "SELECT id FROM t", "new
comment"), Dialects.FLINK);
+
+ // Should have exactly one SetProperty and one ReplaceView (order may vary)
+ Assertions.assertEquals(2, changes.length);
+ ViewChange.SetProperty setProp =
+ (ViewChange.SetProperty)
+ Arrays.stream(changes)
+ .filter(c -> c instanceof ViewChange.SetProperty)
+ .findFirst()
+ .orElseThrow(() -> new AssertionError("expected SetProperty"));
+ Assertions.assertEquals("k1", setProp.getProperty());
+ Assertions.assertEquals("v1", setProp.getValue());
+
+ ViewChange.ReplaceView replaceView =
+ (ViewChange.ReplaceView)
+ Arrays.stream(changes)
+ .filter(c -> c instanceof ViewChange.ReplaceView)
+ .findFirst()
+ .orElseThrow(() -> new AssertionError("expected ReplaceView"));
+ Assertions.assertEquals("new comment", replaceView.getComment());
+ Assertions.assertEquals(1, replaceView.getRepresentations().length);
+ Assertions.assertInstanceOf(SQLRepresentation.class,
replaceView.getRepresentations()[0]);
+ SQLRepresentation sqlRep = (SQLRepresentation)
replaceView.getRepresentations()[0];
+ Assertions.assertEquals(Dialects.FLINK, sqlRep.dialect());
+ Assertions.assertEquals("SELECT id FROM t", sqlRep.sql());
+ }
+
+ @Test
+ public void testGetGravitinoViewChangesFullReplace() {
+ org.apache.flink.table.api.Schema schema =
+ Schema.newBuilder().column("id", DataTypes.INT()).build();
Review Comment:
This test uses a fully qualified name `org.apache.flink.table.api.Schema`
even though `Schema` is already imported. Please use the imported type to match
the project's import style guidelines.
--
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]
