HyukjinKwon commented on code in PR #59386:
URL: https://github.com/apache/spark/pull/59386#discussion_r4260468909
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/command/metricViewCommands.scala:
##########
@@ -78,6 +79,18 @@ case class CreateMetricViewCommand(
// `where`) into the user-supplied properties so v1 DESCRIBE TABLE
EXTENDED surfaces the
// same descriptor rows as the v2 path in `DataSourceV2Strategy`.
val mergedProps = properties ++ metricView.getProperties
+ if (replace && catalog.tableExists(name)) {
+ // Handles `CREATE OR REPLACE VIEW ... WITH METRICS` on an existing
view, the same way
+ // `CreateViewCommand` does: nothing is retained from the old view, so
drop and recreate.
+ val existing = catalog.getTableMetadata(name)
+ if (!existing.isViewLike) {
+ throw
QueryCompilationErrors.unsupportedCreateOrReplaceViewOnTableError(
+ name.nameParts, replace)
+ }
+ ViewHelper.checkCyclicViewReference(analyzed, Seq(existing.fullIdent),
existing.fullIdent)
+ CommandUtils.uncacheTableOrView(sparkSession, existing.identifier)
+ catalog.dropTable(existing.identifier, ignoreIfNotExists = false, purge
= false)
Review Comment:
**Nit (P3):** The existing view is uncached and dropped here, but
`ViewHelper.prepareTable(...)` only runs afterwards as the `createTable`
argument, and it can still reject the new definition. `generateViewProperties`
calls `SchemaUtils.checkColumnNameDuplication`, and nothing earlier on this
path checks that names are unique (`validateUserColumns` only compares counts).
So `CREATE OR REPLACE VIEW mv (a, a) WITH METRICS ...` against an existing
`mv`, or a YAML body with two `select` columns of the same name, fails with
`COLUMN_ALREADY_EXISTS` after `mv` has already been dropped, and the old metric
view is gone. Before this PR that statement failed without touching `mv`, and
the v2 path (`V2CreateViewPreparation.run`) builds the view info before calling
`createOrReplaceView`.
Could you build the `CatalogTable` with `prepareTable` into a local `val`
before the `if (replace && catalog.tableExists(name))` block, and then pass it
to `createTable` after the uncache/drop? `prepareTable` doesn't read the old
view, so moving it up changes nothing else.
**Verification:**
- **Regression:** With an existing session-catalog metric view, CREATE OR
REPLACE VIEW ... WITH METRICS with duplicate output column names fails with
COLUMN_ALREADY_EXISTS, and querying the metric view afterwards still returns
its original results.
- **Behavior:** Existing coverage for successful replace, table rejection,
and direct/indirect metric-view cycles still passes unchanged.
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/command/views.scala:
##########
@@ -616,20 +617,27 @@ object ViewHelper extends SQLConfHelper with Logging with
CapturesConfig {
plan: LogicalPlan,
path: Seq[Seq[String]],
viewIdent: Seq[String]): Unit = {
- plan match {
- case v: View =>
- val ident = v.desc.fullIdent
+ // A metric view read by the plan is a `ResolvedMetricView`, or a
`MetricViewPlaceholder`
+ // when it is the source of another metric view, not a `View`.
+ val viewRef = plan match {
+ case v: View => Some(v.desc.fullIdent)
+ case mv: ResolvedMetricView => Some(mv.identifier.nameParts)
Review Comment:
**Nit (P3):** This arm compares `mv.identifier.nameParts`, but
`ResolvedMetricView.identifier` is the legacy `TableIdentifier` taken from the
placeholder's `CatalogTable`. For a metric view read from a non-session v2
`ViewCatalog`, `V1Table.toCatalogTable` builds it with
`asLegacyTableIdentifier`, which keeps only the last namespace segment; the
full name lives only in `multipartIdentifier`. The `View` and
`MetricViewPlaceholder` arms, and every caller's `viewIdent`, use the full
identifier, as the `@param viewIdent` doc above describes.
With nested namespaces this goes wrong both ways in the v2
`CheckViewReferences` path. `CREATE OR REPLACE VIEW cat.b.v AS SELECT
measure(m) FROM cat.a.b.v` now fails with a spurious `RECURSIVE_VIEW`, because
both sides are `[cat, b, v]`. Replacing `cat.a.b.v` with a view that reads
itself through `measure()` is still not detected. Session-catalog identifiers
are unaffected, which is why the new tests pass.
Could `ResolvedMetricView` carry the metric view's full multi-part
identifier (e.g. `mv.metadata.fullIdent` when `ResolveMetricView` builds it),
so this arm compares the same shape as the other two?
**Verification:**
- **Regression:** In a v2 ViewCatalog with nested namespaces, CREATE OR
REPLACE VIEW of a single-level view whose body reads a same-named metric view
in a deeper namespace through measure() succeeds instead of raising
RECURSIVE_VIEW.
- **Regression:** In the same catalog, replacing a nested-namespace metric
view with a regular view that reads it through measure() fails with
RECURSIVE_VIEW and names the full multi-part identifier.
- **Compatibility:** The session-catalog metric-view cycle tests keep their
RECURSIVE_VIEW condition and rendered paths.
--
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]
