mchades commented on code in PR #11926:
URL: https://github.com/apache/gravitino/pull/11926#discussion_r3654175780
##########
catalogs/catalog-hive/src/main/java/org/apache/gravitino/catalog/hive/HiveViewCatalogOperations.java:
##########
@@ -431,6 +443,22 @@ private SQLRepresentation validateSQLRepresentation(
defaultSchema,
ident);
return selected;
+ case Dialects.TRINO:
+ // HMS has no field to persist a Trino view's default catalog/schema,
so any value
Review Comment:
**[P1] Please preserve the Trino resolution context instead of accepting and
dropping it.** Trino supplies the session catalog/schema in
`ConnectorViewDefinition`; a normal `USE gt_hive.db; CREATE VIEW v AS SELECT *
FROM t` is valid at creation, but this branch stores neither value, so reload
returns null defaults and Trino cannot resolve `t`. Lowering the log level does
not change that behavior. Could we persist both values in reserved HMS
properties and restore them in `toHiveView()` (or reject the definition), and
add a Hive IT using `USE` plus an unqualified source table?
##########
trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/catalog/CatalogConnectorMetadataAdapter.java:
##########
@@ -148,6 +155,83 @@ 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) {
+ Preconditions.checkArgument(
+ view.getSql() != null,
+ "View %s.%s has no Trino dialect SQL representation",
+ view.getSchemaName(),
+ view.getName());
+ 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()),
Review Comment:
**[P2] Avoid constructing an invalid schema-only
`ConnectorViewDefinition`.** Iceberg view metadata can have a non-null default
namespace while its default catalog is null, and `catalog-lakehouse-iceberg`
currently supports that shape. This adapter converts it to an empty catalog
plus a present schema, but the Trino constructor throws
`IllegalArgumentException` because a schema requires a catalog. As a result,
loading a Trino-representation view with that metadata fails before `SELECT` or
`SHOW CREATE VIEW` can use it. Could we normalize the catalog to the current
connector catalog, or hide/reject the view with a controlled error, and add a
regression test?
##########
trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/catalog/CatalogConnectorMetadataAdapter.java:
##########
@@ -148,6 +155,83 @@ 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) {
+ Preconditions.checkArgument(
+ view.getSql() != null,
+ "View %s.%s has no Trino dialect SQL representation",
+ view.getSchemaName(),
+ view.getName());
+ 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:
**[P2] Please preserve or explicitly reject the view path.** Trino stores
the session path in `ConnectorViewDefinition`, but `createView()` below ignores
`definition.getPath()` and this load path always returns `List.of()`. A view
that resolves unqualified functions through `SET PATH` can therefore fail or
bind differently after reload. Could we round-trip the path through Gravitino
metadata, or reject non-empty paths, and cover that behavior with a test?
##########
catalogs/catalog-hive/src/main/java/org/apache/gravitino/catalog/hive/HiveView.java:
##########
@@ -40,9 +40,15 @@
/**
* Represents a view stored in Hive Metastore (VIRTUAL_VIEW table type). The
SQL dialect is detected
- * from table properties: Trino views start with "/* Presto View:", Spark
views carry {@code
- * spark.sql.create.version}, Flink views carry properties prefixed with
{@code flink.}, and all
- * other views are treated as native Hive SQL views.
+ * from table properties: Trino views (written by this catalog) carry {@code
+ * gravitino.view.trino_dialect}, Spark views carry {@code
spark.sql.create.version}, Flink views
+ * carry properties prefixed with {@code flink.}, and all other views are
treated as native Hive SQL
+ * views.
+ *
+ * <p>Native Presto/Trino views created outside Gravitino use the {@code
presto_view} HMS property
+ * and encode their body as a base64-wrapped comment rather than plain SQL
text; since decoding that
+ * native format requires Trino's own serialization logic, this catalog does
not attempt to read it
+ * and such views are treated as Hive dialect (i.e. not exposed as
Trino-readable SQL).
Review Comment:
**[P3] Keep the documented behavior in sync.** The latest implementation
rejects native Presto/Trino views with `UnsupportedOperationException`; it no
longer treats them as Hive dialect. Could we update this sentence so the class
documentation matches `detectDialect()`?
--
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]
