mchades commented on code in PR #11926:
URL: https://github.com/apache/gravitino/pull/11926#discussion_r4279366505
##########
catalogs/catalog-hive/src/main/java/org/apache/gravitino/catalog/hive/HiveViewCatalogOperations.java:
##########
@@ -500,6 +514,9 @@ private HiveView toHiveView(
resolvedComment = decoded.comment;
restoredDefaultCatalog = decoded.catalog;
restoredDefaultSchema = decoded.schema;
+ if (decoded.owner != null) {
+ params.put(TRINO_VIEW_OWNER_PROPERTY, decoded.owner);
+ }
Review Comment:
**[P1] Use the HMS owner for native DEFINER views.** After native Trino runs
`ALTER VIEW v SET AUTHORIZATION USER bob`, HMS contains owner=bob while the
encoded payload can still contain alice. [Native Trino prioritizes the HMS
owner](https://github.com/trinodb/trino/blob/440/plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveMetadata.java#L2874),
but this code returns alice as the DEFINER. The connectors therefore disagree
on the execution identity. The reverse path also diverges: replacing an
alice-owned view as bob updates the payload to bob but leaves HMS owner=alice.
I reproduced both metadata mismatches on cda1ea5f using the current methods
with an in-memory HMS substitute. Please honor native ownership semantics on
load and replacement, with regression tests for ownership transfer and
replacement by a different owner.
##########
trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/catalog/CatalogConnectorMetadataAdapter.java:
##########
@@ -147,6 +164,123 @@ public GravitinoTable createTable(ConnectorTableMetadata
tableMetadata) {
return new GravitinoTable(schemaName, tableName, columns, comment,
properties);
}
+ /**
+ * Transform Gravitino view metadata to Trino ConnectorViewDefinition. The
view's {@code SECURITY
+ * DEFINER} owner, if any, is read back from the {@link
#RESERVED_VIEW_OWNER_PROPERTY} reserved
+ * property (set by {@link #createView}); its absence means the view is
{@code SECURITY INVOKER},
+ * matching Trino's own invariant that {@code runAsInvoker} and a present
owner are mutually
+ * exclusive.
+ *
+ * <p>{@link ConnectorViewDefinition} requires a catalog to be present
whenever a schema is
+ * present. Some catalogs (e.g. Iceberg) can store a default schema without
a default catalog; in
+ * single-metalake mode the current Trino catalog is used as a fallback,
since the schema is
+ * implicitly relative to it. In multi-metalake mode the bare Gravitino
catalog name is not the
+ * name Trino actually resolves catalogs by, so this fallback cannot be
applied and the view is
+ * rejected instead of being exposed with a wrong or unresolvable catalog.
+ *
+ * @param view the Gravitino view
+ * @param catalogName the name of the Trino catalog this view belongs to
+ * @param singleMetalakeMode whether the connector is running in
single-metalake mode
+ * @return the Trino ConnectorViewDefinition
+ */
+ public ConnectorViewDefinition getViewDefinition(
+ GravitinoView view, String catalogName, boolean singleMetalakeMode) {
+ 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());
+
+ String defaultCatalog = view.getDefaultCatalog();
+ if (defaultCatalog == null && view.getDefaultSchema() != null) {
+ if (!singleMetalakeMode) {
+ throw new TrinoException(
+ GravitinoErrorCode.GRAVITINO_UNSUPPORTED_OPERATION,
+ String.format(
+ "View %s.%s has a default schema without a default catalog,
which is not "
+ + "supported in multi-metalake mode",
+ view.getSchemaName(), view.getName()));
+ }
+ defaultCatalog = catalogName;
+ }
+
+ String ownerProperty =
view.getProperties().get(RESERVED_VIEW_OWNER_PROPERTY);
+ return new ConnectorViewDefinition(
+ view.getSql(),
+ Optional.ofNullable(defaultCatalog),
+ Optional.ofNullable(view.getDefaultSchema()),
+ columns,
+ Optional.ofNullable(view.getComment()),
+ Optional.ofNullable(ownerProperty),
+ ownerProperty == null,
+ List.of());
+ }
+
+ /**
+ * Transform Trino ConnectorViewDefinition to Gravitino view metadata. The
{@code viewProperties}
+ * are merged as-is into the resulting view's generic properties; the caller
cannot set {@link
+ * #RESERVED_VIEW_OWNER_PROPERTY} directly through them since it is reserved
to round-trip the
+ * definition's own owner/{@code runAsInvoker}.
+ *
+ * <p>Gravitino views have no field to persist the view's {@code path} (the
catalogs/schemas used
+ * to resolve unqualified function names, set via {@code SET PATH}), so a
definition with a
+ * non-empty path is rejected rather than silently discarding it; loading a
view therefore always
+ * returns an empty path, which is safe because no view with a non-empty
path is ever stored.
+ *
+ * @param viewName the schema-qualified view name
+ * @param definition the Trino ConnectorViewDefinition
+ * @param viewProperties the Trino view properties
+ * @return the Gravitino view metadata
+ */
+ public GravitinoView createView(
+ SchemaTableName viewName,
+ ConnectorViewDefinition definition,
+ Map<String, Object> viewProperties) {
+ if (!definition.getPath().isEmpty()) {
+ throw new TrinoException(
+ GravitinoErrorCode.GRAVITINO_UNSUPPORTED_OPERATION,
+ "View " + viewName + " has a non-empty path (SET PATH), which
Gravitino cannot persist");
+ }
+ TypeManager typeManager =
JsonCodec.getTypeManager(getClass().getClassLoader());
+ List<GravitinoColumn> columns = new ArrayList<>();
+ List<ViewColumn> viewColumns = definition.getColumns();
+ for (int i = 0; i < viewColumns.size(); i++) {
+ ViewColumn column = viewColumns.get(i);
+ Type trinoType = typeManager.getType(column.getType());
+ columns.add(
+ new GravitinoColumn(
+ column.getName(),
+ dataTypeTransformer.getGravitinoType(trinoType),
Review Comment:
**[P2] Still reproducible on cda1ea5f.** `CREATE VIEW v AS SELECT 'x' AS c`
now passes connector conversion, but the server's
`IcebergViewCatalogOperations.createView()` still rejects `VarCharType` through
`ConvertUtil.toIcebergSchema()`. I re-ran the current adapter-to-server call
chain; the failure occurs before persistence. Please complete the server-side
view type conversion and cover create/load through that boundary.
##########
trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/catalog/CatalogConnectorMetadataAdapter.java:
##########
@@ -148,6 +157,116 @@ 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}.
+ *
+ * <p>{@link ConnectorViewDefinition} requires a catalog to be present
whenever a schema is
+ * present. Some catalogs (e.g. Iceberg) can store a default schema without
a default catalog; in
+ * single-metalake mode the current Trino catalog is used as a fallback,
since the schema is
+ * implicitly relative to it. In multi-metalake mode the bare Gravitino
catalog name is not the
+ * name Trino actually resolves catalogs by, so this fallback cannot be
applied and the view is
+ * rejected instead of being exposed with a wrong or unresolvable catalog.
+ *
+ * @param view the Gravitino view
+ * @param catalogName the name of the Trino catalog this view belongs to
+ * @param singleMetalakeMode whether the connector is running in
single-metalake mode
+ * @return the Trino ConnectorViewDefinition
+ */
+ public ConnectorViewDefinition getViewDefinition(
+ GravitinoView view, String catalogName, boolean singleMetalakeMode) {
+ 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());
+
+ String defaultCatalog = view.getDefaultCatalog();
+ if (defaultCatalog == null && view.getDefaultSchema() != null) {
+ if (!singleMetalakeMode) {
+ throw new TrinoException(
+ GravitinoErrorCode.GRAVITINO_UNSUPPORTED_OPERATION,
+ String.format(
+ "View %s.%s has a default schema without a default catalog,
which is not "
+ + "supported in multi-metalake mode",
+ view.getSchemaName(), view.getName()));
+ }
+ defaultCatalog = catalogName;
+ }
+
+ return new ConnectorViewDefinition(
+ view.getSql(),
+ Optional.ofNullable(defaultCatalog),
+ Optional.ofNullable(view.getDefaultSchema()),
+ columns,
+ Optional.ofNullable(view.getComment()),
+ Optional.empty(),
Review Comment:
**[P2] Native SQL path loss still reproduces on cda1ea5f.** A native payload
containing one path entry still loads with an empty
`ConnectorViewDefinition.getPath()`. `toHiveView()` neither preserves nor
rejects `decoded.path`, so unqualified function resolution can change. The
create/replace guards do not cover this read path. Please preserve the path or
reject such views on load, with a native-payload regression test.
##########
trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/catalog/CatalogConnectorMetadata.java:
##########
@@ -454,4 +498,297 @@ public Function getFunction(String schemaName, String
functionName) {
}
return functionCatalog.getFunction(NameIdentifier.of(schemaName,
functionName));
}
+
+ /**
+ * Checks whether the catalog supports view operations.
+ *
+ * @return true if the catalog supports view operations, false otherwise
+ */
+ public boolean supportsViews() {
+ return viewCatalog != null;
+ }
+
+ /**
+ * Retrieves the Gravitino view for the specified name, if it exists and has
a Trino dialect SQL
+ * representation.
+ *
+ * @param schemaName the name of the schema
+ * @param viewName the name of the view
+ * @return an {@link Optional} containing the Gravitino view, or {@link
Optional#empty()} if the
+ * view does not exist or has no Trino dialect SQL representation
+ */
+ public Optional<GravitinoView> getViewIfPresent(String schemaName, String
viewName) {
+ if (!supportsViews()) {
+ return Optional.empty();
+ }
+ try {
+ View view = viewCatalog.loadView(NameIdentifier.of(schemaName,
viewName));
+ GravitinoView gravitinoView = new GravitinoView(schemaName, viewName,
view);
+ if (gravitinoView.getSql() == null) {
+ // The view exists but has no Trino dialect SQL representation, so it
is not visible to
+ // Trino.
+ LOG.debug(
+ "View {}.{} in catalog {} has no Trino dialect SQL representation,
hiding it from"
+ + " Trino",
+ schemaName,
+ viewName,
+ catalogName);
+ return Optional.empty();
+ }
+ return Optional.of(gravitinoView);
+ } catch (NoSuchViewException e) {
+ return Optional.empty();
+ } catch (UnsupportedOperationException e) {
+ LOG.debug(
+ "Catalog {} does not support loading view {}.{}", catalogName,
schemaName, viewName, e);
+ return Optional.empty();
+ }
+ }
+
+ /**
+ * Retrieves the Gravitino view for the specified name.
+ *
+ * @param schemaName the name of the schema
+ * @param viewName the name of the view
+ * @return the Gravitino view
+ * @throws TrinoException if the view does not exist or has no Trino dialect
SQL representation
+ */
+ public GravitinoView getView(String schemaName, String viewName) {
+ return getViewIfPresent(schemaName, viewName)
+ .orElseThrow(
+ () ->
+ new TrinoException(
+ GravitinoErrorCode.GRAVITINO_VIEW_NOT_EXISTS, "View does
not exist"));
+ }
+
+ /**
+ * Lists the names of all views in the specified schema.
+ *
+ * @param schemaName the name of the schema
+ * @return a list of view names, or an empty list if the catalog does not
support views
+ */
+ public List<String> listViews(String schemaName) {
+ if (!supportsViews()) {
+ return List.of();
+ }
+ try {
+ NameIdentifier[] views = viewCatalog.listViews(Namespace.of(schemaName));
+ return Arrays.stream(views)
+ .map(NameIdentifier::name)
+ .filter(viewName -> getViewIfPresent(schemaName,
viewName).isPresent())
+ .toList();
+ } catch (UnsupportedOperationException e) {
+ LOG.debug(
+ "Catalog {} does not support listing views for schema {}",
catalogName, schemaName, e);
+ return List.of();
+ } catch (NoSuchSchemaException e) {
+ throw new TrinoException(
+ GravitinoErrorCode.GRAVITINO_SCHEMA_NOT_EXISTS,
SCHEMA_DOES_NOT_EXIST_MSG, e);
+ }
+ }
+
+ /**
+ * Creates or replaces a view in the catalog.
+ *
+ * <p>Only views with a Trino dialect SQL representation are considered
visible to Trino; if an
+ * entity with the same name already exists but has no Trino representation
(e.g. a view created
+ * by another engine), it is never silently replaced.
+ *
+ * @param view the Gravitino view, with the Trino dialect SQL definition set
+ * @param replace whether to replace the view if it already exists
+ */
+ public void createView(GravitinoView view, boolean replace) {
+ if (!supportsViews()) {
+ throw new TrinoException(
+ GravitinoErrorCode.GRAVITINO_UNSUPPORTED_OPERATION, "Catalog does
not support views");
+ }
+ Preconditions.checkArgument(
+ view.getSql() != null,
+ "View %s.%s has no Trino dialect SQL representation",
+ view.getSchemaName(),
+ view.getName());
+ NameIdentifier identifier = NameIdentifier.of(view.getSchemaName(),
view.getName());
+ SQLRepresentation[] representations = {
+
SQLRepresentation.builder().withDialect(Dialects.TRINO).withSql(view.getSql()).build()
+ };
+ try {
+ boolean exists = viewCatalog.viewExists(identifier);
+ if (exists) {
+ View existingView = viewCatalog.loadView(identifier);
+ if (!existingView.sqlFor(Dialects.TRINO).isPresent()) {
+ // An entity with this name already exists but is not visible to
Trino (e.g. a view
+ // created by another engine), so it must not be treated as
replaceable.
+ throw new TrinoException(
+ GravitinoErrorCode.GRAVITINO_VIEW_ALREADY_EXISTS, "View already
exists");
+ }
+ if (!replace) {
+ throw new TrinoException(
+ GravitinoErrorCode.GRAVITINO_VIEW_ALREADY_EXISTS, "View already
exists");
+ }
+ if (hasNonTrinoRepresentation(existingView.representations())) {
+ // Trino cannot regenerate SQL for other engines' dialects, so
replacing here would
+ // leave those representations referring to a schema/body that no
longer matches.
+ throw new TrinoException(
+ GravitinoErrorCode.GRAVITINO_UNSUPPORTED_OPERATION,
+ "Cannot replace view "
+ + view.getSchemaName()
+ + "."
+ + view.getName()
+ + " because it has SQL representations in other dialects");
+ }
+ List<ViewChange> changes = new ArrayList<>();
+ changes.add(
+ ViewChange.replaceView(
+ view.getRawColumns(),
+ representations,
+ view.getDefaultCatalog(),
+ view.getDefaultSchema(),
+ view.getComment()));
+ changes.addAll(computePropertyChanges(existingView.properties(),
view.getProperties()));
Review Comment:
**[P2] View-property registration remains empty on cda1ea5f.**
`HiveConnectorAdapter.getViewProperties()` still returns `[]`, so `WITH
(extra_properties = MAP(ARRAY['k'], ARRAY['v']))` fails Trino property
validation before reaching `createView()`. Please register/delegate the
supported view-property metadata and cover this with an SQL-level regression
test.
--
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]
