mchades commented on code in PR #12294:
URL: https://github.com/apache/gravitino/pull/12294#discussion_r3711363318
##########
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:
The guard looks fixed. Could you also add the two regression tests mentioned
above: removing `presto_view` 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:
The invariant looks fixed. Could you also add the schema-without-catalog
regression test mentioned above?
##########
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:
The rejection looks fixed. Could you also add regression coverage for
non-default `owner`, `runAsInvoker`, and `path`?
--
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]
