mchades commented on code in PR #11189:
URL: https://github.com/apache/gravitino/pull/11189#discussion_r3301076698
##########
catalogs/catalog-hive/src/main/java/org/apache/gravitino/catalog/hive/HiveViewCatalogOperations.java:
##########
@@ -405,34 +406,37 @@ private SQLRepresentation validateSQLRepresentation(
firstRepresentation == null ? "null" :
firstRepresentation.getClass().getSimpleName());
SQLRepresentation selected = (SQLRepresentation) firstRepresentation;
- boolean isHiveDialect = Dialects.HIVE.equalsIgnoreCase(selected.dialect());
- if (isHiveDialect) {
- Preconditions.checkArgument(
- defaultCatalog == null && defaultSchema == null,
- "Hive dialect '%s' does not support non-null
defaultCatalog/defaultSchema, but got "
- + "defaultCatalog=%s, defaultSchema=%s for view %s",
- Dialects.HIVE,
- defaultCatalog,
- defaultSchema,
- ident);
- return selected;
+ switch (selected.dialect().toLowerCase(java.util.Locale.ROOT)) {
+ case Dialects.HIVE:
+ case Dialects.FLINK:
+ Preconditions.checkArgument(
+ defaultCatalog == null && defaultSchema == null,
+ "Dialect '%s' does not support non-null
defaultCatalog/defaultSchema, but got "
+ + "defaultCatalog=%s, defaultSchema=%s for view %s",
+ selected.dialect(),
Review Comment:
Flink does not support defaultCatalog/defaultSchema too?
##########
flink-connector/flink-common/src/main/java/org/apache/gravitino/flink/connector/catalog/BaseCatalog.java:
##########
@@ -210,32 +225,70 @@ public List<String> listTables(String databaseName)
@Override
public List<String> listViews(String databaseName)
throws DatabaseNotExistException, CatalogException {
- // Gravitino does not support views yet; return empty to keep Flink
callers happy.
- return Collections.emptyList();
+ try {
+ ViewCatalog viewCatalog = catalog().asViewCatalog();
+ // TODO: Currently returns all VIRTUAL_VIEW entries from the underlying
catalog regardless of
+ // dialect. Views created by other engines (e.g. Trino, Spark) may
appear here but will fail
+ // when Flink attempts to load them. Consider filtering to only dialects
that Flink can handle
+ // (hive, flink), but this requires per-view property inspection which
is expensive.
+ return Arrays.stream(viewCatalog.listViews(Namespace.of(databaseName)))
+ .map(NameIdentifier::name)
+ .collect(Collectors.toList());
+ } catch (UnsupportedOperationException e) {
+ LOG.debug("Catalog {} does not support views; returning empty view
list", catalogName(), e);
+ return Collections.emptyList();
Review Comment:
If the catalog does not support the view operation, it should explicitly
return an error rather than an empty list, to avoid misleading users.
##########
flink-connector/flink-common/src/main/java/org/apache/gravitino/flink/connector/catalog/BaseCatalog.java:
##########
@@ -244,12 +297,36 @@ public boolean tableExists(ObjectPath tablePath) throws
CatalogException {
@Override
public void dropTable(ObjectPath tablePath, boolean ignoreIfNotExists)
throws TableNotExistException, CatalogException {
- boolean dropped =
- catalog()
- .asTableCatalog()
- .dropTable(NameIdentifier.of(tablePath.getDatabaseName(),
tablePath.getObjectName()));
- if (!dropped && !ignoreIfNotExists) {
- throw new TableNotExistException(catalogName(), tablePath);
+ NameIdentifier ident =
+ NameIdentifier.of(tablePath.getDatabaseName(),
tablePath.getObjectName());
+
+ // Determine kind before dropping to avoid a view being silently missed.
+ CatalogBaseTable existing;
+ try {
+ existing = getTable(tablePath);
+ } catch (TableNotExistException e) {
+ if (!ignoreIfNotExists) {
+ throw e;
+ }
+ return;
+ }
Review Comment:
The code block seems unnecessary? And the logic below can be updated to
```
try {
tableExist = dropTable(ident)
viewExist = dropView(ident)
}
...
##########
flink-connector/flink-common/src/main/java/org/apache/gravitino/flink/connector/catalog/BaseCatalog.java:
##########
@@ -261,16 +338,36 @@ public void renameTable(ObjectPath tablePath, String
newTableName, boolean ignor
if (catalog().asTableCatalog().tableExists(identifier)) {
throw new TableAlreadyExistException(
- catalogName(), ObjectPath.fromString(tablePath.getDatabaseName() +
newTableName));
+ catalogName(), new ObjectPath(tablePath.getDatabaseName(),
newTableName));
+ }
+ try {
+ if (catalog().asViewCatalog().viewExists(identifier)) {
+ throw new TableAlreadyExistException(
+ catalogName(), new ObjectPath(tablePath.getDatabaseName(),
newTableName));
+ }
+ } catch (UnsupportedOperationException ignored) {
+ // catalog does not support views
Review Comment:
should throw explicitly?
##########
flink-connector/flink-common/src/main/java/org/apache/gravitino/flink/connector/catalog/BaseCatalog.java:
##########
@@ -323,6 +421,35 @@ public void createTable(ObjectPath tablePath,
CatalogBaseTable table, boolean ig
}
}
+ private void createView(ObjectPath tablePath, ResolvedCatalogView view,
boolean ignoreIfExists)
+ throws TableAlreadyExistException, DatabaseNotExistException,
CatalogException {
+ NameIdentifier identifier =
+ NameIdentifier.of(tablePath.getDatabaseName(),
tablePath.getObjectName());
+ Column[] columns = toGravitinoColumns(view);
+ Representation[] representations =
+ buildSqlRepresentation(preferredDialect(), view.getExpandedQuery());
Review Comment:
So the `preferredDialect()` method here is unnecessary?
##########
flink-connector/flink-common/src/main/java/org/apache/gravitino/flink/connector/catalog/BaseCatalog.java:
##########
@@ -261,16 +338,36 @@ public void renameTable(ObjectPath tablePath, String
newTableName, boolean ignor
if (catalog().asTableCatalog().tableExists(identifier)) {
throw new TableAlreadyExistException(
- catalogName(), ObjectPath.fromString(tablePath.getDatabaseName() +
newTableName));
+ catalogName(), new ObjectPath(tablePath.getDatabaseName(),
newTableName));
+ }
+ try {
+ if (catalog().asViewCatalog().viewExists(identifier)) {
+ throw new TableAlreadyExistException(
+ catalogName(), new ObjectPath(tablePath.getDatabaseName(),
newTableName));
+ }
+ } catch (UnsupportedOperationException ignored) {
+ // catalog does not support views
}
+ CatalogBaseTable existing;
try {
- catalog()
- .asTableCatalog()
- .alterTable(
- NameIdentifier.of(tablePath.getDatabaseName(),
tablePath.getObjectName()),
- TableChange.rename(newTableName));
- } catch (NoSuchTableException e) {
+ existing = getTable(tablePath);
Review Comment:
Here calls getTable, so the above table/view existing check is unnecessary?
--
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]
