mchades commented on code in PR #12294: URL: https://github.com/apache/gravitino/pull/12294#discussion_r3729820297
########## docs/apache-hive-catalog.md: ########## @@ -241,9 +241,11 @@ Support for altering partitions is under development. - Supports list, create, load, alter, and drop for views stored in the Hive Metastore Service as `VIRTUAL_VIEW`. - Each view must contain exactly one SQL representation. -- Supports creating views with the `hive`, `flink`, or `spark` dialect. -- When loading an existing HMS view, Gravitino automatically detects whether the view uses the `hive`, `flink`, `spark`, or `trino` dialect. +- Supports creating views with the `hive`, `trino`, `flink`, or `spark` dialect. +- When loading an existing HMS view, Gravitino automatically detects whether the view uses the `hive`, `trino`, `flink`, or `spark` dialect. - For the `hive` and `flink` dialects, `defaultCatalog` and `defaultSchema` must be `null`. +- For the `trino` dialect, `defaultSchema` requires `defaultCatalog` to also be set (a schema without a catalog cannot be represented). +- The `trino` dialect requires at least one output column, and is stored using Trino's own native "Presto View" Hive Metastore encoding, so a view created through Gravitino is interoperable with a native Trino/Presto Hive connector pointed at the same Hive Metastore, and vice versa. The HMS `presto_view` property this relies on is reserved and managed internally based on the view's dialect; it cannot be set or removed directly. Gravitino's view model cannot represent a native Trino view's owner, `runAsInvoker`, or SQL path, so replacing an existing native view that has a non-default value for any of them is rejected rather than silently discarding it. Review Comment: Could we add an end-to-end IT under `trino-connector/integration-test` and wire it into the active `TrinoIT` runner? It should point a native `connector.name=hive` catalog and the Gravitino Hive catalog at the same HMS, then verify both directions: native Trino create → Gravitino REST load (SQL/columns/comment/defaults), and REST create → native Trino `SHOW CREATE VIEW`/`SELECT`. The unit tests exercise only our codec, not Trino’s actual encoder/decoder; a `timestamp(3)` column would also catch type-signature drift. ########## catalogs/catalog-hive/src/main/java/org/apache/gravitino/catalog/hive/TrinoNativeViewCodec.java: ########## @@ -0,0 +1,435 @@ +/* + * 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.core.JsonProcessingException; +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.io.IOException; +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; + // Trino's SQL path setting for the view; Gravitino's view model has no equivalent, so this is + // only populated on decode() (to let callers detect and reject a non-empty path) and is always + // written back empty by encode(). + final List<String> path; + + ViewDefinition( + String originalSql, + @Nullable String catalog, + @Nullable String schema, + List<ViewColumn> columns, + @Nullable String comment, + @Nullable String owner, + boolean runAsInvoker, + List<String> path) { + this.originalSql = originalSql; + this.catalog = catalog; + this.schema = schema; + this.columns = columns; + this.comment = comment; + this.owner = owner; + this.runAsInvoker = runAsInvoker; + this.path = path; + } + } + + /** + * 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 (JsonProcessingException 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 (IOException e) { + throw new IllegalArgumentException("Failed to decode Trino native view definition", e); + } + + JsonNode columnsNode = root.path("columns"); + if (!columnsNode.isArray() || columnsNode.isEmpty()) { + throw new IllegalArgumentException( + "Not a valid Trino native view: 'columns' field is missing, not an array, or empty"); + } + List<ViewColumn> columns = new ArrayList<>(); + for (JsonNode columnNode : columnsNode) { + String name = textOrNull(columnNode, "name"); + String type = textOrNull(columnNode, "type"); + if (name == null || type == null) { + throw new IllegalArgumentException( + "Not a valid Trino native view: a column is missing 'name' or 'type'"); + } + columns.add(new ViewColumn(name, type, textOrNull(columnNode, "comment"))); + } + + String originalSql = textOrNull(root, "originalSql"); + if (originalSql == null) { + throw new IllegalArgumentException( + "Not a valid Trino native view: 'originalSql' field is missing or null"); + } + + List<String> path = new ArrayList<>(); + for (JsonNode pathNode : root.path("path")) { + path.add(pathNode.toString()); + } + + return new ViewDefinition( + originalSql, + textOrNull(root, "catalog"), + textOrNull(root, "schema"), + columns, + textOrNull(root, "comment"), + textOrNull(root, "owner"), + root.path("runAsInvoker").asBoolean(true), + path); + } + + @Nullable + private static String textOrNull(JsonNode node, String field) { + JsonNode value = node.get(field); + return value == null || value.isNull() ? null : value.asText(); + } + + /** + * Converts a Gravitino type to its Trino type signature string (e.g. {@code varchar(10)}, {@code + * row(a integer,b varchar)}), for use in the encoded view's column list. + * + * @param type the Gravitino type + * @return the Trino type signature string + */ + static String toTrinoTypeString(Type type) { + switch (type.name()) { + case BOOLEAN: + return "boolean"; + case BYTE: + return "tinyint"; + case SHORT: + return "smallint"; + case INTEGER: + return "integer"; + case LONG: + return "bigint"; + case FLOAT: + return "real"; + case DOUBLE: + return "double"; + case STRING: + return "varchar"; + case VARCHAR: + return "varchar(" + ((Types.VarCharType) type).length() + ")"; + case FIXEDCHAR: + return "char(" + ((Types.FixedCharType) type).length() + ")"; + case DATE: + return "date"; + case TIMESTAMP: + Types.TimestampType timestampType = (Types.TimestampType) type; + if (timestampType.hasTimeZone()) { + throw new UnsupportedOperationException( + "Unsupported conversion to Trino type: TIMESTAMP WITH TIME ZONE is not supported by " + + "Hive-backed views"); + } + return "timestamp(6)"; Review Comment: `TimestampType` carries an explicit precision, but this always writes `timestamp(6)`, and `fromTrinoTypeString` drops the encoded `p`. A `timestamp(3)` column therefore round-trips as `timestamp(6)`. Could we preserve the precision in both directions, use Trino’s default `3` only when it is unset, and add a `timestamp(3)` round-trip test? ########## catalogs/catalog-hive/src/main/java/org/apache/gravitino/catalog/hive/TrinoNativeViewCodec.java: ########## @@ -0,0 +1,435 @@ +/* + * 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.core.JsonProcessingException; +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.io.IOException; +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; + // Trino's SQL path setting for the view; Gravitino's view model has no equivalent, so this is + // only populated on decode() (to let callers detect and reject a non-empty path) and is always + // written back empty by encode(). + final List<String> path; + + ViewDefinition( + String originalSql, + @Nullable String catalog, + @Nullable String schema, + List<ViewColumn> columns, + @Nullable String comment, + @Nullable String owner, + boolean runAsInvoker, + List<String> path) { + this.originalSql = originalSql; + this.catalog = catalog; + this.schema = schema; + this.columns = columns; + this.comment = comment; + this.owner = owner; + this.runAsInvoker = runAsInvoker; + this.path = path; + } + } + + /** + * 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 (JsonProcessingException 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 (IOException e) { + throw new IllegalArgumentException("Failed to decode Trino native view definition", e); + } + + JsonNode columnsNode = root.path("columns"); + if (!columnsNode.isArray() || columnsNode.isEmpty()) { + throw new IllegalArgumentException( + "Not a valid Trino native view: 'columns' field is missing, not an array, or empty"); + } + List<ViewColumn> columns = new ArrayList<>(); + for (JsonNode columnNode : columnsNode) { + String name = textOrNull(columnNode, "name"); + String type = textOrNull(columnNode, "type"); + if (name == null || type == null) { + throw new IllegalArgumentException( + "Not a valid Trino native view: a column is missing 'name' or 'type'"); + } + columns.add(new ViewColumn(name, type, textOrNull(columnNode, "comment"))); + } + + String originalSql = textOrNull(root, "originalSql"); + if (originalSql == null) { + throw new IllegalArgumentException( + "Not a valid Trino native view: 'originalSql' field is missing or null"); + } + + List<String> path = new ArrayList<>(); + for (JsonNode pathNode : root.path("path")) { + path.add(pathNode.toString()); + } + + return new ViewDefinition( + originalSql, + textOrNull(root, "catalog"), + textOrNull(root, "schema"), + columns, + textOrNull(root, "comment"), + textOrNull(root, "owner"), + root.path("runAsInvoker").asBoolean(true), + path); + } + + @Nullable + private static String textOrNull(JsonNode node, String field) { + JsonNode value = node.get(field); + return value == null || value.isNull() ? null : value.asText(); + } + + /** + * Converts a Gravitino type to its Trino type signature string (e.g. {@code varchar(10)}, {@code + * row(a integer,b varchar)}), for use in the encoded view's column list. + * + * @param type the Gravitino type + * @return the Trino type signature string + */ + static String toTrinoTypeString(Type type) { + switch (type.name()) { + case BOOLEAN: + return "boolean"; + case BYTE: + return "tinyint"; + case SHORT: + return "smallint"; + case INTEGER: + return "integer"; + case LONG: + return "bigint"; + case FLOAT: + return "real"; + case DOUBLE: + return "double"; + case STRING: + return "varchar"; + case VARCHAR: + return "varchar(" + ((Types.VarCharType) type).length() + ")"; + case FIXEDCHAR: + return "char(" + ((Types.FixedCharType) type).length() + ")"; + case DATE: + return "date"; + case TIMESTAMP: + Types.TimestampType timestampType = (Types.TimestampType) type; + if (timestampType.hasTimeZone()) { + throw new UnsupportedOperationException( + "Unsupported conversion to Trino type: TIMESTAMP WITH TIME ZONE is not supported by " + + "Hive-backed views"); + } + return "timestamp(6)"; + case DECIMAL: + Types.DecimalType decimalType = (Types.DecimalType) type; + return "decimal(" + decimalType.precision() + "," + decimalType.scale() + ")"; + case BINARY: + return "varbinary"; + case LIST: + return "array(" + toTrinoTypeString(((Types.ListType) type).elementType()) + ")"; + case MAP: + Types.MapType mapType = (Types.MapType) type; + return "map(" + + toTrinoTypeString(mapType.keyType()) + + "," + + toTrinoTypeString(mapType.valueType()) + + ")"; + case STRUCT: + Types.StructType structType = (Types.StructType) type; + StringBuilder row = new StringBuilder("row("); + Types.StructType.Field[] fields = structType.fields(); + for (int i = 0; i < fields.length; i++) { + if (i > 0) { + row.append(','); + } + row.append(rowFieldName(fields[i].name())) + .append(' ') + .append(toTrinoTypeString(fields[i].type())); + } + return row.append(')').toString(); + default: Review Comment: Native Trino view column types live in the encoded payload; HMS stores only the dummy column, so Hive physical type support should not be the limiter here. Gravitino’s existing Trino transformer supports `time(p)`, `timestamp(p) with time zone`, and `uuid`, while this codec rejects them. Could we support these shared types, or narrow the documented bidirectional interoperability claim? -- 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]
