mchades commented on code in PR #10924:
URL: https://github.com/apache/gravitino/pull/10924#discussion_r3200691913
##########
core/src/test/java/org/apache/gravitino/connector/TestCatalogOperations.java:
##########
@@ -313,12 +314,144 @@ public View createView(
String defaultCatalog,
String defaultSchema,
Map<String, String> properties) {
- throw new UnsupportedOperationException("createView not implemented in
test");
+ if (views.containsKey(ident)) {
+ throw new ViewAlreadyExistsException("View %s already exists", ident);
+ }
+ AuditInfo auditInfo =
+
AuditInfo.builder().withCreator("test").withCreateTime(Instant.now()).build();
+ TestView view =
+ new TestView(
+ ident.name(),
+ comment,
+ columns == null ? new Column[0] : columns,
+ representations,
+ defaultCatalog,
+ defaultSchema,
+ properties == null ? ImmutableMap.of() :
ImmutableMap.copyOf(properties),
+ auditInfo);
+ views.put(ident, view);
+ return view;
}
@Override
public View alterView(NameIdentifier ident, ViewChange... changes) {
- throw new UnsupportedOperationException("alterView not implemented in
test");
+ if (!views.containsKey(ident)) {
+ throw new NoSuchViewException("View %s does not exist", ident);
+ }
+ TestView view = (TestView) views.get(ident);
+ String name = view.name();
+ String comment = view.comment();
+ String defaultCatalog = view.defaultCatalog();
+ String defaultSchema = view.defaultSchema();
+ Column[] columns = view.columns();
+ Representation[] representations = view.representations();
+ Map<String, String> properties = new HashMap<>(view.properties());
+
+ NameIdentifier newIdent = ident;
+ for (ViewChange change : changes) {
+ if (change instanceof ViewChange.RenameView) {
+ name = ((ViewChange.RenameView) change).getNewName();
+ newIdent = NameIdentifier.of(ident.namespace(), name);
Review Comment:
Fixed, thanks! Added the conflict check to throw
`ViewAlreadyExistsException` when the rename target already exists, consistent
with `alterTable` behavior.
##########
core/src/main/java/org/apache/gravitino/catalog/CapabilityHelpers.java:
##########
@@ -93,6 +94,18 @@ public static FilesetChange[] applyCapabilities(
.toArray(FilesetChange[]::new);
}
+ public static ViewChange[] applyCapabilities(Capability capabilities,
ViewChange... changes) {
+ return Arrays.stream(changes)
+ .map(
+ change -> {
+ if (change instanceof ViewChange.RenameView) {
+ return applyCapabilities((ViewChange.RenameView) change,
capabilities);
+ }
+ return change;
+ })
+ .toArray(ViewChange[]::new);
+ }
Review Comment:
Same reasoning as above: view column names are dialect-bound, not
catalog-capability-bound. Normalizing `ReplaceView` columns against catalog
capabilities is not appropriate.
##########
core/src/main/java/org/apache/gravitino/connector/capability/Capability.java:
##########
@@ -36,6 +36,7 @@ public interface Capability {
enum Scope {
SCHEMA,
TABLE,
+ VIEW,
COLUMN,
FILESET,
Review Comment:
Good point. Updated the PR description to call out that
`Capability.Scope.VIEW` is a new value added to an `@Evolving` SPI enum, and
downstream implementations using exhaustive `switch` over `Scope` without a
`default` branch will see compilation errors.
##########
core/src/test/java/org/apache/gravitino/catalog/TestViewOperationDispatcher.java:
##########
@@ -372,4 +374,155 @@ public void testLoadViewAfterManualDelete() throws
IOException {
ViewEntity viewEntity = entityStore.get(viewIdent, VIEW, ViewEntity.class);
Assertions.assertNotNull(viewEntity);
}
+
+ @Test
+ public void testCreateView() throws IOException {
+ Namespace viewNs = Namespace.of(metalake, catalog, "schema_create_view");
+ Map<String, String> schemaProps = ImmutableMap.of("k1", "v1", "k2", "v2");
+ schemaOperationDispatcher.createSchema(
+ NameIdentifier.of(viewNs.levels()), "comment", schemaProps);
+
+ NameIdentifier viewIdent = NameIdentifier.of(viewNs, "created_view");
+ Representation[] representations = {
+ SQLRepresentation.builder().withDialect("spark").withSql("SELECT
1").build()
+ };
+ Map<String, String> viewProps = ImmutableMap.of("k1", "v1", "p1", "pv1");
+
+ View created =
+ viewOperationDispatcher.createView(
+ viewIdent, "view comment", new Column[0], representations, null,
null, viewProps);
+
Review Comment:
Fixed, thanks! Added negative tests for: missing required property on
`createView`, reserved properties (`comment`/`gravitino.identifier`) on
`createView`, immutable property on `alterView`, and an assertion that the
returned view's properties do not expose `gravitino.identifier`.
--
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]
