Copilot commented on code in PR #10831:
URL: https://github.com/apache/gravitino/pull/10831#discussion_r3123060977
##########
catalogs/catalog-lakehouse-iceberg/src/main/java/org/apache/gravitino/catalog/lakehouse/iceberg/IcebergView.java:
##########
@@ -68,6 +70,16 @@ public Map<String, String> properties() {
return properties != null ? properties : Collections.emptyMap();
}
+ @Override
+ public Column[] columns() {
+ return new Column[0];
+ }
+
+ @Override
+ public Representation[] representations() {
+ return new Representation[0];
+ }
Review Comment:
`representations()` currently always returns an empty array. This makes
`View#sqlFor(...)` unusable for Iceberg views and conflicts with the API
contract implied by `ViewCatalog#createView(...)` /
`ViewChange#replaceView(...)` (which both expect at least one representation).
If Iceberg provides a SQL text or other definition in `LoadViewResponse`,
populate it here (e.g., as a `SQLRepresentation`), or otherwise document why
definitions are unavailable.
##########
core/src/main/java/org/apache/gravitino/catalog/ViewOperationDispatcher.java:
##########
@@ -89,6 +91,97 @@ 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 {
+ return doWithCatalog(
+ getCatalogIdentifier(ident),
+ c ->
+ c.doWithViewOps(
+ v ->
+ v.createView(
+ ident,
+ comment,
+ columns,
+ representations,
+ defaultCatalog,
+ defaultSchema,
+ properties)),
+ NoSuchSchemaException.class,
+ ViewAlreadyExistsException.class);
Review Comment:
`createView(...)` is not using `TreeLockUtils` (unlike `loadView(...)` and
other *OperationDispatcher classes such as
`TableOperationDispatcher#createTable`). This makes concurrent
create/load/import/drop of the same view prone to races. Consider (1)
loading/validating the schema first (similar to
`TableOperationDispatcher#createTable`), and (2) acquiring a schema-level WRITE
lock during creation so the entity-store auto-import logic can’t interleave
unexpectedly.
##########
core/src/main/java/org/apache/gravitino/catalog/ViewOperationDispatcher.java:
##########
@@ -89,6 +91,97 @@ 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 {
+ return doWithCatalog(
+ getCatalogIdentifier(ident),
+ c ->
+ c.doWithViewOps(
+ v ->
+ v.createView(
+ ident,
+ comment,
+ columns,
+ representations,
+ defaultCatalog,
+ defaultSchema,
+ properties)),
+ NoSuchSchemaException.class,
+ ViewAlreadyExistsException.class);
+ }
+
+ /**
+ * 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 {
+ return doWithCatalog(
+ getCatalogIdentifier(ident),
+ c -> c.doWithViewOps(v -> v.alterView(ident, changes)),
+ NoSuchViewException.class,
+ IllegalArgumentException.class);
Review Comment:
`alterView(...)` dispatches directly to the underlying catalog without any
tree lock. For consistency with other dispatchers (e.g.,
`TableOperationDispatcher#alterTable`) and to avoid races with concurrent
`loadView` auto-import and `dropView`, this should take an appropriate lock (at
least schema-level WRITE, and possibly catalog-level depending on rename
semantics).
##########
catalogs/catalog-lakehouse-iceberg/src/main/java/org/apache/gravitino/catalog/lakehouse/iceberg/IcebergView.java:
##########
@@ -68,6 +70,16 @@ public Map<String, String> properties() {
return properties != null ? properties : Collections.emptyMap();
}
+ @Override
+ public Column[] columns() {
+ return new Column[0];
+ }
Review Comment:
`columns()` currently always returns an empty array, even though
`IcebergView.fromLoadViewResponse(...)` has access to `LoadViewResponse`. If
the Iceberg response provides the view’s output schema, consider populating
`columns()` from it; returning empty here makes the API look like the schema is
unknown even when it may be available.
##########
api/src/main/java/org/apache/gravitino/rel/ViewCatalog.java:
##########
@@ -54,4 +65,54 @@ default boolean viewExists(NameIdentifier ident) {
return false;
}
}
+
+ /**
+ * 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. At least one
representation is
+ * expected.
+ * @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.
+ */
+ View createView(
+ NameIdentifier ident,
+ String comment,
+ Column[] columns,
+ Representation[] representations,
+ @Nullable String defaultCatalog,
+ @Nullable String defaultSchema,
+ Map<String, String> properties)
+ throws NoSuchSchemaException, ViewAlreadyExistsException;
Review Comment:
The `createView(...)` JavaDoc says `comment` may be `null`, but the
parameter is not annotated `@Nullable` (while `defaultCatalog`/`defaultSchema`
are). For a public API, it’s better to make nullability explicit and consistent
with the documentation.
##########
core/src/main/java/org/apache/gravitino/catalog/ViewOperationDispatcher.java:
##########
@@ -89,6 +91,97 @@ 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 {
+ return doWithCatalog(
+ getCatalogIdentifier(ident),
+ c ->
+ c.doWithViewOps(
+ v ->
+ v.createView(
+ ident,
+ comment,
+ columns,
+ representations,
+ defaultCatalog,
+ defaultSchema,
+ properties)),
+ NoSuchSchemaException.class,
+ ViewAlreadyExistsException.class);
+ }
+
+ /**
+ * 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 {
+ return doWithCatalog(
+ getCatalogIdentifier(ident),
+ c -> c.doWithViewOps(v -> v.alterView(ident, changes)),
+ NoSuchViewException.class,
+ IllegalArgumentException.class);
+ }
+
+ /**
+ * Drop a view from the catalog.
+ *
+ * @param ident A view identifier.
+ * @return True if the view is dropped, false if the view does not exist.
+ */
+ @Override
+ public boolean dropView(NameIdentifier ident) {
+ return doWithCatalog(
+ getCatalogIdentifier(ident),
+ c -> c.doWithViewOps(v -> v.dropView(ident)),
+ RuntimeException.class);
Review Comment:
`dropView(...)` does not acquire any tree lock and also does not clean up
the corresponding VIEW entity in the EntityStore. Since `loadView(...)`
auto-imports a `GenericEntity` into the store, dropping a view can leave stale
entries behind and race with concurrent imports. Consider taking a schema-level
WRITE lock and deleting the VIEW entity from the store (best-effort, similar to
`TableOperationDispatcher#dropTable`).
--
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]
