Copilot commented on code in PR #11926:
URL: https://github.com/apache/gravitino/pull/11926#discussion_r3552414135
##########
trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/catalog/CatalogConnectorMetadataAdapter.java:
##########
@@ -148,6 +154,78 @@ public GravitinoTable createTable(ConnectorTableMetadata
tableMetadata) {
return new GravitinoTable(schemaName, tableName, columns, comment,
properties);
}
+ /**
+ * Transform Gravitino view metadata to Trino ConnectorViewDefinition. Owner
is not supported by
+ * Gravitino views, so the resulting definition always has an empty owner;
since Trino requires an
+ * owner for run-as-definer views, {@code runAsInvoker} is always {@code
true}.
+ *
+ * @param view the Gravitino view
+ * @return the Trino ConnectorViewDefinition
+ */
+ public ConnectorViewDefinition getViewDefinition(GravitinoView view) {
+ List<ViewColumn> columns =
+ view.getColumns().stream()
+ .map(
+ column ->
+ new ViewColumn(
+ column.getName(),
+
dataTypeTransformer.getTrinoType(column.getType()).getTypeId(),
+ Optional.ofNullable(column.getComment())))
+ .collect(Collectors.toList());
+
+ return new ConnectorViewDefinition(
+ view.getSql(),
+ Optional.ofNullable(view.getDefaultCatalog()),
+ Optional.ofNullable(view.getDefaultSchema()),
+ columns,
+ Optional.ofNullable(view.getComment()),
+ Optional.empty(),
+ true,
+ List.of());
Review Comment:
`getViewDefinition` passes `view.getSql()` directly into
`ConnectorViewDefinition`, but `GravitinoView#getSql()` is nullable. Even if
current call sites filter out views without a Trino SQL representation, this
method should defensively fail fast with a clear error to avoid a potential NPE
if it’s ever called with a view lacking SQL.
##########
trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/catalog/CatalogConnectorMetadata.java:
##########
@@ -454,4 +473,192 @@ public Function getFunction(String schemaName, String
functionName) {
}
return functionCatalog.getFunction(NameIdentifier.of(schemaName,
functionName));
}
+
+ /**
+ * Checks whether the catalog supports view operations.
+ *
+ * @return true if the catalog supports view operations, false otherwise
+ */
+ public boolean supportsViews() {
+ return viewCatalog != null;
+ }
+
+ /**
+ * Retrieves the Gravitino view for the specified name, if it exists and has
a Trino dialect SQL
+ * representation.
+ *
+ * @param schemaName the name of the schema
+ * @param viewName the name of the view
+ * @return an {@link Optional} containing the Gravitino view, or {@link
Optional#empty()} if the
+ * view does not exist or has no Trino dialect SQL representation
+ */
+ public Optional<GravitinoView> getViewIfPresent(String schemaName, String
viewName) {
+ if (!supportsViews()) {
+ return Optional.empty();
+ }
+ try {
+ View view = viewCatalog.loadView(NameIdentifier.of(schemaName,
viewName));
+ GravitinoView gravitinoView = new GravitinoView(schemaName, viewName,
view);
+ if (gravitinoView.getSql() == null) {
+ // The view exists but has no Trino dialect SQL representation, so it
is not visible to
+ // Trino.
+ return Optional.empty();
+ }
+ return Optional.of(gravitinoView);
+ } catch (NoSuchViewException | UnsupportedOperationException e) {
+ return Optional.empty();
+ }
+ }
+
+ /**
+ * Retrieves the Gravitino view for the specified name.
+ *
+ * @param schemaName the name of the schema
+ * @param viewName the name of the view
+ * @return the Gravitino view
+ * @throws TrinoException if the view does not exist or has no Trino dialect
SQL representation
+ */
+ public GravitinoView getView(String schemaName, String viewName) {
+ return getViewIfPresent(schemaName, viewName)
+ .orElseThrow(
+ () ->
+ new TrinoException(
+ GravitinoErrorCode.GRAVITINO_VIEW_NOT_EXISTS, "View does
not exist"));
+ }
+
+ /**
+ * Lists the names of all views in the specified schema.
+ *
+ * @param schemaName the name of the schema
+ * @return a list of view names, or an empty list if the catalog does not
support views
+ */
+ public List<String> listViews(String schemaName) {
+ if (!supportsViews()) {
+ return List.of();
+ }
+ try {
+ NameIdentifier[] views = viewCatalog.listViews(Namespace.of(schemaName));
+ return Arrays.stream(views)
+ .map(NameIdentifier::name)
+ .filter(viewName -> getViewIfPresent(schemaName,
viewName).isPresent())
+ .toList();
+ } catch (UnsupportedOperationException e) {
+ return List.of();
+ } catch (NoSuchSchemaException e) {
+ throw new TrinoException(
+ GravitinoErrorCode.GRAVITINO_SCHEMA_NOT_EXISTS,
SCHEMA_DOES_NOT_EXIST_MSG, e);
+ }
+ }
+
+ /**
+ * Creates or replaces a view in the catalog.
+ *
+ * @param view the Gravitino view, with the Trino dialect SQL definition set
+ * @param replace whether to replace the view if it already exists
+ */
+ public void createView(GravitinoView view, boolean replace) {
+ if (!supportsViews()) {
+ throw new TrinoException(
+ GravitinoErrorCode.GRAVITINO_UNSUPPORTED_OPERATION, "Catalog does
not support views");
+ }
+ NameIdentifier identifier = NameIdentifier.of(view.getSchemaName(),
view.getName());
+ SQLRepresentation[] representations = {
+
SQLRepresentation.builder().withDialect(Dialects.TRINO).withSql(view.getSql()).build()
+ };
+ try {
+ boolean exists = viewCatalog.viewExists(identifier);
+ if (exists && replace) {
+ viewCatalog.alterView(
+ identifier,
+ ViewChange.replaceView(
+ view.getRawColumns(),
+ representations,
+ view.getDefaultCatalog(),
+ view.getDefaultSchema(),
+ view.getComment()));
Review Comment:
In the CREATE OR REPLACE path, `ViewChange.replaceView(...)` is called with
only the new Trino representation, which discards any existing non-Trino
representations on the view. Since `replaceView` is defined as a full
replacement of the view body (including representations), this can
unintentionally delete other-engine SQL representations when a Trino user
replaces a view.
##########
trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/metadata/GravitinoView.java:
##########
@@ -0,0 +1,203 @@
+/*
+ * 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.trino.connector.metadata;
+
+import com.google.common.collect.ImmutableList;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import javax.annotation.Nullable;
+import org.apache.gravitino.rel.Column;
+import org.apache.gravitino.rel.Dialects;
+import org.apache.gravitino.rel.SQLRepresentation;
+import org.apache.gravitino.rel.View;
+
+/**
+ * Helps the Apache Gravitino connector access view metadata using the Trino
dialect SQL
+ * representation from the Gravitino client.
+ */
+public class GravitinoView {
+
+ private final String schemaName;
+ private final String viewName;
+ private final List<GravitinoColumn> columns;
+ private final String comment;
+ private final Map<String, String> properties;
+ private final String sql;
+ @Nullable private final String defaultCatalog;
Review Comment:
`comment` and `sql` can both be null (they’re read from `View#comment()` and
`View#sqlFor(...)`), but they’re declared as non-null fields. This makes the
nullability contract unclear and can lead to accidental NPEs in future call
sites; please mark them `@Nullable` consistently (as already done for
defaultCatalog/defaultSchema).
##########
trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/metadata/GravitinoView.java:
##########
@@ -0,0 +1,203 @@
+/*
+ * 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.trino.connector.metadata;
+
+import com.google.common.collect.ImmutableList;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import javax.annotation.Nullable;
+import org.apache.gravitino.rel.Column;
+import org.apache.gravitino.rel.Dialects;
+import org.apache.gravitino.rel.SQLRepresentation;
+import org.apache.gravitino.rel.View;
+
+/**
+ * Helps the Apache Gravitino connector access view metadata using the Trino
dialect SQL
+ * representation from the Gravitino client.
+ */
+public class GravitinoView {
+
+ private final String schemaName;
+ private final String viewName;
+ private final List<GravitinoColumn> columns;
+ private final String comment;
+ private final Map<String, String> properties;
+ private final String sql;
+ @Nullable private final String defaultCatalog;
+ @Nullable private final String defaultSchema;
+
+ /**
+ * Constructs a new GravitinoView by unwrapping the {@link Dialects#TRINO}
SQL representation from
+ * a Gravitino {@link View}.
+ *
+ * @param schemaName the schema name
+ * @param viewName the view name
+ * @param view the Gravitino view metadata
+ */
+ public GravitinoView(String schemaName, String viewName, View view) {
+ this.schemaName = schemaName;
+ this.viewName = viewName;
+
+ ImmutableList.Builder<GravitinoColumn> viewColumns =
ImmutableList.builder();
+ for (int i = 0; i < view.columns().length; i++) {
+ viewColumns.add(new GravitinoColumn(view.columns()[i], i));
+ }
+ this.columns = viewColumns.build();
+ this.comment = view.comment();
+ this.properties = view.properties();
+
+ Optional<SQLRepresentation> trinoRepresentation =
view.sqlFor(Dialects.TRINO);
+ this.sql = trinoRepresentation.map(SQLRepresentation::sql).orElse(null);
+ this.defaultCatalog = view.defaultCatalog();
+ this.defaultSchema = view.defaultSchema();
+ }
+
+ /**
+ * Constructs a new GravitinoView for the Trino-to-Gravitino creation
direction.
+ *
+ * @param schemaName the schema name
+ * @param viewName the view name
+ * @param columns the view output columns
+ * @param comment the view comment
+ * @param properties the view properties
+ * @param sql the Trino dialect SQL definition 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
+ */
+ public GravitinoView(
+ String schemaName,
+ String viewName,
+ List<GravitinoColumn> columns,
+ String comment,
+ Map<String, String> properties,
+ String sql,
+ @Nullable String defaultCatalog,
+ @Nullable String defaultSchema) {
+ this.schemaName = schemaName;
+ this.viewName = viewName;
+ this.columns = columns;
+ this.comment = comment;
+ this.properties = properties;
+ this.sql = sql;
+ this.defaultCatalog = defaultCatalog;
+ this.defaultSchema = defaultSchema;
+ }
+
+ /**
+ * Retrieves the schema name of the view.
+ *
+ * @return the schema name of the view
+ */
+ public String getSchemaName() {
+ return schemaName;
+ }
+
+ /**
+ * Retrieves the name of the view.
+ *
+ * @return the name of the view
+ */
+ public String getName() {
+ return viewName;
+ }
+
+ /**
+ * Retrieves the columns of the view.
+ *
+ * @return the columns of the view
+ */
+ public List<GravitinoColumn> getColumns() {
+ return columns;
+ }
+
+ /**
+ * Retrieves the raw columns of the view.
+ *
+ * @return the raw columns of the view
+ */
+ public Column[] getRawColumns() {
+ Column[] gravitinoColumns = new Column[columns.size()];
+ for (int i = 0; i < columns.size(); i++) {
+ GravitinoColumn column = columns.get(i);
+ gravitinoColumns[i] =
+ Column.of(
+ column.getName(),
+ column.getType(),
+ column.getComment(),
+ column.isNullable(),
+ column.isAutoIncrement(),
+ column.getDefaultValue());
+ }
+ return gravitinoColumns;
+ }
+
+ /**
+ * Retrieves the comment of the view.
+ *
+ * @return the comment of the view
+ */
+ public String getComment() {
+ return comment;
+ }
Review Comment:
`getComment()` may return null (the comment is optional), but the method is
currently declared as returning a non-null `String`. Marking the return
`@Nullable` keeps the API contract accurate and consistent with how callers
already wrap it in `Optional.ofNullable(...)`.
--
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]
