Copilot commented on code in PR #11926:
URL: https://github.com/apache/gravitino/pull/11926#discussion_r3600006887
##########
catalogs/catalog-hive/src/main/java/org/apache/gravitino/catalog/hive/HiveViewCatalogOperations.java:
##########
@@ -121,6 +123,9 @@ public View createView(
try {
Map<String, String> params = Maps.newHashMap(safeProperties);
params.put(TABLE_TYPE, TableType.VIRTUAL_VIEW.name());
+ if (Dialects.TRINO.equalsIgnoreCase(sqlRepresentation.dialect())) {
+ params.put(HiveView.TRINO_VIEW_MARKER_KEY, "true");
+ }
Review Comment:
`presto_view` is used by `HiveView.detectDialect(...)` as a Trino marker. In
`createView`, the marker is only added for Trino dialect but never removed if
it was present in the incoming `properties`, which can cause non-Trino views to
be mis-detected as Trino on reload.
##########
catalogs/catalog-hive/src/main/java/org/apache/gravitino/catalog/hive/HiveViewCatalogOperations.java:
##########
@@ -219,6 +224,9 @@ public View alterView(NameIdentifier ident, ViewChange...
changes)
updatedColumns = copyColumns(replace.getColumns());
updatedComment = replace.getComment();
updatedViewOriginalText = toHmsViewOriginalText(sqlRepresentation,
ident);
+ if (Dialects.TRINO.equalsIgnoreCase(sqlRepresentation.dialect())) {
+ updatedProperties.put(HiveView.TRINO_VIEW_MARKER_KEY, "true");
+ }
Review Comment:
When replacing a view with a non-Trino dialect, the existing `presto_view`
marker (if any) is currently preserved. Since dialect detection checks this
marker first, the updated view can be mis-classified as Trino after a replace.
Clear the marker when the replacement dialect is not Trino.
##########
trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/catalog/CatalogConnectorMetadata.java:
##########
@@ -454,4 +473,258 @@ 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");
+ }
+ NameIdentifier identifier = NameIdentifier.of(view.getSchemaName(),
view.getName());
+ SQLRepresentation[] representations = {
+
SQLRepresentation.builder().withDialect(Dialects.TRINO).withSql(view.getSql()).build()
+ };
Review Comment:
`GravitinoView#getSql()` is nullable, but `createView` unconditionally
builds a `SQLRepresentation` with it. If a caller accidentally passes a view
without a Trino SQL representation, this will either NPE or persist an invalid
view body. Validate `sql` early and return a clear error.
--
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]
