Copilot commented on code in PR #10924:
URL: https://github.com/apache/gravitino/pull/10924#discussion_r3167205416
##########
core/src/main/java/org/apache/gravitino/catalog/ViewOperationDispatcher.java:
##########
@@ -66,149 +73,498 @@ public ViewOperationDispatcher(
}
/**
- * Load view metadata by identifier from the catalog.
+ * Lists the views within a schema.
*
- * <p>This method first checks if the view exists in Gravitino's
EntityStore. If not found, it
- * loads from the catalog and auto-imports into EntityStore.
+ * @param namespace The namespace of the schema containing the views.
+ * @return An array of {@link NameIdentifier} objects representing the
identifiers of the views in
+ * the schema.
+ * @throws NoSuchSchemaException If the specified schema does not exist.
+ */
+ @Override
+ public NameIdentifier[] listViews(Namespace namespace) throws
NoSuchSchemaException {
+ return TreeLockUtils.doWithTreeLock(
+ NameIdentifier.of(namespace.levels()),
+ LockType.READ,
+ () ->
+ doWithCatalog(
+ getCatalogIdentifier(NameIdentifier.of(namespace.levels())),
+ c -> c.doWithViewOps(v -> v.listViews(namespace)),
+ NoSuchSchemaException.class));
+ }
+
+ /**
+ * Loads a view.
*
- * @param ident The view identifier.
- * @return The loaded view metadata.
- * @throws NoSuchViewException If the view does not exist.
+ * @param ident The identifier of the view to load.
+ * @return The loaded {@link View} object representing the requested view.
+ * @throws NoSuchViewException If the specified view does not exist.
*/
@Override
public View loadView(NameIdentifier ident) throws NoSuchViewException {
LOG.info("Loading view: {}", ident);
- // First load with READ lock to check if view is already imported
EntityCombinedView entityCombinedView =
TreeLockUtils.doWithTreeLock(ident, LockType.READ, () ->
internalLoadView(ident));
if (!entityCombinedView.imported()) {
- // Load the schema to make sure the schema is imported.
SchemaDispatcher schemaDispatcher =
GravitinoEnv.getInstance().schemaDispatcher();
NameIdentifier schemaIdent =
NameIdentifier.of(ident.namespace().levels());
schemaDispatcher.loadSchema(schemaIdent);
- // Import the view.
entityCombinedView =
TreeLockUtils.doWithTreeLock(schemaIdent, LockType.WRITE, () ->
importView(ident));
}
return entityCombinedView;
}
+ /**
+ * Creates a new view in a schema.
+ *
+ * @param ident The identifier of the view to create.
+ * @param comment A description or comment associated with the view.
+ * @param columns An array of {@link Column} objects representing the output
columns of the view.
+ * @param representations An array of {@link Representation} objects
representing the SQL
+ * definitions of the view across different dialects.
+ * @param defaultCatalog The default catalog to use for unqualified
references.
+ * @param defaultSchema The default schema to use for unqualified references.
+ * @param properties Additional properties to set for the view.
+ * @return The newly created {@link View} object.
+ * @throws NoSuchSchemaException If the schema in which to create the view
does not exist.
+ * @throws ViewAlreadyExistsException If a view with the same name already
exists in the schema.
+ */
+ @Override
+ public View createView(
+ NameIdentifier ident,
+ String comment,
+ Column[] columns,
+ Representation[] representations,
+ @Nullable String defaultCatalog,
+ @Nullable String defaultSchema,
+ Map<String, String> properties)
+ throws NoSuchSchemaException, ViewAlreadyExistsException {
+ Preconditions.checkArgument(
+ representations != null && representations.length >= 1,
+ "representations must not be null or empty");
+
+ // Load the schema to make sure the schema exists.
+ SchemaDispatcher schemaDispatcher =
GravitinoEnv.getInstance().schemaDispatcher();
+ NameIdentifier schemaIdent = NameIdentifier.of(ident.namespace().levels());
+ schemaDispatcher.loadSchema(schemaIdent);
+
+ return TreeLockUtils.doWithTreeLock(
+ schemaIdent,
+ LockType.WRITE,
+ () ->
+ internalCreateView(
+ ident,
+ comment,
+ columns,
+ representations,
+ defaultCatalog,
+ defaultSchema,
+ properties));
+ }
+
+ /**
+ * Alters an existing view.
+ *
+ * @param ident The identifier of the view to alter.
+ * @param changes An array of {@link ViewChange} objects representing the
changes to apply to the
+ * view.
+ * @return The altered {@link View} object after applying the changes.
+ * @throws NoSuchViewException If the view to alter does not exist.
+ * @throws IllegalArgumentException If an unsupported or invalid change is
specified.
+ */
@Override
- public NameIdentifier[] listViews(Namespace namespace) {
- throw new UnsupportedOperationException("Listing views is not supported
yet");
+ public View alterView(NameIdentifier ident, ViewChange... changes)
+ throws NoSuchViewException, IllegalArgumentException {
+ validateAlterProperties(ident,
HasPropertyMetadata::tablePropertiesMetadata, changes);
+ NameIdentifier lockIdent = ident;
+ for (ViewChange change : changes) {
+ if (change instanceof ViewChange.RenameView) {
+ lockIdent = getSchemaIdentifier(ident);
+ break;
+ }
+ }
+
+ NameIdentifier nameIdentifierForLock = lockIdent;
+ return TreeLockUtils.doWithTreeLock(
+ nameIdentifierForLock,
+ nameIdentifierForLock.equals(ident) ? LockType.READ : LockType.WRITE,
+ () -> {
+ NameIdentifier catalogIdent = getCatalogIdentifier(ident);
+ View alteredView =
+ doWithCatalog(
+ catalogIdent,
+ c ->
+ c.doWithViewOps(
+ v -> v.alterView(ident,
applyCapabilities(c.capabilities(), changes))),
+ NoSuchViewException.class,
+ IllegalArgumentException.class);
+
+ boolean isManagedView = isManagedEntity(catalogIdent,
Capability.Scope.VIEW);
+ if (isManagedView) {
+ return EntityCombinedView.of(alteredView)
+ .withHiddenProperties(
+ getHiddenPropertyNames(
+ catalogIdent,
+ HasPropertyMetadata::tablePropertiesMetadata,
+ alteredView.properties()));
+ }
+
+ ViewEntity existing = getEntity(ident, VIEW, ViewEntity.class);
+ if (existing == null) {
+ return EntityCombinedView.of(alteredView)
+ .withHiddenProperties(
+ getHiddenPropertyNames(
+ catalogIdent,
+ HasPropertyMetadata::tablePropertiesMetadata,
+ alteredView.properties()));
+ }
+
+ long viewId = existing.id();
+ ViewEntity updatedViewEntity =
+ operateOnEntity(
+ ident,
+ id ->
+ store.update(
+ id,
+ ViewEntity.class,
+ VIEW,
+ viewEntity -> applyChangesToEntity(viewEntity,
alteredView, changes)),
+ "UPDATE",
+ viewId);
+
+ return EntityCombinedView.of(alteredView, updatedViewEntity)
+ .withHiddenProperties(
+ getHiddenPropertyNames(
+ catalogIdent,
+ HasPropertyMetadata::tablePropertiesMetadata,
+ alteredView.properties()));
+ });
}
+ /**
+ * Drops a view from the catalog.
+ *
+ * @param ident The identifier of the view to drop.
+ * @return {@code true} if the view was successfully dropped, {@code false}
if the view does not
+ * exist.
+ * @throws RuntimeException If an error occurs while dropping the view.
+ */
@Override
public boolean dropView(NameIdentifier ident) {
- throw new UnsupportedOperationException("Dropping a view is not supported
yet");
+ NameIdentifier schemaIdentifier = getSchemaIdentifier(ident);
+ return TreeLockUtils.doWithTreeLock(
+ schemaIdentifier,
+ LockType.WRITE,
+ () -> {
+ NameIdentifier catalogIdent = getCatalogIdentifier(ident);
+ boolean droppedFromCatalog =
+ doWithCatalog(
+ catalogIdent,
+ c -> c.doWithViewOps(v -> v.dropView(ident)),
+ RuntimeException.class);
+
+ boolean isManagedView = isManagedEntity(catalogIdent,
Capability.Scope.VIEW);
+ if (isManagedView) {
+ return droppedFromCatalog;
+ }
+
+ // For unmanaged view, it could happen that the view:
+ // 1. Is not found in the catalog (dropped directly from underlying
sources)
+ // 2. Is found in the catalog but not in the store (not managed by
Gravitino)
+ // 3. Is found in the catalog and the store (managed by Gravitino)
+ // 4. Neither found in the catalog nor in the store.
+ // In all situations, we try to delete the view from the store, but
we don't take the
+ // return value of the store operation into account. We only take
the return value of the
+ // catalog into account.
+ try {
+ store.delete(ident, VIEW);
+ } catch (NoSuchEntityException e) {
+ LOG.warn("The view to be dropped does not exist in the store: {}",
ident, e);
+ } catch (Exception e) {
+ throw new RuntimeException(e);
+ }
+ return droppedFromCatalog;
+ });
}
- @Override
- public View createView(
+ private View internalCreateView(
NameIdentifier ident,
String comment,
Column[] columns,
Representation[] representations,
@Nullable String defaultCatalog,
@Nullable String defaultSchema,
Map<String, String> properties) {
- throw new UnsupportedOperationException("Creating a view is not supported
yet");
- }
+ NameIdentifier catalogIdent = getCatalogIdentifier(ident);
- @Override
- public View alterView(NameIdentifier ident, ViewChange... changes) {
- throw new UnsupportedOperationException("Altering a view is not supported
yet");
+ doWithCatalog(
+ catalogIdent,
+ c ->
+ c.doWithPropertiesMeta(
+ p -> {
+ validatePropertyForCreate(p.tablePropertiesMetadata(),
properties);
+ return null;
+ }),
+ IllegalArgumentException.class);
+
+ long uid = idGenerator.nextId();
+ // Add StringIdentifier to the properties, the specific catalog will
handle this
+ // StringIdentifier to make sure only when the operation is successful,
the related
+ // ViewEntity will be visible.
+ StringIdentifier stringId = StringIdentifier.fromId(uid);
+ Map<String, String> updatedProperties =
+ StringIdentifier.newPropertiesWithId(stringId, properties);
+
+ View catalogView =
+ doWithCatalog(
+ catalogIdent,
+ c ->
+ c.doWithViewOps(
+ v ->
+ v.createView(
+ ident,
+ comment,
+ columns == null ? new Column[0] : columns,
+ representations,
+ defaultCatalog,
+ defaultSchema,
+ updatedProperties)),
+ NoSuchSchemaException.class,
+ ViewAlreadyExistsException.class);
+
+ // If the view is managed by Gravitino, we don't need to create ViewEntity
and store it again.
+ boolean isManagedView = isManagedEntity(catalogIdent,
Capability.Scope.VIEW);
+ if (isManagedView) {
+ return EntityCombinedView.of(catalogView)
+ .withHiddenProperties(
+ getHiddenPropertyNames(
+ catalogIdent,
+ HasPropertyMetadata::tablePropertiesMetadata,
+ catalogView.properties()));
+ }
+
+ AuditInfo audit =
+ AuditInfo.builder()
+ .withCreator(PrincipalUtils.getCurrentPrincipal().getName())
+ .withCreateTime(Instant.now())
+ .build();
+
+ ViewEntity viewEntity =
+ ViewEntity.builder()
+ .withId(uid)
+ .withName(ident.name())
+ .withNamespace(ident.namespace())
+ .withComment(comment)
+ .withColumns(columns == null ? new Column[0] : columns)
+ .withRepresentations(representations)
+ .withDefaultCatalog(defaultCatalog)
+ .withDefaultSchema(defaultSchema)
+ .withProperties(properties)
+ .withAuditInfo(audit)
+ .build();
+
+ try {
+ store.put(viewEntity, true /* overwrite */);
+ } catch (Exception e) {
+ LOG.error(FormattedErrorMessages.STORE_OP_FAILURE, "put", ident, e);
+ return EntityCombinedView.of(catalogView)
+ .withHiddenProperties(
+ getHiddenPropertyNames(
+ catalogIdent,
+ HasPropertyMetadata::tablePropertiesMetadata,
+ catalogView.properties()));
+ }
+
+ // Merge both the metadata from catalog operation and the metadata from
entity store.
+ return EntityCombinedView.of(catalogView, viewEntity)
+ .withHiddenProperties(
+ getHiddenPropertyNames(
+ catalogIdent,
+ HasPropertyMetadata::tablePropertiesMetadata,
+ catalogView.properties()));
}
- /**
- * Internal method to load view and check if it exists in entity store.
- *
- * @param ident The view identifier.
- * @return EntityCombinedView containing the view and import status.
- * @throws NoSuchViewException If the view does not exist.
- */
private EntityCombinedView internalLoadView(NameIdentifier ident) throws
NoSuchViewException {
- // Load view from the underlying catalog
- View catalogView =
+ NameIdentifier catalogIdentifier = getCatalogIdentifier(ident);
+ View view =
doWithCatalog(
- getCatalogIdentifier(ident),
+ catalogIdentifier,
c -> c.doWithViewOps(v -> v.loadView(ident)),
NoSuchViewException.class);
- // Check if view exists in entity store
- try {
- ViewEntity viewEntity = store.get(ident, Entity.EntityType.VIEW,
ViewEntity.class);
- return EntityCombinedView.of(catalogView, viewEntity).withImported(true);
- } catch (NoSuchEntityException e) {
- // View not in store yet
- LOG.debug("View {} not found in entity store", ident);
- return EntityCombinedView.of(catalogView).withImported(false);
- } catch (IOException ioe) {
- LOG.warn("Failed to check if view {} exists in entity store", ident,
ioe);
- return EntityCombinedView.of(catalogView).withImported(false);
+ boolean isManagedView = isManagedEntity(catalogIdentifier,
Capability.Scope.VIEW);
+ if (isManagedView) {
+ return EntityCombinedView.of(view)
+ .withHiddenProperties(
+ getHiddenPropertyNames(
+ catalogIdentifier,
+ HasPropertyMetadata::tablePropertiesMetadata,
+ view.properties()))
+ // The metadata of managed view is stored by Gravitino, so it is
always imported.
+ .withImported(true /* imported */);
+ }
+
+ StringIdentifier stringId = getStringIdFromProperties(view.properties());
+ // Case 1: The view is not created by Gravitino or the external system
does not support storing
+ // string identifier.
+ if (stringId == null) {
+ ViewEntity viewEntity = getEntity(ident, VIEW, ViewEntity.class);
+ if (viewEntity == null) {
+ return EntityCombinedView.of(view)
+ .withHiddenProperties(
+ getHiddenPropertyNames(
+ catalogIdentifier,
+ HasPropertyMetadata::tablePropertiesMetadata,
+ view.properties()))
+ // Some views don't have properties or are not created by
Gravitino,
+ // we can't use stringIdentifier to judge whether view is ever
imported or not.
+ // We need to check whether the entity exists.
+ .withImported(false);
+ }
+
+ return EntityCombinedView.of(view, viewEntity)
+ .withHiddenProperties(
+ getHiddenPropertyNames(
+ catalogIdentifier,
+ HasPropertyMetadata::tablePropertiesMetadata,
+ view.properties()))
+ // For some catalogs, the identifier information is not stored in
the view's
+ // metadata, we need to check if this view exists in the store, if
so we don't
+ // need to import.
+ .withImported(true);
}
+
+ ViewEntity viewEntity =
+ operateOnEntity(
+ ident,
+ identifier -> store.get(identifier, VIEW, ViewEntity.class),
+ "GET",
+ stringId.id());
+
+ return EntityCombinedView.of(view, viewEntity)
+ .withHiddenProperties(
+ getHiddenPropertyNames(
+ catalogIdentifier,
HasPropertyMetadata::tablePropertiesMetadata, view.properties()))
+ .withImported(viewEntity != null);
}
- /**
- * Import view into Gravitino entity store.
- *
- * @param ident The view identifier.
- * @return EntityCombinedView containing the view and import status.
- * @throws NoSuchViewException If the view does not exist.
- */
private EntityCombinedView importView(NameIdentifier ident) throws
NoSuchViewException {
- // Double-check if already imported (another thread might have imported
between locks)
EntityCombinedView entityCombinedView = internalLoadView(ident);
if (entityCombinedView.imported()) {
return entityCombinedView;
}
- LOG.info("Auto-importing view {} into Gravitino entity store", ident);
- long uid = idGenerator.nextId();
+ StringIdentifier stringId =
+
getStringIdFromProperties(entityCombinedView.viewFromCatalog().properties());
+
+ long uid;
+ if (stringId != null) {
+ // If the entity in the store doesn't match the external system, we use
the data
+ // of external system to correct it.
+ LOG.warn(
+ "The View uid {} existed but still need to be imported, this could
be happened "
+ + "when View is renamed by external systems not controlled by
Gravitino. In this "
+ + "case, we need to overwrite the stored entity to keep the
consistency.",
+ stringId);
+ uid = stringId.id();
+ } else {
+ // If entity doesn't exist, we import the entity from the external
system.
+ uid = idGenerator.nextId();
+ }
+
View catalogView = entityCombinedView.viewFromCatalog();
- ViewEntity newViewEntity = buildViewEntityForImport(uid, ident,
catalogView);
+ AuditInfo audit =
+ AuditInfo.builder()
+ .withCreator(catalogView.auditInfo().creator())
+ .withCreateTime(catalogView.auditInfo().createTime())
+ .withLastModifier(catalogView.auditInfo().lastModifier())
+ .withLastModifiedTime(catalogView.auditInfo().lastModifiedTime())
+ .build();
+ ViewEntity viewEntity =
+ ViewEntity.builder()
+ .withId(uid)
+ .withName(ident.name())
+ .withNamespace(ident.namespace())
+ .withComment(catalogView.comment())
+ .withColumns(catalogView.columns() == null ? new Column[0] :
catalogView.columns())
+ .withRepresentations(
+ catalogView.representations() == null
+ ? new Representation[0]
+ : catalogView.representations())
+ .withDefaultCatalog(catalogView.defaultCatalog())
+ .withDefaultSchema(catalogView.defaultSchema())
+ .withProperties(catalogView.properties())
+ .withAuditInfo(audit)
+ .build();
try {
- store.put(newViewEntity, false /* overwrite */);
- LOG.info("Successfully imported view {} into entity store with id {}",
ident, uid);
- return EntityCombinedView.of(catalogView,
newViewEntity).withImported(true);
+ store.put(viewEntity, true /* overwrite */);
+ } catch (EntityAlreadyExistsException e) {
+ LOG.error("Failed to import view {} with id {} to the store.", ident,
uid, e);
+ throw new UnsupportedOperationException(
+ "View managed by multiple catalogs. This may cause unexpected issues
such as privilege conflicts. "
+ + "To resolve: Remove all catalogs managing this view, then
recreate one catalog to ensure single-catalog management.");
Review Comment:
`importView` converts `EntityAlreadyExistsException` into an
`UnsupportedOperationException`. This exception type is misleading (the
operation *is* supported; it failed due to a conflicting entity). Consider
throwing an `IllegalStateException` (or a domain-specific exception) that
captures the conflict cause and includes the view identifier/uid to make
diagnostics and upstream handling clearer.
```suggestion
throw new IllegalStateException(
String.format(
"Failed to import view %s with id %s because a conflicting
view entity already exists in the store. "
+ "This may indicate that the view is managed by multiple
catalogs. "
+ "To resolve: remove all catalogs managing this view,
then recreate one catalog to ensure single-catalog management.",
ident, uid),
e);
```
##########
core/src/main/java/org/apache/gravitino/catalog/ViewOperationDispatcher.java:
##########
@@ -66,149 +73,498 @@ public ViewOperationDispatcher(
}
/**
- * Load view metadata by identifier from the catalog.
+ * Lists the views within a schema.
*
- * <p>This method first checks if the view exists in Gravitino's
EntityStore. If not found, it
- * loads from the catalog and auto-imports into EntityStore.
+ * @param namespace The namespace of the schema containing the views.
+ * @return An array of {@link NameIdentifier} objects representing the
identifiers of the views in
+ * the schema.
+ * @throws NoSuchSchemaException If the specified schema does not exist.
+ */
+ @Override
+ public NameIdentifier[] listViews(Namespace namespace) throws
NoSuchSchemaException {
+ return TreeLockUtils.doWithTreeLock(
+ NameIdentifier.of(namespace.levels()),
+ LockType.READ,
+ () ->
+ doWithCatalog(
+ getCatalogIdentifier(NameIdentifier.of(namespace.levels())),
+ c -> c.doWithViewOps(v -> v.listViews(namespace)),
+ NoSuchSchemaException.class));
+ }
+
+ /**
+ * Loads a view.
*
- * @param ident The view identifier.
- * @return The loaded view metadata.
- * @throws NoSuchViewException If the view does not exist.
+ * @param ident The identifier of the view to load.
+ * @return The loaded {@link View} object representing the requested view.
+ * @throws NoSuchViewException If the specified view does not exist.
*/
@Override
public View loadView(NameIdentifier ident) throws NoSuchViewException {
LOG.info("Loading view: {}", ident);
- // First load with READ lock to check if view is already imported
EntityCombinedView entityCombinedView =
TreeLockUtils.doWithTreeLock(ident, LockType.READ, () ->
internalLoadView(ident));
if (!entityCombinedView.imported()) {
- // Load the schema to make sure the schema is imported.
SchemaDispatcher schemaDispatcher =
GravitinoEnv.getInstance().schemaDispatcher();
NameIdentifier schemaIdent =
NameIdentifier.of(ident.namespace().levels());
schemaDispatcher.loadSchema(schemaIdent);
- // Import the view.
entityCombinedView =
TreeLockUtils.doWithTreeLock(schemaIdent, LockType.WRITE, () ->
importView(ident));
}
return entityCombinedView;
}
+ /**
+ * Creates a new view in a schema.
+ *
+ * @param ident The identifier of the view to create.
+ * @param comment A description or comment associated with the view.
+ * @param columns An array of {@link Column} objects representing the output
columns of the view.
+ * @param representations An array of {@link Representation} objects
representing the SQL
+ * definitions of the view across different dialects.
+ * @param defaultCatalog The default catalog to use for unqualified
references.
+ * @param defaultSchema The default schema to use for unqualified references.
+ * @param properties Additional properties to set for the view.
+ * @return The newly created {@link View} object.
+ * @throws NoSuchSchemaException If the schema in which to create the view
does not exist.
+ * @throws ViewAlreadyExistsException If a view with the same name already
exists in the schema.
+ */
+ @Override
+ public View createView(
+ NameIdentifier ident,
+ String comment,
+ Column[] columns,
+ Representation[] representations,
+ @Nullable String defaultCatalog,
+ @Nullable String defaultSchema,
+ Map<String, String> properties)
+ throws NoSuchSchemaException, ViewAlreadyExistsException {
+ Preconditions.checkArgument(
+ representations != null && representations.length >= 1,
+ "representations must not be null or empty");
+
+ // Load the schema to make sure the schema exists.
+ SchemaDispatcher schemaDispatcher =
GravitinoEnv.getInstance().schemaDispatcher();
+ NameIdentifier schemaIdent = NameIdentifier.of(ident.namespace().levels());
+ schemaDispatcher.loadSchema(schemaIdent);
+
+ return TreeLockUtils.doWithTreeLock(
+ schemaIdent,
+ LockType.WRITE,
+ () ->
+ internalCreateView(
+ ident,
+ comment,
+ columns,
+ representations,
+ defaultCatalog,
+ defaultSchema,
+ properties));
+ }
+
+ /**
+ * Alters an existing view.
+ *
+ * @param ident The identifier of the view to alter.
+ * @param changes An array of {@link ViewChange} objects representing the
changes to apply to the
+ * view.
+ * @return The altered {@link View} object after applying the changes.
+ * @throws NoSuchViewException If the view to alter does not exist.
+ * @throws IllegalArgumentException If an unsupported or invalid change is
specified.
+ */
@Override
- public NameIdentifier[] listViews(Namespace namespace) {
- throw new UnsupportedOperationException("Listing views is not supported
yet");
+ public View alterView(NameIdentifier ident, ViewChange... changes)
+ throws NoSuchViewException, IllegalArgumentException {
+ validateAlterProperties(ident,
HasPropertyMetadata::tablePropertiesMetadata, changes);
+ NameIdentifier lockIdent = ident;
+ for (ViewChange change : changes) {
+ if (change instanceof ViewChange.RenameView) {
+ lockIdent = getSchemaIdentifier(ident);
+ break;
+ }
+ }
+
+ NameIdentifier nameIdentifierForLock = lockIdent;
+ return TreeLockUtils.doWithTreeLock(
+ nameIdentifierForLock,
+ nameIdentifierForLock.equals(ident) ? LockType.READ : LockType.WRITE,
+ () -> {
+ NameIdentifier catalogIdent = getCatalogIdentifier(ident);
+ View alteredView =
+ doWithCatalog(
+ catalogIdent,
+ c ->
+ c.doWithViewOps(
+ v -> v.alterView(ident,
applyCapabilities(c.capabilities(), changes))),
+ NoSuchViewException.class,
+ IllegalArgumentException.class);
+
+ boolean isManagedView = isManagedEntity(catalogIdent,
Capability.Scope.VIEW);
+ if (isManagedView) {
+ return EntityCombinedView.of(alteredView)
+ .withHiddenProperties(
+ getHiddenPropertyNames(
+ catalogIdent,
+ HasPropertyMetadata::tablePropertiesMetadata,
+ alteredView.properties()));
+ }
+
+ ViewEntity existing = getEntity(ident, VIEW, ViewEntity.class);
+ if (existing == null) {
+ return EntityCombinedView.of(alteredView)
+ .withHiddenProperties(
+ getHiddenPropertyNames(
+ catalogIdent,
+ HasPropertyMetadata::tablePropertiesMetadata,
+ alteredView.properties()));
+ }
Review Comment:
For unmanaged views, `alterView` looks up the existing `ViewEntity` only by
`NameIdentifier` (`getEntity(ident, VIEW, ...)`). For views created by
Gravitino (i.e., carrying a `StringIdentifier` in catalog properties), this can
fail when the store is keyed under a different identifier (e.g., external
rename / identifier not persisted by catalog). The table/schema dispatchers
handle this by deriving the entity ID from `StringIdentifier` and updating via
`operateOnEntity(..., id)`. Consider mirroring that logic here: resolve the
view entity ID from `alteredView.properties()` when present, and update the
store by ID; otherwise fall back to the name-based lookup.
##########
core/src/main/java/org/apache/gravitino/catalog/ViewOperationDispatcher.java:
##########
@@ -66,149 +73,498 @@ public ViewOperationDispatcher(
}
/**
- * Load view metadata by identifier from the catalog.
+ * Lists the views within a schema.
*
- * <p>This method first checks if the view exists in Gravitino's
EntityStore. If not found, it
- * loads from the catalog and auto-imports into EntityStore.
+ * @param namespace The namespace of the schema containing the views.
+ * @return An array of {@link NameIdentifier} objects representing the
identifiers of the views in
+ * the schema.
+ * @throws NoSuchSchemaException If the specified schema does not exist.
+ */
+ @Override
+ public NameIdentifier[] listViews(Namespace namespace) throws
NoSuchSchemaException {
+ return TreeLockUtils.doWithTreeLock(
+ NameIdentifier.of(namespace.levels()),
+ LockType.READ,
+ () ->
+ doWithCatalog(
+ getCatalogIdentifier(NameIdentifier.of(namespace.levels())),
+ c -> c.doWithViewOps(v -> v.listViews(namespace)),
+ NoSuchSchemaException.class));
+ }
+
+ /**
+ * Loads a view.
*
- * @param ident The view identifier.
- * @return The loaded view metadata.
- * @throws NoSuchViewException If the view does not exist.
+ * @param ident The identifier of the view to load.
+ * @return The loaded {@link View} object representing the requested view.
+ * @throws NoSuchViewException If the specified view does not exist.
*/
@Override
public View loadView(NameIdentifier ident) throws NoSuchViewException {
LOG.info("Loading view: {}", ident);
- // First load with READ lock to check if view is already imported
EntityCombinedView entityCombinedView =
TreeLockUtils.doWithTreeLock(ident, LockType.READ, () ->
internalLoadView(ident));
if (!entityCombinedView.imported()) {
- // Load the schema to make sure the schema is imported.
SchemaDispatcher schemaDispatcher =
GravitinoEnv.getInstance().schemaDispatcher();
NameIdentifier schemaIdent =
NameIdentifier.of(ident.namespace().levels());
schemaDispatcher.loadSchema(schemaIdent);
- // Import the view.
entityCombinedView =
TreeLockUtils.doWithTreeLock(schemaIdent, LockType.WRITE, () ->
importView(ident));
}
return entityCombinedView;
}
+ /**
+ * Creates a new view in a schema.
+ *
+ * @param ident The identifier of the view to create.
+ * @param comment A description or comment associated with the view.
+ * @param columns An array of {@link Column} objects representing the output
columns of the view.
+ * @param representations An array of {@link Representation} objects
representing the SQL
+ * definitions of the view across different dialects.
+ * @param defaultCatalog The default catalog to use for unqualified
references.
+ * @param defaultSchema The default schema to use for unqualified references.
+ * @param properties Additional properties to set for the view.
+ * @return The newly created {@link View} object.
+ * @throws NoSuchSchemaException If the schema in which to create the view
does not exist.
+ * @throws ViewAlreadyExistsException If a view with the same name already
exists in the schema.
+ */
+ @Override
+ public View createView(
+ NameIdentifier ident,
+ String comment,
+ Column[] columns,
+ Representation[] representations,
+ @Nullable String defaultCatalog,
+ @Nullable String defaultSchema,
+ Map<String, String> properties)
+ throws NoSuchSchemaException, ViewAlreadyExistsException {
+ Preconditions.checkArgument(
+ representations != null && representations.length >= 1,
+ "representations must not be null or empty");
+
+ // Load the schema to make sure the schema exists.
+ SchemaDispatcher schemaDispatcher =
GravitinoEnv.getInstance().schemaDispatcher();
+ NameIdentifier schemaIdent = NameIdentifier.of(ident.namespace().levels());
+ schemaDispatcher.loadSchema(schemaIdent);
+
+ return TreeLockUtils.doWithTreeLock(
+ schemaIdent,
+ LockType.WRITE,
+ () ->
+ internalCreateView(
+ ident,
+ comment,
+ columns,
+ representations,
+ defaultCatalog,
+ defaultSchema,
+ properties));
+ }
+
+ /**
+ * Alters an existing view.
+ *
+ * @param ident The identifier of the view to alter.
+ * @param changes An array of {@link ViewChange} objects representing the
changes to apply to the
+ * view.
+ * @return The altered {@link View} object after applying the changes.
+ * @throws NoSuchViewException If the view to alter does not exist.
+ * @throws IllegalArgumentException If an unsupported or invalid change is
specified.
+ */
@Override
- public NameIdentifier[] listViews(Namespace namespace) {
- throw new UnsupportedOperationException("Listing views is not supported
yet");
+ public View alterView(NameIdentifier ident, ViewChange... changes)
+ throws NoSuchViewException, IllegalArgumentException {
+ validateAlterProperties(ident,
HasPropertyMetadata::tablePropertiesMetadata, changes);
+ NameIdentifier lockIdent = ident;
+ for (ViewChange change : changes) {
+ if (change instanceof ViewChange.RenameView) {
+ lockIdent = getSchemaIdentifier(ident);
+ break;
+ }
+ }
+
+ NameIdentifier nameIdentifierForLock = lockIdent;
+ return TreeLockUtils.doWithTreeLock(
+ nameIdentifierForLock,
+ nameIdentifierForLock.equals(ident) ? LockType.READ : LockType.WRITE,
Review Comment:
`alterView` acquires a READ tree lock when `nameIdentifierForLock` equals
`ident`, but this method performs mutating operations (catalog alter +
entity-store update). With `TreeLock` using a `ReentrantReadWriteLock`, a READ
lock allows concurrent callers to interleave multiple alters/loads on the same
view, which can lead to inconsistent state. Consider using a WRITE lock for
non-rename alters as well (or otherwise ensure the lock semantics match the
intended exclusivity for mutations).
```suggestion
LockType.WRITE,
```
##########
core/src/main/java/org/apache/gravitino/catalog/ViewOperationDispatcher.java:
##########
@@ -66,149 +73,498 @@ public ViewOperationDispatcher(
}
/**
- * Load view metadata by identifier from the catalog.
+ * Lists the views within a schema.
*
- * <p>This method first checks if the view exists in Gravitino's
EntityStore. If not found, it
- * loads from the catalog and auto-imports into EntityStore.
+ * @param namespace The namespace of the schema containing the views.
+ * @return An array of {@link NameIdentifier} objects representing the
identifiers of the views in
+ * the schema.
+ * @throws NoSuchSchemaException If the specified schema does not exist.
+ */
+ @Override
+ public NameIdentifier[] listViews(Namespace namespace) throws
NoSuchSchemaException {
+ return TreeLockUtils.doWithTreeLock(
+ NameIdentifier.of(namespace.levels()),
+ LockType.READ,
+ () ->
+ doWithCatalog(
+ getCatalogIdentifier(NameIdentifier.of(namespace.levels())),
+ c -> c.doWithViewOps(v -> v.listViews(namespace)),
+ NoSuchSchemaException.class));
+ }
+
+ /**
+ * Loads a view.
*
- * @param ident The view identifier.
- * @return The loaded view metadata.
- * @throws NoSuchViewException If the view does not exist.
+ * @param ident The identifier of the view to load.
+ * @return The loaded {@link View} object representing the requested view.
+ * @throws NoSuchViewException If the specified view does not exist.
*/
@Override
public View loadView(NameIdentifier ident) throws NoSuchViewException {
LOG.info("Loading view: {}", ident);
- // First load with READ lock to check if view is already imported
EntityCombinedView entityCombinedView =
TreeLockUtils.doWithTreeLock(ident, LockType.READ, () ->
internalLoadView(ident));
if (!entityCombinedView.imported()) {
- // Load the schema to make sure the schema is imported.
SchemaDispatcher schemaDispatcher =
GravitinoEnv.getInstance().schemaDispatcher();
NameIdentifier schemaIdent =
NameIdentifier.of(ident.namespace().levels());
schemaDispatcher.loadSchema(schemaIdent);
- // Import the view.
entityCombinedView =
TreeLockUtils.doWithTreeLock(schemaIdent, LockType.WRITE, () ->
importView(ident));
}
return entityCombinedView;
}
+ /**
+ * Creates a new view in a schema.
+ *
+ * @param ident The identifier of the view to create.
+ * @param comment A description or comment associated with the view.
+ * @param columns An array of {@link Column} objects representing the output
columns of the view.
+ * @param representations An array of {@link Representation} objects
representing the SQL
+ * definitions of the view across different dialects.
+ * @param defaultCatalog The default catalog to use for unqualified
references.
+ * @param defaultSchema The default schema to use for unqualified references.
+ * @param properties Additional properties to set for the view.
+ * @return The newly created {@link View} object.
+ * @throws NoSuchSchemaException If the schema in which to create the view
does not exist.
+ * @throws ViewAlreadyExistsException If a view with the same name already
exists in the schema.
+ */
+ @Override
+ public View createView(
+ NameIdentifier ident,
+ String comment,
+ Column[] columns,
+ Representation[] representations,
+ @Nullable String defaultCatalog,
+ @Nullable String defaultSchema,
+ Map<String, String> properties)
+ throws NoSuchSchemaException, ViewAlreadyExistsException {
+ Preconditions.checkArgument(
+ representations != null && representations.length >= 1,
+ "representations must not be null or empty");
+
+ // Load the schema to make sure the schema exists.
+ SchemaDispatcher schemaDispatcher =
GravitinoEnv.getInstance().schemaDispatcher();
+ NameIdentifier schemaIdent = NameIdentifier.of(ident.namespace().levels());
+ schemaDispatcher.loadSchema(schemaIdent);
+
+ return TreeLockUtils.doWithTreeLock(
+ schemaIdent,
+ LockType.WRITE,
+ () ->
+ internalCreateView(
+ ident,
+ comment,
+ columns,
+ representations,
+ defaultCatalog,
+ defaultSchema,
+ properties));
+ }
+
+ /**
+ * Alters an existing view.
+ *
+ * @param ident The identifier of the view to alter.
+ * @param changes An array of {@link ViewChange} objects representing the
changes to apply to the
+ * view.
+ * @return The altered {@link View} object after applying the changes.
+ * @throws NoSuchViewException If the view to alter does not exist.
+ * @throws IllegalArgumentException If an unsupported or invalid change is
specified.
+ */
@Override
- public NameIdentifier[] listViews(Namespace namespace) {
- throw new UnsupportedOperationException("Listing views is not supported
yet");
+ public View alterView(NameIdentifier ident, ViewChange... changes)
+ throws NoSuchViewException, IllegalArgumentException {
+ validateAlterProperties(ident,
HasPropertyMetadata::tablePropertiesMetadata, changes);
+ NameIdentifier lockIdent = ident;
+ for (ViewChange change : changes) {
+ if (change instanceof ViewChange.RenameView) {
+ lockIdent = getSchemaIdentifier(ident);
+ break;
+ }
+ }
+
+ NameIdentifier nameIdentifierForLock = lockIdent;
+ return TreeLockUtils.doWithTreeLock(
+ nameIdentifierForLock,
+ nameIdentifierForLock.equals(ident) ? LockType.READ : LockType.WRITE,
+ () -> {
+ NameIdentifier catalogIdent = getCatalogIdentifier(ident);
+ View alteredView =
+ doWithCatalog(
+ catalogIdent,
+ c ->
+ c.doWithViewOps(
+ v -> v.alterView(ident,
applyCapabilities(c.capabilities(), changes))),
+ NoSuchViewException.class,
+ IllegalArgumentException.class);
+
+ boolean isManagedView = isManagedEntity(catalogIdent,
Capability.Scope.VIEW);
+ if (isManagedView) {
+ return EntityCombinedView.of(alteredView)
+ .withHiddenProperties(
+ getHiddenPropertyNames(
+ catalogIdent,
+ HasPropertyMetadata::tablePropertiesMetadata,
+ alteredView.properties()));
+ }
+
+ ViewEntity existing = getEntity(ident, VIEW, ViewEntity.class);
+ if (existing == null) {
+ return EntityCombinedView.of(alteredView)
+ .withHiddenProperties(
+ getHiddenPropertyNames(
+ catalogIdent,
+ HasPropertyMetadata::tablePropertiesMetadata,
+ alteredView.properties()));
+ }
+
+ long viewId = existing.id();
+ ViewEntity updatedViewEntity =
+ operateOnEntity(
+ ident,
+ id ->
+ store.update(
+ id,
+ ViewEntity.class,
+ VIEW,
+ viewEntity -> applyChangesToEntity(viewEntity,
alteredView, changes)),
+ "UPDATE",
+ viewId);
+
+ return EntityCombinedView.of(alteredView, updatedViewEntity)
+ .withHiddenProperties(
+ getHiddenPropertyNames(
+ catalogIdent,
+ HasPropertyMetadata::tablePropertiesMetadata,
+ alteredView.properties()));
+ });
}
+ /**
+ * Drops a view from the catalog.
+ *
+ * @param ident The identifier of the view to drop.
+ * @return {@code true} if the view was successfully dropped, {@code false}
if the view does not
+ * exist.
+ * @throws RuntimeException If an error occurs while dropping the view.
+ */
@Override
public boolean dropView(NameIdentifier ident) {
- throw new UnsupportedOperationException("Dropping a view is not supported
yet");
+ NameIdentifier schemaIdentifier = getSchemaIdentifier(ident);
+ return TreeLockUtils.doWithTreeLock(
+ schemaIdentifier,
+ LockType.WRITE,
+ () -> {
+ NameIdentifier catalogIdent = getCatalogIdentifier(ident);
+ boolean droppedFromCatalog =
+ doWithCatalog(
+ catalogIdent,
+ c -> c.doWithViewOps(v -> v.dropView(ident)),
+ RuntimeException.class);
+
+ boolean isManagedView = isManagedEntity(catalogIdent,
Capability.Scope.VIEW);
+ if (isManagedView) {
+ return droppedFromCatalog;
+ }
+
+ // For unmanaged view, it could happen that the view:
+ // 1. Is not found in the catalog (dropped directly from underlying
sources)
+ // 2. Is found in the catalog but not in the store (not managed by
Gravitino)
+ // 3. Is found in the catalog and the store (managed by Gravitino)
+ // 4. Neither found in the catalog nor in the store.
+ // In all situations, we try to delete the view from the store, but
we don't take the
+ // return value of the store operation into account. We only take
the return value of the
+ // catalog into account.
+ try {
+ store.delete(ident, VIEW);
+ } catch (NoSuchEntityException e) {
+ LOG.warn("The view to be dropped does not exist in the store: {}",
ident, e);
+ } catch (Exception e) {
+ throw new RuntimeException(e);
+ }
+ return droppedFromCatalog;
+ });
}
- @Override
- public View createView(
+ private View internalCreateView(
NameIdentifier ident,
String comment,
Column[] columns,
Representation[] representations,
@Nullable String defaultCatalog,
@Nullable String defaultSchema,
Map<String, String> properties) {
- throw new UnsupportedOperationException("Creating a view is not supported
yet");
- }
+ NameIdentifier catalogIdent = getCatalogIdentifier(ident);
- @Override
- public View alterView(NameIdentifier ident, ViewChange... changes) {
- throw new UnsupportedOperationException("Altering a view is not supported
yet");
+ doWithCatalog(
+ catalogIdent,
+ c ->
+ c.doWithPropertiesMeta(
+ p -> {
+ validatePropertyForCreate(p.tablePropertiesMetadata(),
properties);
+ return null;
+ }),
+ IllegalArgumentException.class);
+
+ long uid = idGenerator.nextId();
+ // Add StringIdentifier to the properties, the specific catalog will
handle this
+ // StringIdentifier to make sure only when the operation is successful,
the related
+ // ViewEntity will be visible.
+ StringIdentifier stringId = StringIdentifier.fromId(uid);
+ Map<String, String> updatedProperties =
+ StringIdentifier.newPropertiesWithId(stringId, properties);
+
+ View catalogView =
+ doWithCatalog(
+ catalogIdent,
+ c ->
+ c.doWithViewOps(
+ v ->
+ v.createView(
+ ident,
+ comment,
+ columns == null ? new Column[0] : columns,
+ representations,
+ defaultCatalog,
+ defaultSchema,
+ updatedProperties)),
+ NoSuchSchemaException.class,
+ ViewAlreadyExistsException.class);
+
+ // If the view is managed by Gravitino, we don't need to create ViewEntity
and store it again.
+ boolean isManagedView = isManagedEntity(catalogIdent,
Capability.Scope.VIEW);
+ if (isManagedView) {
+ return EntityCombinedView.of(catalogView)
+ .withHiddenProperties(
+ getHiddenPropertyNames(
+ catalogIdent,
+ HasPropertyMetadata::tablePropertiesMetadata,
+ catalogView.properties()));
+ }
+
+ AuditInfo audit =
+ AuditInfo.builder()
+ .withCreator(PrincipalUtils.getCurrentPrincipal().getName())
+ .withCreateTime(Instant.now())
+ .build();
+
+ ViewEntity viewEntity =
+ ViewEntity.builder()
+ .withId(uid)
+ .withName(ident.name())
+ .withNamespace(ident.namespace())
+ .withComment(comment)
+ .withColumns(columns == null ? new Column[0] : columns)
+ .withRepresentations(representations)
+ .withDefaultCatalog(defaultCatalog)
+ .withDefaultSchema(defaultSchema)
+ .withProperties(properties)
+ .withAuditInfo(audit)
+ .build();
+
+ try {
+ store.put(viewEntity, true /* overwrite */);
+ } catch (Exception e) {
+ LOG.error(FormattedErrorMessages.STORE_OP_FAILURE, "put", ident, e);
+ return EntityCombinedView.of(catalogView)
+ .withHiddenProperties(
+ getHiddenPropertyNames(
+ catalogIdent,
+ HasPropertyMetadata::tablePropertiesMetadata,
+ catalogView.properties()));
+ }
+
+ // Merge both the metadata from catalog operation and the metadata from
entity store.
+ return EntityCombinedView.of(catalogView, viewEntity)
+ .withHiddenProperties(
+ getHiddenPropertyNames(
+ catalogIdent,
+ HasPropertyMetadata::tablePropertiesMetadata,
+ catalogView.properties()));
}
- /**
- * Internal method to load view and check if it exists in entity store.
- *
- * @param ident The view identifier.
- * @return EntityCombinedView containing the view and import status.
- * @throws NoSuchViewException If the view does not exist.
- */
private EntityCombinedView internalLoadView(NameIdentifier ident) throws
NoSuchViewException {
- // Load view from the underlying catalog
- View catalogView =
+ NameIdentifier catalogIdentifier = getCatalogIdentifier(ident);
+ View view =
doWithCatalog(
- getCatalogIdentifier(ident),
+ catalogIdentifier,
c -> c.doWithViewOps(v -> v.loadView(ident)),
NoSuchViewException.class);
- // Check if view exists in entity store
- try {
- ViewEntity viewEntity = store.get(ident, Entity.EntityType.VIEW,
ViewEntity.class);
- return EntityCombinedView.of(catalogView, viewEntity).withImported(true);
- } catch (NoSuchEntityException e) {
- // View not in store yet
- LOG.debug("View {} not found in entity store", ident);
- return EntityCombinedView.of(catalogView).withImported(false);
- } catch (IOException ioe) {
- LOG.warn("Failed to check if view {} exists in entity store", ident,
ioe);
- return EntityCombinedView.of(catalogView).withImported(false);
+ boolean isManagedView = isManagedEntity(catalogIdentifier,
Capability.Scope.VIEW);
+ if (isManagedView) {
+ return EntityCombinedView.of(view)
+ .withHiddenProperties(
+ getHiddenPropertyNames(
+ catalogIdentifier,
+ HasPropertyMetadata::tablePropertiesMetadata,
+ view.properties()))
+ // The metadata of managed view is stored by Gravitino, so it is
always imported.
+ .withImported(true /* imported */);
+ }
+
+ StringIdentifier stringId = getStringIdFromProperties(view.properties());
+ // Case 1: The view is not created by Gravitino or the external system
does not support storing
+ // string identifier.
+ if (stringId == null) {
+ ViewEntity viewEntity = getEntity(ident, VIEW, ViewEntity.class);
+ if (viewEntity == null) {
+ return EntityCombinedView.of(view)
+ .withHiddenProperties(
+ getHiddenPropertyNames(
+ catalogIdentifier,
+ HasPropertyMetadata::tablePropertiesMetadata,
+ view.properties()))
+ // Some views don't have properties or are not created by
Gravitino,
+ // we can't use stringIdentifier to judge whether view is ever
imported or not.
+ // We need to check whether the entity exists.
+ .withImported(false);
+ }
+
+ return EntityCombinedView.of(view, viewEntity)
+ .withHiddenProperties(
+ getHiddenPropertyNames(
+ catalogIdentifier,
+ HasPropertyMetadata::tablePropertiesMetadata,
+ view.properties()))
+ // For some catalogs, the identifier information is not stored in
the view's
+ // metadata, we need to check if this view exists in the store, if
so we don't
+ // need to import.
+ .withImported(true);
}
+
+ ViewEntity viewEntity =
+ operateOnEntity(
+ ident,
+ identifier -> store.get(identifier, VIEW, ViewEntity.class),
+ "GET",
+ stringId.id());
+
+ return EntityCombinedView.of(view, viewEntity)
+ .withHiddenProperties(
+ getHiddenPropertyNames(
+ catalogIdentifier,
HasPropertyMetadata::tablePropertiesMetadata, view.properties()))
+ .withImported(viewEntity != null);
}
- /**
- * Import view into Gravitino entity store.
- *
- * @param ident The view identifier.
- * @return EntityCombinedView containing the view and import status.
- * @throws NoSuchViewException If the view does not exist.
- */
private EntityCombinedView importView(NameIdentifier ident) throws
NoSuchViewException {
- // Double-check if already imported (another thread might have imported
between locks)
EntityCombinedView entityCombinedView = internalLoadView(ident);
if (entityCombinedView.imported()) {
return entityCombinedView;
}
- LOG.info("Auto-importing view {} into Gravitino entity store", ident);
- long uid = idGenerator.nextId();
+ StringIdentifier stringId =
+
getStringIdFromProperties(entityCombinedView.viewFromCatalog().properties());
+
+ long uid;
+ if (stringId != null) {
+ // If the entity in the store doesn't match the external system, we use
the data
+ // of external system to correct it.
+ LOG.warn(
+ "The View uid {} existed but still need to be imported, this could
be happened "
+ + "when View is renamed by external systems not controlled by
Gravitino. In this "
+ + "case, we need to overwrite the stored entity to keep the
consistency.",
+ stringId);
+ uid = stringId.id();
+ } else {
+ // If entity doesn't exist, we import the entity from the external
system.
+ uid = idGenerator.nextId();
+ }
+
View catalogView = entityCombinedView.viewFromCatalog();
- ViewEntity newViewEntity = buildViewEntityForImport(uid, ident,
catalogView);
+ AuditInfo audit =
+ AuditInfo.builder()
+ .withCreator(catalogView.auditInfo().creator())
+ .withCreateTime(catalogView.auditInfo().createTime())
+ .withLastModifier(catalogView.auditInfo().lastModifier())
+ .withLastModifiedTime(catalogView.auditInfo().lastModifiedTime())
+ .build();
+ ViewEntity viewEntity =
+ ViewEntity.builder()
+ .withId(uid)
+ .withName(ident.name())
+ .withNamespace(ident.namespace())
+ .withComment(catalogView.comment())
+ .withColumns(catalogView.columns() == null ? new Column[0] :
catalogView.columns())
+ .withRepresentations(
+ catalogView.representations() == null
+ ? new Representation[0]
+ : catalogView.representations())
+ .withDefaultCatalog(catalogView.defaultCatalog())
+ .withDefaultSchema(catalogView.defaultSchema())
+ .withProperties(catalogView.properties())
+ .withAuditInfo(audit)
+ .build();
try {
- store.put(newViewEntity, false /* overwrite */);
- LOG.info("Successfully imported view {} into entity store with id {}",
ident, uid);
- return EntityCombinedView.of(catalogView,
newViewEntity).withImported(true);
+ store.put(viewEntity, true /* overwrite */);
+ } catch (EntityAlreadyExistsException e) {
+ LOG.error("Failed to import view {} with id {} to the store.", ident,
uid, e);
+ throw new UnsupportedOperationException(
+ "View managed by multiple catalogs. This may cause unexpected issues
such as privilege conflicts. "
+ + "To resolve: Remove all catalogs managing this view, then
recreate one catalog to ensure single-catalog management.");
} catch (Exception e) {
- // Log but don't fail - view import is best-effort
- LOG.warn("Failed to import view {} into entity store: {}", ident,
e.getMessage());
- return EntityCombinedView.of(catalogView).withImported(false);
+ LOG.error(FormattedErrorMessages.STORE_OP_FAILURE, "put", ident, e);
+ throw new RuntimeException("Fail to import the view entity to the
store.", e);
}
- }
- private ViewEntity buildViewEntityForImport(Long uid, NameIdentifier ident,
View view) {
- return ViewEntity.builder()
- .withId(uid)
- .withName(ident.name())
- .withNamespace(ident.namespace())
- .withComment(view.comment())
- .withColumns(view.columns() == null ? new Column[0] : view.columns())
- .withRepresentations(
- view.representations() == null ? new Representation[0] :
view.representations())
- .withDefaultCatalog(view.defaultCatalog())
- .withDefaultSchema(view.defaultSchema())
- .withProperties(view.properties())
- .withAuditInfo(toAuditInfo(view.auditInfo()))
- .build();
+ return EntityCombinedView.of(catalogView, viewEntity)
+ .withHiddenProperties(
+ getHiddenPropertyNames(
+ getCatalogIdentifier(ident),
+ HasPropertyMetadata::tablePropertiesMetadata,
+ catalogView.properties()))
+ .withImported(true);
}
- private AuditInfo toAuditInfo(Audit audit) {
- if (audit == null) {
- return AuditInfo.EMPTY;
+ private ViewEntity applyChangesToEntity(
+ ViewEntity current, View alteredView, ViewChange[] changes) {
+ String name = alteredView.name();
+ String comment = alteredView.comment();
+ String defaultCatalog = alteredView.defaultCatalog();
+ String defaultSchema = alteredView.defaultSchema();
+ Map<String, String> properties =
+ current.properties() == null ? new HashMap<>() : new
HashMap<>(current.properties());
+
+ for (ViewChange change : changes) {
+ if (change instanceof ViewChange.SetProperty) {
+ ViewChange.SetProperty sp = (ViewChange.SetProperty) change;
+ properties.put(sp.getProperty(), sp.getValue());
+ } else if (change instanceof ViewChange.RemoveProperty) {
+ properties.remove(((ViewChange.RemoveProperty) change).getProperty());
+ } else if (!(change instanceof ViewChange.RenameView)
+ && !(change instanceof ViewChange.ReplaceView)) {
+ throw new IllegalArgumentException("Unsupported view change: " +
change);
+ }
}
- return AuditInfo.builder()
- .withCreator(audit.creator())
- .withCreateTime(audit.createTime())
- .withLastModifier(audit.lastModifier())
- .withLastModifiedTime(audit.lastModifiedTime())
+ Namespace namespace = current.namespace();
+
+ AuditInfo newAudit =
+ AuditInfo.builder()
+ .withCreator(current.auditInfo().creator())
+ .withCreateTime(current.auditInfo().createTime())
+ .withLastModifier(PrincipalUtils.getCurrentPrincipal().getName())
+ .withLastModifiedTime(Instant.now())
+ .build();
+
+ return ViewEntity.builder()
+ .withId(current.id())
+ .withName(name)
+ .withNamespace(namespace)
+ .withComment(comment)
+ .withColumns(Arrays.copyOf(alteredView.columns(),
alteredView.columns().length))
+ .withRepresentations(
+ Arrays.copyOf(alteredView.representations(),
alteredView.representations().length))
Review Comment:
`applyChangesToEntity` calls `Arrays.copyOf(alteredView.columns(), ...)` and
`Arrays.copyOf(alteredView.representations(), ...)` without null checks.
Elsewhere in this dispatcher (e.g., import) you already defensively handle
`columns()` / `representations()` potentially being null, so this can throw an
NPE if a catalog returns null arrays. Please guard these with null→empty-array
handling before copying.
##########
core/src/main/java/org/apache/gravitino/catalog/ViewOperationDispatcher.java:
##########
@@ -66,149 +73,498 @@ public ViewOperationDispatcher(
}
/**
- * Load view metadata by identifier from the catalog.
+ * Lists the views within a schema.
*
- * <p>This method first checks if the view exists in Gravitino's
EntityStore. If not found, it
- * loads from the catalog and auto-imports into EntityStore.
+ * @param namespace The namespace of the schema containing the views.
+ * @return An array of {@link NameIdentifier} objects representing the
identifiers of the views in
+ * the schema.
+ * @throws NoSuchSchemaException If the specified schema does not exist.
+ */
+ @Override
+ public NameIdentifier[] listViews(Namespace namespace) throws
NoSuchSchemaException {
+ return TreeLockUtils.doWithTreeLock(
+ NameIdentifier.of(namespace.levels()),
+ LockType.READ,
+ () ->
+ doWithCatalog(
+ getCatalogIdentifier(NameIdentifier.of(namespace.levels())),
+ c -> c.doWithViewOps(v -> v.listViews(namespace)),
+ NoSuchSchemaException.class));
+ }
+
+ /**
+ * Loads a view.
*
- * @param ident The view identifier.
- * @return The loaded view metadata.
- * @throws NoSuchViewException If the view does not exist.
+ * @param ident The identifier of the view to load.
+ * @return The loaded {@link View} object representing the requested view.
+ * @throws NoSuchViewException If the specified view does not exist.
*/
@Override
public View loadView(NameIdentifier ident) throws NoSuchViewException {
LOG.info("Loading view: {}", ident);
- // First load with READ lock to check if view is already imported
EntityCombinedView entityCombinedView =
TreeLockUtils.doWithTreeLock(ident, LockType.READ, () ->
internalLoadView(ident));
if (!entityCombinedView.imported()) {
- // Load the schema to make sure the schema is imported.
SchemaDispatcher schemaDispatcher =
GravitinoEnv.getInstance().schemaDispatcher();
NameIdentifier schemaIdent =
NameIdentifier.of(ident.namespace().levels());
schemaDispatcher.loadSchema(schemaIdent);
- // Import the view.
entityCombinedView =
TreeLockUtils.doWithTreeLock(schemaIdent, LockType.WRITE, () ->
importView(ident));
}
return entityCombinedView;
}
+ /**
+ * Creates a new view in a schema.
+ *
+ * @param ident The identifier of the view to create.
+ * @param comment A description or comment associated with the view.
+ * @param columns An array of {@link Column} objects representing the output
columns of the view.
+ * @param representations An array of {@link Representation} objects
representing the SQL
+ * definitions of the view across different dialects.
+ * @param defaultCatalog The default catalog to use for unqualified
references.
+ * @param defaultSchema The default schema to use for unqualified references.
+ * @param properties Additional properties to set for the view.
+ * @return The newly created {@link View} object.
+ * @throws NoSuchSchemaException If the schema in which to create the view
does not exist.
+ * @throws ViewAlreadyExistsException If a view with the same name already
exists in the schema.
+ */
+ @Override
+ public View createView(
+ NameIdentifier ident,
+ String comment,
+ Column[] columns,
+ Representation[] representations,
+ @Nullable String defaultCatalog,
+ @Nullable String defaultSchema,
+ Map<String, String> properties)
+ throws NoSuchSchemaException, ViewAlreadyExistsException {
+ Preconditions.checkArgument(
+ representations != null && representations.length >= 1,
+ "representations must not be null or empty");
+
+ // Load the schema to make sure the schema exists.
+ SchemaDispatcher schemaDispatcher =
GravitinoEnv.getInstance().schemaDispatcher();
+ NameIdentifier schemaIdent = NameIdentifier.of(ident.namespace().levels());
+ schemaDispatcher.loadSchema(schemaIdent);
+
+ return TreeLockUtils.doWithTreeLock(
+ schemaIdent,
+ LockType.WRITE,
+ () ->
+ internalCreateView(
+ ident,
+ comment,
+ columns,
+ representations,
+ defaultCatalog,
+ defaultSchema,
+ properties));
+ }
+
+ /**
+ * Alters an existing view.
+ *
+ * @param ident The identifier of the view to alter.
+ * @param changes An array of {@link ViewChange} objects representing the
changes to apply to the
+ * view.
+ * @return The altered {@link View} object after applying the changes.
+ * @throws NoSuchViewException If the view to alter does not exist.
+ * @throws IllegalArgumentException If an unsupported or invalid change is
specified.
+ */
@Override
- public NameIdentifier[] listViews(Namespace namespace) {
- throw new UnsupportedOperationException("Listing views is not supported
yet");
+ public View alterView(NameIdentifier ident, ViewChange... changes)
+ throws NoSuchViewException, IllegalArgumentException {
+ validateAlterProperties(ident,
HasPropertyMetadata::tablePropertiesMetadata, changes);
+ NameIdentifier lockIdent = ident;
+ for (ViewChange change : changes) {
+ if (change instanceof ViewChange.RenameView) {
+ lockIdent = getSchemaIdentifier(ident);
+ break;
+ }
+ }
+
+ NameIdentifier nameIdentifierForLock = lockIdent;
+ return TreeLockUtils.doWithTreeLock(
+ nameIdentifierForLock,
+ nameIdentifierForLock.equals(ident) ? LockType.READ : LockType.WRITE,
+ () -> {
+ NameIdentifier catalogIdent = getCatalogIdentifier(ident);
+ View alteredView =
+ doWithCatalog(
+ catalogIdent,
+ c ->
+ c.doWithViewOps(
+ v -> v.alterView(ident,
applyCapabilities(c.capabilities(), changes))),
+ NoSuchViewException.class,
+ IllegalArgumentException.class);
+
+ boolean isManagedView = isManagedEntity(catalogIdent,
Capability.Scope.VIEW);
+ if (isManagedView) {
+ return EntityCombinedView.of(alteredView)
+ .withHiddenProperties(
+ getHiddenPropertyNames(
+ catalogIdent,
+ HasPropertyMetadata::tablePropertiesMetadata,
+ alteredView.properties()));
+ }
+
+ ViewEntity existing = getEntity(ident, VIEW, ViewEntity.class);
+ if (existing == null) {
+ return EntityCombinedView.of(alteredView)
+ .withHiddenProperties(
+ getHiddenPropertyNames(
+ catalogIdent,
+ HasPropertyMetadata::tablePropertiesMetadata,
+ alteredView.properties()));
+ }
+
+ long viewId = existing.id();
+ ViewEntity updatedViewEntity =
+ operateOnEntity(
+ ident,
+ id ->
+ store.update(
+ id,
+ ViewEntity.class,
+ VIEW,
+ viewEntity -> applyChangesToEntity(viewEntity,
alteredView, changes)),
+ "UPDATE",
+ viewId);
+
+ return EntityCombinedView.of(alteredView, updatedViewEntity)
+ .withHiddenProperties(
+ getHiddenPropertyNames(
+ catalogIdent,
+ HasPropertyMetadata::tablePropertiesMetadata,
+ alteredView.properties()));
+ });
}
+ /**
+ * Drops a view from the catalog.
+ *
+ * @param ident The identifier of the view to drop.
+ * @return {@code true} if the view was successfully dropped, {@code false}
if the view does not
+ * exist.
+ * @throws RuntimeException If an error occurs while dropping the view.
+ */
@Override
public boolean dropView(NameIdentifier ident) {
- throw new UnsupportedOperationException("Dropping a view is not supported
yet");
+ NameIdentifier schemaIdentifier = getSchemaIdentifier(ident);
+ return TreeLockUtils.doWithTreeLock(
+ schemaIdentifier,
+ LockType.WRITE,
+ () -> {
+ NameIdentifier catalogIdent = getCatalogIdentifier(ident);
+ boolean droppedFromCatalog =
+ doWithCatalog(
+ catalogIdent,
+ c -> c.doWithViewOps(v -> v.dropView(ident)),
+ RuntimeException.class);
+
+ boolean isManagedView = isManagedEntity(catalogIdent,
Capability.Scope.VIEW);
+ if (isManagedView) {
+ return droppedFromCatalog;
+ }
+
+ // For unmanaged view, it could happen that the view:
+ // 1. Is not found in the catalog (dropped directly from underlying
sources)
+ // 2. Is found in the catalog but not in the store (not managed by
Gravitino)
+ // 3. Is found in the catalog and the store (managed by Gravitino)
+ // 4. Neither found in the catalog nor in the store.
+ // In all situations, we try to delete the view from the store, but
we don't take the
+ // return value of the store operation into account. We only take
the return value of the
+ // catalog into account.
+ try {
+ store.delete(ident, VIEW);
+ } catch (NoSuchEntityException e) {
+ LOG.warn("The view to be dropped does not exist in the store: {}",
ident, e);
+ } catch (Exception e) {
+ throw new RuntimeException(e);
+ }
+ return droppedFromCatalog;
+ });
}
- @Override
- public View createView(
+ private View internalCreateView(
NameIdentifier ident,
String comment,
Column[] columns,
Representation[] representations,
@Nullable String defaultCatalog,
@Nullable String defaultSchema,
Map<String, String> properties) {
- throw new UnsupportedOperationException("Creating a view is not supported
yet");
- }
+ NameIdentifier catalogIdent = getCatalogIdentifier(ident);
- @Override
- public View alterView(NameIdentifier ident, ViewChange... changes) {
- throw new UnsupportedOperationException("Altering a view is not supported
yet");
+ doWithCatalog(
+ catalogIdent,
+ c ->
+ c.doWithPropertiesMeta(
+ p -> {
+ validatePropertyForCreate(p.tablePropertiesMetadata(),
properties);
+ return null;
+ }),
+ IllegalArgumentException.class);
+
+ long uid = idGenerator.nextId();
+ // Add StringIdentifier to the properties, the specific catalog will
handle this
+ // StringIdentifier to make sure only when the operation is successful,
the related
+ // ViewEntity will be visible.
+ StringIdentifier stringId = StringIdentifier.fromId(uid);
+ Map<String, String> updatedProperties =
+ StringIdentifier.newPropertiesWithId(stringId, properties);
+
+ View catalogView =
+ doWithCatalog(
+ catalogIdent,
+ c ->
+ c.doWithViewOps(
+ v ->
+ v.createView(
+ ident,
+ comment,
+ columns == null ? new Column[0] : columns,
+ representations,
+ defaultCatalog,
+ defaultSchema,
+ updatedProperties)),
+ NoSuchSchemaException.class,
+ ViewAlreadyExistsException.class);
+
+ // If the view is managed by Gravitino, we don't need to create ViewEntity
and store it again.
+ boolean isManagedView = isManagedEntity(catalogIdent,
Capability.Scope.VIEW);
+ if (isManagedView) {
+ return EntityCombinedView.of(catalogView)
+ .withHiddenProperties(
+ getHiddenPropertyNames(
+ catalogIdent,
+ HasPropertyMetadata::tablePropertiesMetadata,
+ catalogView.properties()));
+ }
+
+ AuditInfo audit =
+ AuditInfo.builder()
+ .withCreator(PrincipalUtils.getCurrentPrincipal().getName())
+ .withCreateTime(Instant.now())
+ .build();
+
+ ViewEntity viewEntity =
+ ViewEntity.builder()
+ .withId(uid)
+ .withName(ident.name())
+ .withNamespace(ident.namespace())
+ .withComment(comment)
+ .withColumns(columns == null ? new Column[0] : columns)
+ .withRepresentations(representations)
+ .withDefaultCatalog(defaultCatalog)
+ .withDefaultSchema(defaultSchema)
+ .withProperties(properties)
+ .withAuditInfo(audit)
+ .build();
+
+ try {
+ store.put(viewEntity, true /* overwrite */);
+ } catch (Exception e) {
+ LOG.error(FormattedErrorMessages.STORE_OP_FAILURE, "put", ident, e);
+ return EntityCombinedView.of(catalogView)
+ .withHiddenProperties(
+ getHiddenPropertyNames(
+ catalogIdent,
+ HasPropertyMetadata::tablePropertiesMetadata,
+ catalogView.properties()));
+ }
+
+ // Merge both the metadata from catalog operation and the metadata from
entity store.
+ return EntityCombinedView.of(catalogView, viewEntity)
+ .withHiddenProperties(
+ getHiddenPropertyNames(
+ catalogIdent,
+ HasPropertyMetadata::tablePropertiesMetadata,
+ catalogView.properties()));
}
- /**
- * Internal method to load view and check if it exists in entity store.
- *
- * @param ident The view identifier.
- * @return EntityCombinedView containing the view and import status.
- * @throws NoSuchViewException If the view does not exist.
- */
private EntityCombinedView internalLoadView(NameIdentifier ident) throws
NoSuchViewException {
- // Load view from the underlying catalog
- View catalogView =
+ NameIdentifier catalogIdentifier = getCatalogIdentifier(ident);
+ View view =
doWithCatalog(
- getCatalogIdentifier(ident),
+ catalogIdentifier,
c -> c.doWithViewOps(v -> v.loadView(ident)),
NoSuchViewException.class);
- // Check if view exists in entity store
- try {
- ViewEntity viewEntity = store.get(ident, Entity.EntityType.VIEW,
ViewEntity.class);
- return EntityCombinedView.of(catalogView, viewEntity).withImported(true);
- } catch (NoSuchEntityException e) {
- // View not in store yet
- LOG.debug("View {} not found in entity store", ident);
- return EntityCombinedView.of(catalogView).withImported(false);
- } catch (IOException ioe) {
- LOG.warn("Failed to check if view {} exists in entity store", ident,
ioe);
- return EntityCombinedView.of(catalogView).withImported(false);
+ boolean isManagedView = isManagedEntity(catalogIdentifier,
Capability.Scope.VIEW);
+ if (isManagedView) {
+ return EntityCombinedView.of(view)
+ .withHiddenProperties(
+ getHiddenPropertyNames(
+ catalogIdentifier,
+ HasPropertyMetadata::tablePropertiesMetadata,
+ view.properties()))
+ // The metadata of managed view is stored by Gravitino, so it is
always imported.
+ .withImported(true /* imported */);
+ }
+
+ StringIdentifier stringId = getStringIdFromProperties(view.properties());
+ // Case 1: The view is not created by Gravitino or the external system
does not support storing
+ // string identifier.
+ if (stringId == null) {
+ ViewEntity viewEntity = getEntity(ident, VIEW, ViewEntity.class);
+ if (viewEntity == null) {
+ return EntityCombinedView.of(view)
+ .withHiddenProperties(
+ getHiddenPropertyNames(
+ catalogIdentifier,
+ HasPropertyMetadata::tablePropertiesMetadata,
+ view.properties()))
+ // Some views don't have properties or are not created by
Gravitino,
+ // we can't use stringIdentifier to judge whether view is ever
imported or not.
+ // We need to check whether the entity exists.
+ .withImported(false);
+ }
+
+ return EntityCombinedView.of(view, viewEntity)
+ .withHiddenProperties(
+ getHiddenPropertyNames(
+ catalogIdentifier,
+ HasPropertyMetadata::tablePropertiesMetadata,
+ view.properties()))
+ // For some catalogs, the identifier information is not stored in
the view's
+ // metadata, we need to check if this view exists in the store, if
so we don't
+ // need to import.
+ .withImported(true);
}
+
+ ViewEntity viewEntity =
+ operateOnEntity(
+ ident,
+ identifier -> store.get(identifier, VIEW, ViewEntity.class),
+ "GET",
+ stringId.id());
+
+ return EntityCombinedView.of(view, viewEntity)
+ .withHiddenProperties(
+ getHiddenPropertyNames(
+ catalogIdentifier,
HasPropertyMetadata::tablePropertiesMetadata, view.properties()))
+ .withImported(viewEntity != null);
}
- /**
- * Import view into Gravitino entity store.
- *
- * @param ident The view identifier.
- * @return EntityCombinedView containing the view and import status.
- * @throws NoSuchViewException If the view does not exist.
- */
private EntityCombinedView importView(NameIdentifier ident) throws
NoSuchViewException {
- // Double-check if already imported (another thread might have imported
between locks)
EntityCombinedView entityCombinedView = internalLoadView(ident);
if (entityCombinedView.imported()) {
return entityCombinedView;
}
- LOG.info("Auto-importing view {} into Gravitino entity store", ident);
- long uid = idGenerator.nextId();
+ StringIdentifier stringId =
+
getStringIdFromProperties(entityCombinedView.viewFromCatalog().properties());
+
+ long uid;
+ if (stringId != null) {
+ // If the entity in the store doesn't match the external system, we use
the data
+ // of external system to correct it.
+ LOG.warn(
+ "The View uid {} existed but still need to be imported, this could
be happened "
Review Comment:
Grammar in the log message: "this could be happened" should be "this could
happen".
```suggestion
"The View uid {} existed but still need to be imported, this could
happen "
```
--
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]
