Copilot commented on code in PR #10831:
URL: https://github.com/apache/gravitino/pull/10831#discussion_r3130894101
##########
core/src/main/java/org/apache/gravitino/catalog/ViewOperationDispatcher.java:
##########
@@ -89,6 +93,153 @@ public View loadView(NameIdentifier ident) throws
NoSuchViewException {
return entityCombinedView;
}
+ /**
+ * List the views in a namespace.
+ *
+ * @param namespace A namespace.
+ * @return An array of view identifiers in the namespace.
+ * @throws NoSuchSchemaException If the schema does not exist.
+ */
+ @Override
+ public NameIdentifier[] listViews(Namespace namespace) throws
NoSuchSchemaException {
+ return doWithCatalog(
+ getCatalogIdentifier(NameIdentifier.of(namespace.levels())),
+ c -> c.doWithViewOps(v -> v.listViews(namespace)),
+ NoSuchSchemaException.class);
+ }
+
+ /**
+ * Create a view in the catalog.
+ *
+ * @param ident A view identifier.
+ * @param comment The view comment, may be {@code null}.
+ * @param columns The output columns of the view.
+ * @param representations The representations of the view.
+ * @param defaultCatalog The default catalog used to resolve unqualified
identifiers referenced by
+ * the view definition, or {@code null} if not set.
+ * @param defaultSchema The default schema used to resolve unqualified
identifiers referenced by
+ * the view definition, or {@code null} if not set.
+ * @param properties The view properties.
+ * @return The created view metadata.
+ * @throws NoSuchSchemaException If the schema does not exist.
+ * @throws ViewAlreadyExistsException If the view already exists.
+ */
+ @Override
+ public View createView(
+ NameIdentifier ident,
+ String comment,
+ Column[] columns,
+ Representation[] representations,
+ String defaultCatalog,
+ String defaultSchema,
+ Map<String, String> properties)
+ throws NoSuchSchemaException, ViewAlreadyExistsException {
+ SchemaDispatcher schemaDispatcher =
GravitinoEnv.getInstance().schemaDispatcher();
+ NameIdentifier schemaIdent = NameIdentifier.of(ident.namespace().levels());
+ schemaDispatcher.loadSchema(schemaIdent);
+
+ return TreeLockUtils.doWithTreeLock(
+ schemaIdent,
+ LockType.WRITE,
+ () ->
+ internalCreateView(
+ ident,
+ comment,
+ columns,
+ representations,
+ defaultCatalog,
+ defaultSchema,
+ properties));
+ }
Review Comment:
`createView(...)` currently only delegates to the underlying catalog and
never writes a VIEW entity into `EntityStore`. This means a view created via
Gravitino won’t be considered “imported” until a later `loadView(...)` call,
and `dropView(...)` will routinely log WARN on `NoSuchEntityException` when
dropping views that were never loaded. Consider persisting a `GenericEntity`
for the new view in `EntityStore` (similar to `importView(...)` /
`TableOperationDispatcher#internalCreateTable`) after the catalog create
succeeds.
##########
core/src/main/java/org/apache/gravitino/catalog/ViewOperationDispatcher.java:
##########
@@ -89,6 +93,153 @@ public View loadView(NameIdentifier ident) throws
NoSuchViewException {
return entityCombinedView;
}
+ /**
+ * List the views in a namespace.
+ *
+ * @param namespace A namespace.
+ * @return An array of view identifiers in the namespace.
+ * @throws NoSuchSchemaException If the schema does not exist.
+ */
+ @Override
+ public NameIdentifier[] listViews(Namespace namespace) throws
NoSuchSchemaException {
+ return doWithCatalog(
+ getCatalogIdentifier(NameIdentifier.of(namespace.levels())),
+ c -> c.doWithViewOps(v -> v.listViews(namespace)),
+ NoSuchSchemaException.class);
Review Comment:
`listViews(...)` doesn’t acquire a TreeLock, unlike other list operations
(e.g., `TopicOperationDispatcher#listTopics` /
`TableOperationDispatcher#listTables`). Without a READ lock on the schema
identifier, `listViews` can race with concurrent create/drop/rename and return
inconsistent results. Consider wrapping the `doWithCatalog(...)` call in
`TreeLockUtils.doWithTreeLock(NameIdentifier.of(namespace.levels()),
LockType.READ, ...)` for consistency and thread safety.
##########
core/src/main/java/org/apache/gravitino/catalog/ViewOperationDispatcher.java:
##########
@@ -89,6 +93,153 @@ public View loadView(NameIdentifier ident) throws
NoSuchViewException {
return entityCombinedView;
}
+ /**
+ * List the views in a namespace.
+ *
+ * @param namespace A namespace.
+ * @return An array of view identifiers in the namespace.
+ * @throws NoSuchSchemaException If the schema does not exist.
+ */
+ @Override
+ public NameIdentifier[] listViews(Namespace namespace) throws
NoSuchSchemaException {
+ return doWithCatalog(
+ getCatalogIdentifier(NameIdentifier.of(namespace.levels())),
+ c -> c.doWithViewOps(v -> v.listViews(namespace)),
+ NoSuchSchemaException.class);
+ }
+
+ /**
+ * Create a view in the catalog.
+ *
+ * @param ident A view identifier.
+ * @param comment The view comment, may be {@code null}.
+ * @param columns The output columns of the view.
+ * @param representations The representations of the view.
+ * @param defaultCatalog The default catalog used to resolve unqualified
identifiers referenced by
+ * the view definition, or {@code null} if not set.
+ * @param defaultSchema The default schema used to resolve unqualified
identifiers referenced by
+ * the view definition, or {@code null} if not set.
+ * @param properties The view properties.
+ * @return The created view metadata.
+ * @throws NoSuchSchemaException If the schema does not exist.
+ * @throws ViewAlreadyExistsException If the view already exists.
+ */
+ @Override
+ public View createView(
+ NameIdentifier ident,
+ String comment,
+ Column[] columns,
+ Representation[] representations,
+ String defaultCatalog,
+ String defaultSchema,
+ Map<String, String> properties)
+ throws NoSuchSchemaException, ViewAlreadyExistsException {
+ SchemaDispatcher schemaDispatcher =
GravitinoEnv.getInstance().schemaDispatcher();
+ NameIdentifier schemaIdent = NameIdentifier.of(ident.namespace().levels());
+ schemaDispatcher.loadSchema(schemaIdent);
+
+ return TreeLockUtils.doWithTreeLock(
+ schemaIdent,
+ LockType.WRITE,
+ () ->
+ internalCreateView(
+ ident,
+ comment,
+ columns,
+ representations,
+ defaultCatalog,
+ defaultSchema,
+ properties));
+ }
+
+ /**
+ * Apply the {@link ViewChange changes} to a view in the catalog.
+ *
+ * @param ident A view identifier.
+ * @param changes View changes to apply to the view.
+ * @return The updated view metadata.
+ * @throws NoSuchViewException If the view does not exist.
+ * @throws IllegalArgumentException If the change is rejected by the
implementation.
+ */
+ @Override
+ public View alterView(NameIdentifier ident, ViewChange... changes)
+ throws NoSuchViewException, IllegalArgumentException {
+ NameIdentifier lockIdentifier = ident;
+ LockType lockType = LockType.READ;
+ for (ViewChange change : changes) {
+ if (change instanceof ViewChange.RenameView) {
+ lockIdentifier = getSchemaIdentifier(ident);
+ lockType = LockType.WRITE;
+ break;
+ }
+ }
+
+ return TreeLockUtils.doWithTreeLock(
+ lockIdentifier,
+ lockType,
+ () ->
+ doWithCatalog(
+ getCatalogIdentifier(ident),
+ c -> c.doWithViewOps(v -> v.alterView(ident, changes)),
+ NoSuchViewException.class,
+ IllegalArgumentException.class));
+ }
Review Comment:
`alterView(...)` delegates the rename to the catalog but doesn’t reconcile
`EntityStore` state. If the view had been imported previously, the store will
keep the old identifier entry after a rename, while the new identifier may be
missing (triggering a redundant auto-import on first `loadView(newIdent)`).
Consider detecting `ViewChange.RenameView` and moving/updating the VIEW entity
in the store (delete old ident, create/update entity for new ident) within the
same lock scope.
--
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]
