mchades commented on code in PR #12294:
URL: https://github.com/apache/gravitino/pull/12294#discussion_r3701892879
##########
catalogs/catalog-hive/src/main/java/org/apache/gravitino/catalog/hive/HiveViewCatalogOperations.java:
##########
@@ -457,27 +548,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));
+ }
+ }
+
+ /**
+ * 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,
NameIdentifier ident) {
+ 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());
+ return TrinoNativeViewCodec.encode(
+ new TrinoNativeViewCodec.ViewDefinition(
+ representation.sql(),
+ defaultCatalog,
+ defaultSchema,
+ viewColumns,
+ comment,
+ /* owner= */ null,
+ /* runAsInvoker= */ true));
Review Comment:
A native Trino view definition also carries `owner`, `runAsInvoker`, and
`path`. At present, `path` is not decoded, `toHiveView` drops the
owner/security fields, and every replacement is encoded with `owner=null`,
`runAsInvoker=true`, and an empty path.
I verified that replacing a native view with `owner="alice"` and
`runAsInvoker=false` silently changes it to an invoker view with no owner.
Since Trino uses `SECURITY DEFINER` by default, this can change both
authorization and name-resolution behavior.
Could we preserve these fields end to end? If the Gravitino view model
cannot represent them yet, rejecting unsupported native definitions or
replacements would be safer than silently changing their semantics.
##########
catalogs/catalog-hive/src/main/java/org/apache/gravitino/catalog/hive/HiveViewCatalogOperations.java:
##########
@@ -177,6 +189,12 @@ public View alterView(NameIdentifier ident, ViewChange...
changes)
throw new NoSuchViewException("No view named %s (it is a table, not a
view)", ident.name());
}
+ // Reuse the same dialect detection as loadHiveView()/toHiveView() so
that a presto_view
+ // entry that is not a plain Trino view (e.g. a Trino/Presto
materialized view) is rejected
+ // here too, instead of being silently treated as a non-Trino view.
+ boolean isTrinoView =
+
Dialects.TRINO.equalsIgnoreCase(HiveView.detectDialect(currentHiveTable.properties()));
Review Comment:
Could we also prevent `presto_view` from being modified as a regular view
property in the `ViewChange.SetProperty` / `ViewChange.RemoveProperty` branches
below?
This property is part of the native Trino view storage contract. I verified
that removing it from a Trino view succeeds and is persisted; the returned and
subsequently loaded view is then classified as a Hive view and exposes the
encoded `/* Presto View: ... */` wrapper as its SQL.
Please treat this key as reserved, or restore it according to the final
dialect after applying all changes. It would also be useful to cover both
removing it from a Trino view and setting it on a non-Trino view.
##########
catalogs/catalog-hive/src/main/java/org/apache/gravitino/catalog/hive/HiveViewCatalogOperations.java:
##########
@@ -431,6 +511,17 @@ private SQLRepresentation validateSQLRepresentation(
defaultSchema,
ident);
return selected;
+ case Dialects.TRINO:
Review Comment:
Could we add the invariant `defaultSchema == null || defaultCatalog != null`
here? This still permits both values to be null; it rejects only the
schema-without-catalog combination.
Trino's `ConnectorViewDefinition` explicitly rejects a schema when the
catalog is absent. I verified that Gravitino currently accepts this input and
persists a payload that native Trino cannot decode. A regression test for the
schema-only case would help.
Reference:
https://github.com/trinodb/trino/blob/435/core/trino-spi/src/main/java/io/trino/spi/connector/ConnectorViewDefinition.java#L30-L65
--
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]
