mchades commented on code in PR #11926:
URL: https://github.com/apache/gravitino/pull/11926#discussion_r4282910795
##########
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] Alteration still restores a stale owner on 88266eb6.** Loading now
honors HMS owner=bob, but `alterView()` seeds the owner property from the stale
payload owner=alice. Replacing the view as bob emits no owner update because
the loaded owner is already bob; the replacement then encodes alice and
overwrites HMS ownership with alice. A rename alone also changes HMS owner from
bob back to alice.
Both paths reproduce through the current connector/Hive methods with an
in-memory HMS substitute. Please initialize alteration from the effective HMS
owner and preserve it unless explicitly changed, with ownership-transfer
followed by replace/rename regression tests.
##########
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] Nested varchar still fails on 88266eb6.** Scalar varchar
create/load/replace now passes against Iceberg's `InMemoryCatalog`. However,
`widenVarcharColumns()` only checks the top-level column type: `CREATE VIEW v
AS SELECT ARRAY['x'] AS c` still reaches `ToIcebergType` with a nested
`VarCharType` and throws `UnsupportedOperationException`. Map keys/values and
named row fields have the same problem; both create and replace fail before
persistence.
Please recursively normalize varchar inside list/map/struct types and cover
these nested cases through the server boundary.
##########
trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/catalog/CatalogConnectorMetadataAdapter.java:
##########
@@ -147,6 +179,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(),
+
VIEW_DATA_TYPE_TRANSFORMER.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(),
+ VIEW_DATA_TYPE_TRANSFORMER.getGravitinoType(trinoType),
Review Comment:
**[P2] Handle anonymous row fields in view outputs.** `CREATE VIEW v AS
SELECT ROW(1, 'x') AS r` produces a row whose nested fields have no names. This
conversion calls `GeneralDataTypeTransformer.trinoRowTypeToGravitinoRowType()`,
which unconditionally evaluates `field.getName().get()` and throws
`NoSuchElementException`. I reproduced this with both Hive and Iceberg adapters
on 88266eb6, before persistence.
Please handle anonymous row fields and add a create/load regression for this
query.
--
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]
