Copilot commented on code in PR #12294:
URL: https://github.com/apache/gravitino/pull/12294#discussion_r3690202621
##########
catalogs/catalog-hive/src/main/java/org/apache/gravitino/catalog/hive/HiveViewCatalogOperations.java:
##########
@@ -457,27 +540,61 @@ private SQLRepresentation validateSQLRepresentation(
HiveView.SPARK_VERSION_KEY);
return selected;
default:
- // TODO(design-docs/gravitino-logical-view-management.md): support
creating trino HMS views.
throw new UnsupportedOperationException(
String.format(
- "Hive catalog currently supports only '%s', '%s' and '%s' view
dialects, but got '%s' for view %s",
- Dialects.HIVE, Dialects.FLINK, Dialects.SPARK,
selected.dialect(), ident));
+ "Hive catalog currently supports only [%s] view dialects, but
got '%s' for view %s",
+ SUPPORTED_VIEW_DIALECTS, selected.dialect(), ident));
}
}
- private String toHmsViewOriginalText(SQLRepresentation representation,
NameIdentifier ident) {
+ /**
+ * Sets or clears the {@code presto_view} marker in the given HMS property
map, so that a Trino
+ * dialect view is recognized as a native Trino view (see {@link
TrinoNativeViewCodec}).
+ */
+ private static void applyTrinoViewMarker(Map<String, String> params, String
dialect) {
+ if (!Dialects.TRINO.equalsIgnoreCase(dialect)) {
+ params.remove(TrinoNativeViewCodec.PRESTO_VIEW_FLAG);
+ return;
+ }
+ params.put(TrinoNativeViewCodec.PRESTO_VIEW_FLAG, "true");
+ }
+
+ private String toHmsViewOriginalText(
+ SQLRepresentation representation,
+ Column[] columns,
+ String comment,
+ String defaultCatalog,
+ String defaultSchema,
+ NameIdentifier ident) {
switch (representation.dialect().toLowerCase(Locale.ROOT)) {
case Dialects.HIVE:
case Dialects.FLINK:
case Dialects.SPARK:
return representation.sql();
+ case Dialects.TRINO:
+ List<TrinoNativeViewCodec.ViewColumn> viewColumns =
+ Arrays.stream(columns == null ? new Column[0] : columns)
+ .map(
+ c ->
+ new TrinoNativeViewCodec.ViewColumn(
+ c.name(),
+
TrinoNativeViewCodec.toTrinoTypeString(c.dataType()),
+ c.comment()))
+ .collect(Collectors.toList());
Review Comment:
For Trino dialect views, `columns` may be null/empty and will be encoded as
an empty `columns` list in the payload. `TrinoNativeViewCodec.decode()` rejects
empty `columns`, so this allows persisting a Trino view that becomes unloadable
on the next read.
##########
catalogs/catalog-hive/src/main/java/org/apache/gravitino/catalog/hive/TrinoNativeViewCodec.java:
##########
@@ -0,0 +1,408 @@
+/*
+ * 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.hive;
+
+import com.fasterxml.jackson.databind.JsonNode;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.fasterxml.jackson.databind.node.ArrayNode;
+import com.fasterxml.jackson.databind.node.ObjectNode;
+import java.util.ArrayList;
+import java.util.Base64;
+import java.util.List;
+import java.util.Locale;
+import java.util.regex.Pattern;
+import javax.annotation.Nullable;
+import org.apache.gravitino.rel.types.Type;
+import org.apache.gravitino.rel.types.Types;
+
+/**
+ * Encodes and decodes Trino/Presto's native "Presto View" HMS view format, so
that views created by
+ * Gravitino's Trino dialect are readable by a native Trino Hive connector
(and vice versa).
+ *
+ * <p>Trino recognizes a Hive Metastore VIRTUAL_VIEW table as a Trino view
when it carries the
+ * {@code presto_view=true} table parameter and its {@code comment} table
parameter equals {@code
+ * "Presto View"}; the view body is stored in {@code viewOriginalText} as
{@code "/* Presto View: "
+ * + base64(json) + " * /"}. The JSON payload mirrors Trino's {@code
ConnectorViewDefinition} wire
+ * format (see {@code io.trino.plugin.hive.ViewReaderUtil} /{@code
+ * io.trino.spi.connector.ConnectorViewDefinition} in the Trino source tree):
{@code Optional}
+ * fields are serialized as the raw value or JSON {@code null}, never omitted,
since Trino's decoder
+ * requires every field to be present.
+ */
+final class TrinoNativeViewCodec {
+
+ static final String PRESTO_VIEW_FLAG = "presto_view";
+ static final String PRESTO_VIEW_COMMENT = "Presto View";
+
+ private static final String VIEW_PREFIX = "/* Presto View: ";
+ private static final String VIEW_SUFFIX = " */";
+ private static final ObjectMapper MAPPER = new ObjectMapper();
+ private static final Pattern SIMPLE_IDENTIFIER =
Pattern.compile("[a-z_][a-z0-9_]*");
+
+ private TrinoNativeViewCodec() {}
+
+ /** A single view output column, mirroring Trino's {@code
ConnectorViewDefinition.ViewColumn}. */
+ static final class ViewColumn {
+ final String name;
+ final String type;
+ @Nullable final String comment;
+
+ ViewColumn(String name, String type, @Nullable String comment) {
+ this.name = name;
+ this.type = type;
+ this.comment = comment;
+ }
+ }
+
+ /** Mirrors the fields of Trino's {@code ConnectorViewDefinition}. */
+ static final class ViewDefinition {
+ final String originalSql;
+ @Nullable final String catalog;
+ @Nullable final String schema;
+ final List<ViewColumn> columns;
+ @Nullable final String comment;
+ @Nullable final String owner;
+ final boolean runAsInvoker;
+
+ ViewDefinition(
+ String originalSql,
+ @Nullable String catalog,
+ @Nullable String schema,
+ List<ViewColumn> columns,
+ @Nullable String comment,
+ @Nullable String owner,
+ boolean runAsInvoker) {
+ this.originalSql = originalSql;
+ this.catalog = catalog;
+ this.schema = schema;
+ this.columns = columns;
+ this.comment = comment;
+ this.owner = owner;
+ this.runAsInvoker = runAsInvoker;
+ }
+ }
+
+ /**
+ * Encodes a view definition into Trino's native {@code viewOriginalText}
format.
+ *
+ * @param definition the view definition to encode
+ * @return the encoded {@code "/* Presto View: ... * /"} string
+ */
+ static String encode(ViewDefinition definition) {
+ ObjectNode root = MAPPER.createObjectNode();
+ root.put("originalSql", definition.originalSql);
+ root.put("catalog", definition.catalog);
+ root.put("schema", definition.schema);
+
+ ArrayNode columns = root.putArray("columns");
+ for (ViewColumn column : definition.columns) {
+ ObjectNode columnNode = columns.addObject();
+ columnNode.put("name", column.name);
+ columnNode.put("type", column.type);
+ columnNode.put("comment", column.comment);
+ }
+
+ root.put("comment", definition.comment);
+ root.put("owner", definition.owner);
+ root.put("runAsInvoker", definition.runAsInvoker);
+ root.putArray("path");
+
+ byte[] bytes;
+ try {
+ bytes = MAPPER.writeValueAsBytes(root);
+ } catch (Exception e) {
+ throw new RuntimeException("Failed to encode Trino native view
definition", e);
+ }
Review Comment:
Catching a generic `Exception` here can mask unexpected runtime failures.
`ObjectMapper.writeValueAsBytes(...)` throws `JsonProcessingException`;
catching that specific exception keeps error handling narrower and clearer.
##########
catalogs/catalog-hive/src/main/java/org/apache/gravitino/catalog/hive/TrinoNativeViewCodec.java:
##########
@@ -0,0 +1,408 @@
+/*
+ * 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.hive;
+
+import com.fasterxml.jackson.databind.JsonNode;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.fasterxml.jackson.databind.node.ArrayNode;
+import com.fasterxml.jackson.databind.node.ObjectNode;
+import java.util.ArrayList;
+import java.util.Base64;
+import java.util.List;
+import java.util.Locale;
+import java.util.regex.Pattern;
+import javax.annotation.Nullable;
+import org.apache.gravitino.rel.types.Type;
+import org.apache.gravitino.rel.types.Types;
+
+/**
+ * Encodes and decodes Trino/Presto's native "Presto View" HMS view format, so
that views created by
+ * Gravitino's Trino dialect are readable by a native Trino Hive connector
(and vice versa).
+ *
+ * <p>Trino recognizes a Hive Metastore VIRTUAL_VIEW table as a Trino view
when it carries the
+ * {@code presto_view=true} table parameter and its {@code comment} table
parameter equals {@code
+ * "Presto View"}; the view body is stored in {@code viewOriginalText} as
{@code "/* Presto View: "
+ * + base64(json) + " * /"}. The JSON payload mirrors Trino's {@code
ConnectorViewDefinition} wire
+ * format (see {@code io.trino.plugin.hive.ViewReaderUtil} /{@code
+ * io.trino.spi.connector.ConnectorViewDefinition} in the Trino source tree):
{@code Optional}
+ * fields are serialized as the raw value or JSON {@code null}, never omitted,
since Trino's decoder
+ * requires every field to be present.
+ */
+final class TrinoNativeViewCodec {
+
+ static final String PRESTO_VIEW_FLAG = "presto_view";
+ static final String PRESTO_VIEW_COMMENT = "Presto View";
+
+ private static final String VIEW_PREFIX = "/* Presto View: ";
+ private static final String VIEW_SUFFIX = " */";
+ private static final ObjectMapper MAPPER = new ObjectMapper();
+ private static final Pattern SIMPLE_IDENTIFIER =
Pattern.compile("[a-z_][a-z0-9_]*");
+
+ private TrinoNativeViewCodec() {}
+
+ /** A single view output column, mirroring Trino's {@code
ConnectorViewDefinition.ViewColumn}. */
+ static final class ViewColumn {
+ final String name;
+ final String type;
+ @Nullable final String comment;
+
+ ViewColumn(String name, String type, @Nullable String comment) {
+ this.name = name;
+ this.type = type;
+ this.comment = comment;
+ }
+ }
+
+ /** Mirrors the fields of Trino's {@code ConnectorViewDefinition}. */
+ static final class ViewDefinition {
+ final String originalSql;
+ @Nullable final String catalog;
+ @Nullable final String schema;
+ final List<ViewColumn> columns;
+ @Nullable final String comment;
+ @Nullable final String owner;
+ final boolean runAsInvoker;
+
+ ViewDefinition(
+ String originalSql,
+ @Nullable String catalog,
+ @Nullable String schema,
+ List<ViewColumn> columns,
+ @Nullable String comment,
+ @Nullable String owner,
+ boolean runAsInvoker) {
+ this.originalSql = originalSql;
+ this.catalog = catalog;
+ this.schema = schema;
+ this.columns = columns;
+ this.comment = comment;
+ this.owner = owner;
+ this.runAsInvoker = runAsInvoker;
+ }
+ }
+
+ /**
+ * Encodes a view definition into Trino's native {@code viewOriginalText}
format.
+ *
+ * @param definition the view definition to encode
+ * @return the encoded {@code "/* Presto View: ... * /"} string
+ */
+ static String encode(ViewDefinition definition) {
+ ObjectNode root = MAPPER.createObjectNode();
+ root.put("originalSql", definition.originalSql);
+ root.put("catalog", definition.catalog);
+ root.put("schema", definition.schema);
+
+ ArrayNode columns = root.putArray("columns");
+ for (ViewColumn column : definition.columns) {
+ ObjectNode columnNode = columns.addObject();
+ columnNode.put("name", column.name);
+ columnNode.put("type", column.type);
+ columnNode.put("comment", column.comment);
+ }
+
+ root.put("comment", definition.comment);
+ root.put("owner", definition.owner);
+ root.put("runAsInvoker", definition.runAsInvoker);
+ root.putArray("path");
+
+ byte[] bytes;
+ try {
+ bytes = MAPPER.writeValueAsBytes(root);
+ } catch (Exception e) {
+ throw new RuntimeException("Failed to encode Trino native view
definition", e);
+ }
+ return VIEW_PREFIX + Base64.getEncoder().encodeToString(bytes) +
VIEW_SUFFIX;
+ }
+
+ /**
+ * Decodes a Trino native {@code viewOriginalText} value.
+ *
+ * @param viewOriginalText the raw {@code viewOriginalText} HMS field value
+ * @return the decoded view definition
+ */
+ static ViewDefinition decode(String viewOriginalText) {
+ if (viewOriginalText == null
+ || !viewOriginalText.startsWith(VIEW_PREFIX)
+ || !viewOriginalText.endsWith(VIEW_SUFFIX)) {
+ throw new IllegalArgumentException(
+ "Not a valid Trino native view: viewOriginalText is missing the
Presto View prefix/suffix");
+ }
+ String encoded =
+ viewOriginalText.substring(
+ VIEW_PREFIX.length(), viewOriginalText.length() -
VIEW_SUFFIX.length());
+ byte[] bytes = Base64.getDecoder().decode(encoded);
+
+ JsonNode root;
+ try {
+ root = MAPPER.readTree(bytes);
+ } catch (Exception e) {
+ throw new IllegalArgumentException("Failed to decode Trino native view
definition", e);
+ }
Review Comment:
Catching a generic `Exception` here is broader than needed.
`ObjectMapper.readTree(byte[])` throws `IOException`; catching that specific
type avoids unintentionally swallowing other runtime issues.
##########
catalogs/catalog-hive/src/main/java/org/apache/gravitino/catalog/hive/HiveViewCatalogOperations.java:
##########
@@ -216,9 +243,20 @@ public View alterView(NameIdentifier ident, ViewChange...
changes)
replace.getDefaultSchema(),
updatedProperties,
ident);
- updatedColumns = copyColumns(replace.getColumns());
- updatedComment = replace.getComment();
- updatedViewOriginalText = toHmsViewOriginalText(sqlRepresentation,
ident);
+ updatedColumns = hmsColumns(replace.getColumns(),
sqlRepresentation.dialect());
+ updatedViewOriginalText =
+ toHmsViewOriginalText(
+ sqlRepresentation,
+ replace.getColumns(),
+ replace.getComment(),
+ replace.getDefaultCatalog(),
+ replace.getDefaultSchema(),
+ ident);
+ updatedComment =
+ Dialects.TRINO.equalsIgnoreCase(sqlRepresentation.dialect())
+ ? TrinoNativeViewCodec.PRESTO_VIEW_COMMENT
+ : replace.getComment();
+ applyTrinoViewMarker(updatedProperties, sqlRepresentation.dialect());
Review Comment:
`isTrinoView` is computed once from the current HMS table and never updated
after a `ReplaceView` change. If a single `alterView(...)` call includes
`ReplaceView` that changes the dialect, subsequent `SetProperty(comment)` /
`RemoveProperty(comment)` changes in the same call can be incorrectly
allowed/blocked, potentially desynchronizing the HMS comment marker for Trino
views.
--
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]
