mchades commented on code in PR #11926:
URL: https://github.com/apache/gravitino/pull/11926#discussion_r4254110376
##########
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] The Iceberg type fix still fails at the server boundary on
cc06630e.** The connector now accepts `varchar(1)`, but
`IcebergViewCatalogOperations.createView()` passes those columns to
`ConvertUtil.toIcebergSchema()`, whose `ToIcebergType` rejects `VarCharType`. I
reproduced this by passing the current adapter output to the server's
`createView()` method; it throws before persistence. Thus `CREATE VIEW v AS
SELECT 'x' AS c` still fails, while the new test stops at adapter conversion.
Please complete the Iceberg-side conversion and add a create/load regression
through the server. The Hive timestamp-with-time-zone and unbounded varchar
round trips now pass.
##########
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 cc06630e.** A native view
payload with one path entry is returned as a `ConnectorViewDefinition` with an
empty path. `toHiveView()` still neither preserves nor rejects `decoded.path`,
and `getViewDefinition()` returns `List.of()`. This can change unqualified
function resolution; the create/replace guards do not protect loading native
views.
Please preserve the path or reject these 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 is still empty on cc06630e.** I rechecked
`HiveConnectorAdapter.getViewProperties()` and it returns `[]`. The shims
delegate through `CatalogConnectorContext` to the unchanged default method,
with no concrete adapter supplying metadata. Consequently, `WITH
(extra_properties = MAP(ARRAY['k'], ARRAY['v']))` is rejected by Trino property
validation before reaching `createView()`.
Please expose the supported view-property metadata and add 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]
