mchades commented on code in PR #11926:
URL: https://github.com/apache/gravitino/pull/11926#discussion_r4123306602
##########
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] The new overrides still expose an empty property list on f0fada93.**
`CatalogConnectorContext.getViewProperties()` delegates to
`adapter.getViewProperties()`, but no concrete adapter overrides the new
default method, which returns `emptyList()`. I verified that
`HiveConnectorAdapter.getViewProperties()` returns `[]`, so a non-empty `WITH
(...)` clause still fails Trino property validation.
Please delegate/register the actual view-property metadata and add an
SQL-level test using Hive's `extra_properties`; the 445 shim also remains
unwired.
##########
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:
Rechecked on f0fada93: this conversion is unchanged, and both failures still
reproduce with the current adapter methods. Iceberg rejects `varchar(1)`; Hive
accepts `timestamp(3) with time zone` during creation but rejects it when
loading the definition. Please keep this issue open until view-specific type
conversion and regression coverage are added.
##########
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 views with a non-empty SQL path still lose that context on
load.** On f0fada93, `toHiveView()` does not preserve or reject `decoded.path`,
and `getViewDefinition()` always returns `List.of()`. I reproduced a native
payload with one path entry being loaded with an empty path. A view using that
path to resolve an unqualified function can fail or bind differently.
The create/replace guards do not protect this read path. Please preserve the
native path or explicitly reject such views during loading, and add a
native-payload 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]
