osscm commented on code in PR #9830:
URL: https://github.com/apache/iceberg/pull/9830#discussion_r3525692587
##########
spark/v3.5/spark-extensions/src/main/scala/org/apache/spark/sql/execution/datasources/v2/DropV2ViewExec.scala:
##########
@@ -30,6 +35,33 @@ case class DropV2ViewExec(catalog: ViewCatalog, ident:
Identifier, ifExists: Boo
override lazy val output: Seq[Attribute] = Nil
override protected def run(): Seq[InternalRow] = {
+ // If the catalog is a SparkCatalog, check for materialized view storage
table cleanup
+ catalog match {
+ case sparkCatalog: SparkCatalog =>
+ val icebergCatalog = sparkCatalog.icebergCatalog()
+ val icebergViewCatalog =
icebergCatalog.asInstanceOf[org.apache.iceberg.catalog.ViewCatalog]
+ var view: Option[View] = None
+ try {
+ val ns = Namespace.of(ident.namespace(): _*)
+ val viewId = TableIdentifier.of(ns, ident.name())
+ view = Some(icebergViewCatalog.loadView(viewId))
+ } catch {
+ case _: exceptions.NoSuchViewException =>
+ if (!ifExists) {
+ throw new NoSuchViewException(ident)
+ }
+ }
+ // if view is a materialized view, drop the storage table first
+ view.foreach { v =>
+ val storageTable = v.currentVersion().storageTable()
+ if (storageTable != null) {
+ val storageIdent =
Identifier.of(storageTable.namespace().levels(), storageTable.name())
+ sparkCatalog.dropTable(storageIdent)
+ }
+ }
+ case _ => // not a SparkCatalog, skip MV cleanup
+ }
+
val dropped = catalog.dropView(ident)
Review Comment:
What happens today if another materialized view depends on this view (either
as a base view, or as a nested MV)? Looking at this method, dropping proceeds
unconditionally — there's no check for dependents anywhere here.
Tracing it through: since nested view/MV dependencies aren't currently
tracked in freshness checks (see my comment on
`RefreshMaterializedViewExec.scala` line 128), a dependent MV would keep
reporting itself as fresh forever after this drop, silently serving orphaned
storage-table data — the only thing that would surface the problem is a later
`REFRESH MATERIALIZED VIEW`, which would fail once it tries to re-resolve the
dropped dependency in the SQL text.
I also checked the spec PR (#11041) — it defines the dependency graph and
how nested MVs get recorded in source-states, but doesn't say anything about
what should happen when a dependency gets dropped (block it? cascade? mark
dependents stale?). Might be worth resolving at the spec level first, since the
implementation can't really do the right thing here until that's defined.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
