Copilot commented on code in PR #10924: URL: https://github.com/apache/gravitino/pull/10924#discussion_r3199768447
########## core/src/main/java/org/apache/gravitino/catalog/ViewNormalizeDispatcher.java: ########## @@ -0,0 +1,123 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.gravitino.catalog; + +import static org.apache.gravitino.catalog.CapabilityHelpers.applyCapabilities; +import static org.apache.gravitino.catalog.CapabilityHelpers.applyCaseSensitive; +import static org.apache.gravitino.catalog.CapabilityHelpers.getCapability; + +import java.util.Map; +import javax.annotation.Nullable; +import org.apache.commons.lang3.ArrayUtils; +import org.apache.gravitino.NameIdentifier; +import org.apache.gravitino.Namespace; +import org.apache.gravitino.connector.capability.Capability; +import org.apache.gravitino.exceptions.NoSuchSchemaException; +import org.apache.gravitino.exceptions.NoSuchViewException; +import org.apache.gravitino.exceptions.ViewAlreadyExistsException; +import org.apache.gravitino.rel.Column; +import org.apache.gravitino.rel.Representation; +import org.apache.gravitino.rel.View; +import org.apache.gravitino.rel.ViewChange; + +public class ViewNormalizeDispatcher implements ViewDispatcher { + + private final CatalogManager catalogManager; + private final ViewDispatcher dispatcher; + + public ViewNormalizeDispatcher(ViewDispatcher dispatcher, CatalogManager catalogManager) { + this.dispatcher = dispatcher; + this.catalogManager = catalogManager; + } + + @Override + public NameIdentifier[] listViews(Namespace namespace) throws NoSuchSchemaException { + // The constraints of the name spec may be more strict than underlying catalog, + // and for compatibility reasons, we only apply case-sensitive capabilities here. + Namespace caseSensitiveNs = normalizeCaseSensitive(namespace); + NameIdentifier[] identifiers = dispatcher.listViews(caseSensitiveNs); + return normalizeCaseSensitive(identifiers); + } + + @Override + public View loadView(NameIdentifier ident) throws NoSuchViewException { + return dispatcher.loadView(normalizeCaseSensitive(ident)); + } + + @Override + public boolean viewExists(NameIdentifier ident) { + return dispatcher.viewExists(normalizeCaseSensitive(ident)); + } + + @Override + public View createView( + NameIdentifier ident, + String comment, + Column[] columns, + Representation[] representations, + @Nullable String defaultCatalog, + @Nullable String defaultSchema, + Map<String, String> properties) + throws NoSuchSchemaException, ViewAlreadyExistsException { + return dispatcher.createView( + normalizeNameIdentifier(ident), + comment, + columns, + representations, + defaultCatalog, + defaultSchema, + properties); Review Comment: `ViewNormalizeDispatcher#createView` only normalizes the `NameIdentifier` but passes `columns` through unchanged. This differs from `TableNormalizeDispatcher#createTable`, which applies capability-based normalization/validation to column names as well. Views can currently be created (or replaced) with column names that violate case-sensitivity/specification capabilities. Consider applying `CapabilityHelpers.applyCapabilities(columns, capability)` (and any related column validation) before delegating. ########## 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: `TestCatalogOperations.alterView` handles `RenameView` but doesn’t check whether `newIdent` already exists. This can silently overwrite an existing view entry in the in-memory map, which diverges from expected catalog semantics (and from `alterTable` above, which throws on rename conflicts). Please add a conflict check and throw `ViewAlreadyExistsException` when renaming to an existing view. ########## 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: `CapabilityHelpers.applyCapabilities(Capability, ViewChange...)` currently normalizes only `RenameView`. `ViewChange.ReplaceView` carries `Column[]` and should also have its columns normalized/validated against the catalog capabilities (similar to table column change handling). Without this, `ViewNormalizeDispatcher#alterView` won’t normalize column names when a view body is replaced. ########## 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: PR description says there are no user-facing API changes, but adding `Capability.Scope.VIEW` extends a public connector SPI enum. This can be source-incompatible for downstream `Capability` implementations that use an exhaustive `switch` over `Scope` without a `default` branch. Consider calling this out in the PR description/release notes (or verifying downstream compatibility expectations given `@Evolving`). ########## 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: New view property validation/hidden-property behavior is introduced in `ViewOperationDispatcher` (e.g., reserved `gravitino.identifier`, required `k1`, immutable/reserved properties on alter), but the added tests don’t assert these behaviors. Please add negative/edge tests (e.g., createView missing required `k1`, create/alter attempting to set `gravitino.identifier`/reserved properties) and an assertion that returned `View#properties()` does not expose hidden internal keys like `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]
