[SPARK-54119] Support METRIC_VIEW creation on V2 catalogs

[chenwang-databricks]

What changes were proposed in this pull request?

This PR extends metric-view support to DS v2 catalogs by routing CREATE VIEW ... WITH METRICS through the ViewCatalog / TableViewCatalog APIs introduced by SPARK-52729 and finalized by SPARK-56655. Third-party v2 catalogs that implement ViewCatalog can now host metric views with the same metadata fidelity as session-catalog metric views.

1. V2 metric-view CREATE path -- shared with CreateV2ViewExec. A new CreateV2MetricViewExec and CreateV2ViewExec both extend a new V2CreateViewPreparation trait (which itself extends V2ViewPreparation). The trait owns the shared CREATE-side run(): viewExists short-circuit on IF NOT EXISTS, createOrReplaceView for OR REPLACE, and cross-type collision decoding (ViewAlreadyExistsException -> tableExists -> EXPECT_VIEW_NOT_TABLE.NO_ALTERNATIVE). The metric-view subclass only supplies the metric-view-specific bits (no collation, schema-mode UNSUPPORTED, typed viewDependencies, PROP_TABLE_TYPE = METRIC_VIEW, retainColumnMetadata = true) via optional hooks on V2ViewPreparation. DataSourceV2Strategy intercepts CreateMetricViewCommand on a non-session catalog and routes to the new exec; the v1 session-catalog path stays in CreateMetricViewCommand.run.

2. First-class METRIC_VIEW table type.

• CatalogTableType.METRIC_VIEW is added alongside EXTERNAL / MANAGED / VIEW.
• TableSummary.METRIC_VIEW_TABLE_TYPE = "METRIC_VIEW" constant for the V2 surface.
• The previous view.viewWithMetrics property hack is removed; CatalogTable.isMetricView checks tableType == METRIC_VIEW directly.
• V1Table.summarizeTableType and V1Table.toCatalogTable(catalog, ident, ViewInfo) translate between the V2 property form and the V1 enum.
• HMS round-trip support: HiveTableType has no METRIC_VIEW variant (both regular views and metric views serialize as VIRTUAL_VIEW). HiveExternalCatalog now persists a view.subType = METRIC_VIEW property on write and lifts tableType back to METRIC_VIEW on read, so HMS-backed metric views survive the round trip.

3. Repo-wide tableType == VIEW audit + CatalogTable.isViewLike helper. Promoting metric views to a distinct CatalogTableType opens silent regressions wherever existing code branches on VIEW. To consolidate the audit and reduce divergence with the Databricks Runtime (which has the same helper), this PR introduces:

• CatalogTable.isViewLike instance method (DBR parity: today returns tableType == VIEW || tableType == METRIC_VIEW; forks may extend the set).
• CatalogTable.isViewLike(t: CatalogTableType) companion form for the few sites that have a CatalogTableType but no CatalogTable (e.g. SessionCatalog.isView, verifyAlterTableType, HiveClientImpl.toHiveTableType).

All 18 sites in catalyst / core / hive that previously did inline tableType == VIEW || tableType == METRIC_VIEW (or the CatalogTableType.VIEW | CatalogTableType.METRIC_VIEW pattern alternation) are now routed through these helpers, so adding a new view-like type in the future is a one-line change in the helper body. Notable touched call sites: CatalogTable.toJsonLinkedHashMap (DESCRIBE EXTENDED rows), HiveExternalCatalog.{createTable, alterTable, restoreTableMetadata}, HiveClientImpl.toHiveTableType, SessionCatalog.isView, InMemoryCatalog.listViews, RelationResolution, Analyzer.lookupTableOrView, rules.scala, DataStreamWriter, DescribeRelationJsonCommand, AnalyzeColumnCommand, AnalyzePartitionCommand, CommandUtils.analyzeTable, V2SessionCatalog.dropTableInternal, verifyAlterTableType in ddl.scala, and 3 sites in tables.scala.

Explicit rejection (uniform error class): SHOW CREATE TABLE on a metric view has no round-trippable CREATE VIEW ... WITH METRICS form, so it's rejected explicitly with the dedicated UNSUPPORTED_SHOW_CREATE_TABLE.ON_METRIC_VIEW error class on both the v1 session-catalog path (in tables.scala) and the v2 catalog path (in DataSourceV2Strategy), so users see the same actionable message regardless of catalog kind.

4. Drop-command parity.

• DropTableCommand (v1 path) treats both VIEW and METRIC_VIEW as views: DROP TABLE rejects either with wrongCommandForObjectTypeError, and DROP VIEW accepts either.
• V2SessionCatalog.dropTableInternal extends the existing "view rejected from DROP TABLE" guard to cover METRIC_VIEW.
• For non-session v2: DropTableExec (post-SPARK-56655) actively rejects with WRONG_COMMAND_FOR_OBJECT_TYPE ("Use DROP VIEW instead") when a view sits at the ident -- works unchanged for metric views since TableViewCatalog's default viewExists derives from loadTableOrView and recognizes MetadataTable + ViewInfo.
• ResolveSessionCatalog's DropView routing comment is clarified: v2 metric views fall through to DataSourceV2Strategy and ViewCatalog.dropView.

5. Typed view dependencies (ViewInfo.viewDependencies).

• New public DTOs in org.apache.spark.sql.connector.catalog: Dependency (sealed interface with Dependency.table(String[]) / Dependency.function(String[]) non-vararg factories), TableDependency, FunctionDependency, DependencyList(Dependency[]).
• TableDependency and FunctionDependency carry the dependency identifier as structural multi-part name parts (record TableDependency(String[] nameParts)), not a single dot-flattened string. Arity is preserved per source so multi-level-namespace V2 catalogs (e.g. Iceberg cat.db1.db2.tbl -> 4 parts) round-trip without ambiguity against quoted identifiers containing literal .. v1 sources resolved through the session catalog are normalized by a new MetricViewHelper.qualifyV1 to a stable 3-part [spark_catalog, db, table] shape so consumers see deterministic arity per source kind (otherwise TableIdentifier.nameParts could return 1, 2, or 3 parts depending on what the analyzer captured).
• All three records (TableDependency, FunctionDependency, DependencyList) override equals / hashCode / toString using Arrays.equals / Arrays.hashCode / Arrays.toString to give value semantics on their array fields. Without the overrides, records' auto-generated methods on array fields fall through to Object.equals (reference equality), which would make structural multi-part names unusable as Map keys / for dedup. Each record also overrides the canonical accessor to return a defensive clone() so callers cannot mutate the record's internal array.
• ViewInfo gains a viewDependencies field and a ViewInfo.Builder.withViewDependencies(...) setter. Per the field's contract, null means "no dependency list was supplied" while an empty DependencyList.of(new Dependency[0]) means "supplied but the object has none" -- metric-view CREATE always emits the latter, never the former, even when collectTableDependencies returns empty.
• MetricViewHelper.collectTableDependencies walks the analyzed plan and emits structural Seq[Seq[String]] parts; the v2 source arm preserves full namespace arity, the v1 source arms (View, HiveTableRelation, LogicalRelation) all route through qualifyV1 for the stable 3-part shape.

6. Multi-level-namespace targets for v2 metric views. MetricViewHelper.analyzeMetricViewText previously required a TableIdentifier, capping the metric-view target at 3 name parts. v2 metric views with multi-level-namespace targets (e.g. cat.db1.db2.mv) failed at ident.asTableIdentifier with requiresSinglePartNamespaceError. The helper now takes nameParts: Seq[String] directly; call sites in both the v1 path (CreateMetricViewCommand) and the v2 path (DataSourceV2Strategy) updated. The helper now also returns (LogicalPlan, MetricView) so callers don't have to re-parse the YAML body just to read descriptor properties.

7. metric_view.* descriptor properties (v1/v2 parity). MetricView.getProperties produces canonical descriptive properties (metric_view.from.type, metric_view.from.name / metric_view.from.sql, metric_view.where) that both the v1 path (CreateMetricViewCommand.createMetricViewInSessionCatalog) and the v2 path (DataSourceV2Strategy) merge into the view's properties bag, so catalog browsers and tooling see the same descriptor rows in DESCRIBE TABLE EXTENDED regardless of catalog kind. Long values are truncated to Constants.MAXIMUM_PROPERTY_SIZE; the Scaladoc on getProperties calls out that metric_view.from.sql is therefore a descriptive value, not a round-trippable representation -- consumers should re-read the YAML body for the full SQL.

8. ViewInfo constructor cleanup. The metric-view-specific PROP_TABLE_TYPE = METRIC_VIEW special case is dropped from the generic ViewInfo constructor in favor of properties().putIfAbsent(...). Callers that want a more specific kind (e.g. METRIC_VIEW) call BaseBuilder.withTableType(...) before build() -- exercised by CreateV2MetricViewExec via the new V2ViewPreparation.tableType hook.

9. ViewHelper.aliasPlan(retainMetadata). The user-specified-column-with-comment branch in aliasPlan previously dropped existing column metadata. A new retainMetadata: Boolean = false parameter merges the analyzed attribute's metadata into the new comment metadata. ViewHelper.prepareTable passes retainMetadata = isMetricView (v1 path); V2ViewPreparation exposes a retainColumnMetadata hook that CreateV2MetricViewExec overrides to true (v2 path). Both preserve the per-column metric_view.type / metric_view.expr keys that the analyzer attaches to dimensions and measures even when the user renames columns and adds comments.

10. Error classes.

• New INVALID_METRIC_VIEW_YAML (sqlState 42K0L). MetricViewPlanner.parseYAML's catch blocks now route through QueryCompilationErrors.invalidMetricViewYamlError instead of SparkException.internalError, so a typo in the user's YAML body surfaces as a user-correctable AnalysisException rather than "please contact support".
• New UNSUPPORTED_SHOW_CREATE_TABLE.ON_METRIC_VIEW (sqlState 0A000), used by both the v1 session-catalog path and the v2 catalog path so SHOW CREATE TABLE on a metric view produces the same actionable message regardless of catalog kind.

11. Misc.

• MetricViewCanonical.parseSource accepts multipart identifiers (parseMultipartIdentifier) so 3-part catalog.schema.table source references work as AssetSource.

Why are the changes needed?

Before this PR, metric-view DDL only worked against the session catalog: the create path called SessionCatalog.createTable directly, and there was no way for a third-party v2 catalog (Unity Catalog, Hive Metastore catalog, custom REST catalogs, etc.) to own a metric view's lifecycle. SPARK-52729 / SPARK-56655 shipped ViewCatalog and TableViewCatalog as the public v2 surface for catalog-managed views; metric views are a kind of view and naturally belong on this surface.

Once metric views can live on a v2 catalog, two more constraints surface:

1. Type discriminator. A consumer reading a row through ViewCatalog.loadView needs to know it's a metric view, not a plain SQL view, so it can render the right UI / planner output. Encoding this in PROP_TABLE_TYPE = METRIC_VIEW keeps the distinction wire-compatible and lets V1Table.toCatalogTable reconstruct CatalogTableType.METRIC_VIEW on the read path.
2. Structured dependency lineage. Metric views always reference at least one source table; cataloging that lineage as flat string properties or single dot-joined strings loses arity for multi-level namespaces and is ambiguous against quoted identifiers. A typed DependencyList of TableDependency / FunctionDependency with structural String[] nameParts lets catalogs persist the lineage as a first-class field with full fidelity.

The remaining changes (drop-command parity, aliasPlan metadata retention, metric_view.* properties, parseMultipartIdentifier, tableType == VIEW audit + isViewLike helper, multi-level-namespace lift, HMS round-trip marker) are mechanical follow-ups that fall out of supporting metric views as a real CatalogTableType and as a v2 catalog citizen -- without them, basic operations like DROP VIEW, DESCRIBE TABLE EXTENDED, CREATE VIEW (a COMMENT 'c') WITH METRICS ..., or CREATE VIEW cat.db1.db2.mv WITH METRICS ... would silently degrade.

Does this PR introduce any user-facing change?

Yes, both for end users and for catalog plugin developers:

End users:

• CREATE VIEW <ident> WITH METRICS ... now works against any v2 catalog that implements ViewCatalog, including catalogs with multi-level namespace targets. Previously it was rejected with MISSING_CATALOG_ABILITY.VIEWS for non-session catalogs, and capped at single-level namespaces. IF NOT EXISTS and OR REPLACE are honored on the v2 path (regression vs. v1 fixed).
• A v2 metric view can be queried with SELECT region, measure(count_sum) FROM <mv> ..., dropped with DROP VIEW, listed via SHOW VIEWS (and via SHOW TABLES on a TableViewCatalog, matching v1 SHOW TABLES output), and described with DESCRIBE TABLE / DESCRIBE TABLE EXTENDED. DROP TABLE on a metric view throws WRONG_COMMAND_FOR_OBJECT_TYPE ("Use DROP VIEW instead").
• ALTER VIEW <metric_view> RENAME TO ... is wired through RenameV2ViewExec and preserves the metric-view kind across the rename.
• SHOW CREATE TABLE on a metric view throws UNSUPPORTED_SHOW_CREATE_TABLE.ON_METRIC_VIEW (no round-trippable form yet) on both the v1 and v2 paths -- same error class regardless of catalog kind.
• Session-catalog metric views are now stored as CatalogTableType.METRIC_VIEW instead of CatalogTableType.VIEW + view.viewWithMetrics=true. Observable in DESCRIBE TABLE EXTENDED's Type row and the tableType column of the tables system table. SQL behavior is unchanged. Hive-metastore-backed metric views also round-trip through HMS via a view.subType = METRIC_VIEW property marker.
• DESCRIBE TABLE EXTENDED on metric views (v1 and v2) now consistently surfaces metric_view.from.type / metric_view.from.name / metric_view.from.sql / metric_view.where descriptor rows.
• Error messages from DROP TABLE / DROP VIEW mismatch now mention METRIC_VIEW alongside VIEW.
• Malformed metric-view YAML now surfaces as INVALID_METRIC_VIEW_YAML (user-correctable) instead of "Spark internal error, please contact support".

Catalog plugin developers:

• New public API surface in org.apache.spark.sql.connector.catalog: sealed interface Dependency with permits TableDependency, FunctionDependency, both records carrying String[] nameParts. Dependency.table(String[]) / Dependency.function(String[]) static factories (non-vararg per review; callers pass an existing array directly). DependencyList(Dependency[]) with DependencyList.of(Dependency[]) factory. All three records override equals / hashCode / toString to give value semantics on their array fields, and the canonical accessors return a defensive clone() so internal state is not mutable through the public API. All @Evolving, @since 4.2.0. Note: today's only producer in Spark itself is metric-view dependency extraction, which emits TableDependency only; FunctionDependency and Dependency.function(...) are exposed as groundwork for future producers (e.g. SQL UDF dep tracking).
• ViewInfo gains a typed viewDependencies() accessor and ViewInfo.Builder.withViewDependencies(...) setter. viewDependencies is populated only on the non-session v2 CREATE path; v1 metric views (and v2 metric views read back through V1Table.toCatalogTable) carry null. Catalog plugin authors persisting dependency lineage should treat the field as v2-only for now -- broadening to v1 is a tracked follow-up.
• TableSummary.METRIC_VIEW_TABLE_TYPE = "METRIC_VIEW" constant.
• CatalogTableType.METRIC_VIEW enum value (v1 surface).
• CatalogTable.isViewLike instance + CatalogTable.isViewLike(CatalogTableType) companion helpers (DBR parity helpers for "does this table behave like a view at resolution / DDL time?"). Forks that add their own view-like types (e.g. DBR's MATERIALIZED_VIEW, STREAMING_TABLE) only need to extend the helper body.
• V2ViewPreparation (private to org.apache.spark.sql.execution.datasources.v2) gains optional viewDependencies / tableType / retainColumnMetadata hooks.

How was this patch tested?

MetricViewV2CatalogSuite -- 31 tests across 5 sections, all against an in-memory ViewCatalog test fixture (MetricViewRecordingCatalog extends InMemoryTableCatalog with TableViewCatalog):

Section 1 -- CREATE-related (11 tests):

• V2 catalog receives METRIC_VIEW table type and view text via ViewInfo.
• V2 catalog path populates metric_view.* descriptor properties + view context (currentCatalog / currentNamespace) + captured SQL configs.
• V2 catalog path captures SQLSource and comment.
• Metric view columns carry metric_view.type / metric_view.expr in column metadata.
• User-specified column names with comments preserve metric_view.* metadata (pins the aliasPlan(retainMetadata = true) fix).
• CREATE OR REPLACE VIEW ... WITH METRICS replaces an existing v2 metric view (asserts on the replacement's distinguishing fields: queryText, metric_view.where, dependencies).
• CREATE VIEW IF NOT EXISTS ... WITH METRICS is a no-op when the view exists (catalog never sees the second createView call).
• CREATE VIEW ... WITH METRICS over a v2 table at the ident throws TABLE_OR_VIEW_ALREADY_EXISTS (analyzer-time pre-check).
• CREATE VIEW IF NOT EXISTS ... WITH METRICS is a no-op when a v2 table sits at the ident (v1 parity).
• CREATE VIEW ... WITH METRICS on a non-ViewCatalog catalog fails with MISSING_CATALOG_ABILITY.VIEWS.
• CREATE VIEW ... WITH METRICS at a multi-level-namespace v2 target (testcat.ns_a.ns_b.mv_deep) succeeds (pins the analyzeMetricViewText lift to Seq[String]).

Section 2 -- Dependency extraction (5 tests):

• SQL source JOIN captures both tables as 3-part nameParts.
• SQL source subquery deduplicates same-table references.
• SQL source self-join deduplicates same-table references.
• V1 session-catalog source emits exactly 3 parts, normalized to [spark_catalog, db, table] by qualifyV1.
• Multi-level V2 namespace source (testcat.ns_a.ns_b.events_deep) emits 4-part nameParts.

Section 3 -- SELECT cases (5 tests, modeled on MetricViewSuite patterns):

• SELECT measure(count_sum) FROM <mv> GROUP BY region returns aggregated rows (exercises the full loadTableOrView -> MetadataTable(ViewInfo) -> V1Table.toCatalogTable(ViewInfo) -> ResolveMetricView round-trip).
• SELECT measure(...) WHERE region = ... -- query-layer filter on top of the view.
• View's pre-defined where clause is applied (where = Some("count > 1") filters at view-resolution time).
• Multiple measures with different aggregations (sum / sum / max).
• ORDER BY measure(...) DESC LIMIT 1 over the metric view.

Section 4 -- DESCRIBE cases (2 tests):

• DESCRIBE TABLE EXTENDED round-trips through loadTableOrView and emits the View Text / Type rows (gated through CatalogTable.isViewLike in toJsonLinkedHashMap, which now recognizes METRIC_VIEW).
• DESCRIBE TABLE (non-EXTENDED) returns the aliased columns.

Section 5 -- DROP / SHOW / RENAME cases (8 tests):

• DROP VIEW succeeds on a v2 metric view.
• DROP VIEW IF EXISTS on a non-existent v2 metric view is a no-op.
• DROP TABLE on a v2 metric view throws WRONG_COMMAND_FOR_OBJECT_TYPE ("Use DROP VIEW instead", per SPARK-56655's DropTableExec) and asserts the metric view is not deleted.
• DROP TABLE IF EXISTS on a v2 metric view also throws (IF EXISTS doesn't silence the wrong-type error, v1 parity).
• SHOW CREATE TABLE on a v2 metric view throws UNSUPPORTED_SHOW_CREATE_TABLE.ON_METRIC_VIEW (same dedicated error class as the v1 path).
• SHOW TABLES on a TableViewCatalog lists both tables and metric views (matches v1 SHOW TABLES output per SPARK-56655).
• SHOW VIEWS lists v2 metric views.
• ALTER VIEW <metric_view> RENAME TO ... succeeds and preserves the metric-view kind across the rename (pins RenameV2ViewExec end-to-end against the fixture's renameView).

Existing session-catalog metric-view tests (MetricViewSuite, SimpleMetricViewSuite, HiveMetricViewSuite) and v1 path tests pass unchanged. DDLSuite and HiveDDLSuite had their tableTypes enumerations updated to include METRIC_VIEW in two assertion lists. PlanResolutionSuite test fixture was updated to stub the new CatalogTable.isViewLike method on the Mockito mock.

Was this patch authored or co-authored using generative AI tooling?

Generated-by: Cursor (Claude Sonnet 4.7)

[cloud-fan]

Thanks for extending metric views to v2 catalogs. The shape of the change is right — route through ViewCatalog.createView with PROP_TABLE_TYPE = METRIC_VIEW and a typed viewDependencies, and promote METRIC_VIEW to a first-class CatalogTableType. A few significant concerns:

1. v2 path forks instead of reusing V2ViewPreparation and changes behavior on IF NOT EXISTS / OR REPLACE. CreateMetricViewCommand.createMetricViewInV2Catalog is open-coded inside a v1 RunnableCommand and duplicates most of V2ViewPreparation.buildViewInfo, but with materially different behavior: allowExisting and replace are silently dropped on the v2 path (regression vs. v1, which honored ignoreIfExists), there's no viewExists short-circuit, no createOrReplaceView, no cross-type collision decoding (so a table sitting at the ident leaks ViewAlreadyExistsException instead of unsupportedCreateOrReplaceViewOnTableError), and the CharVarcharUtils / SchemaUtils.checkColumnNameDuplication / checkIndeterminateCollationInSchema / owner-stamping work that V2ViewPreparation does is missing. Refactoring this into a CreateV2MetricViewExec (or extending V2ViewPreparation with a viewDependencies / extraProperties hook) plus a DataSourceV2Strategy rule would close all of these gaps in one move.

2. Audit other tableType == VIEW gates for METRIC_VIEW. Promoting metric views to a distinct CatalogTableType opens up silent regressions wherever existing code branches on VIEW. Two I'm fairly sure about: CatalogTable.toJsonLinkedHashMap (interface.scala:670) gates View Text / View Original Text / View Schema Mode / View Catalog and Namespace / SQL Path on tableType == VIEW, so DESCRIBE TABLE EXTENDED on a metric view will lose all of those rows — visible regression vs. the pre-PR viewWithMetrics storage. And HiveExternalCatalog.{createTable, alterTable, restoreTableMetadata} (lines 277, 297, 598, 838) only special-case VIEW, so a Hive-metastore-backed session-catalog metric view will go through the wrong branches on persist and read-back. Tests don't cover Hive-backed metric views, so this would only surface in production. Worth grepping tableType == CatalogTableType.VIEW and tableType == VIEW repo-wide and deciding which sites also need to accept METRIC_VIEW.

3. TableDependency.tableFullName is a single dot-joined string but the Javadoc claims a fixed three-part shape. This breaks down two ways: (a) MetricViewHelper.collectTableDependencies uses unquotedString for V1 sources, which yields a 2-part db.tbl whenever TableIdentifier.catalog is unset, and mkString(".") for V2 sources, which yields N-part names for catalogs with multi-level namespaces (e.g., Iceberg cat.db1.db2.tbl is 4 parts) — so the producer doesn't honor the contract; (b) any consumer parsing the string back via split/join loses arity and is ambiguous against quoted identifiers containing a literal .. Since this is brand-new public API, it's the right time to make the storage structural — e.g., TableDependency(String[] nameParts) — rather than committing to a string the producer can't always honor.

4. MetricViewPlanner.parseYAML raises SparkException.internalError for malformed user YAML. Replacing invalidLiteralForWindowDurationError is right, but internalError is the wrong category — a typo in the user's YAML body shouldn't surface as "Spark internal error, please contact support." Should be a QueryCompilationErrors / AnalysisException with a real error class.

Minor stuff (typed DTO sealing/mutability, the ViewInfo constructor's metric-view special case, missing tests for OR REPLACE / IF NOT EXISTS / V1-source dependency / multi-level-namespace V2 source / read-back through loadRelation) is in the inline comments below.

[cloud-fan]

Round 2 review.

Status: 13 prior findings addressed (4 main concerns + 9 inline), 0 remaining, 4 new (3 newly introduced in commits since the prior review at 8f62efc6, 1 late catch). The shared V2ViewPreparation / V2CreateViewPreparation refactor reads cleanly, the typed Dependency / DependencyList API replaces the dot-joined-string form, the INVALID_METRIC_VIEW_YAML error is in place, and the tableType == VIEW audit covers most sites. Four follow-ups inline.

[cloud-fan]

Round 3 review.

Status: All 4 prior findings addressed (verifyAlterTableType audit miss, StructField/Column public-API split, Column.fromStructField comment-duplication via revert, multi-level namespace lift in analyzeMetricViewText), 0 remaining, 7 new (1 newly introduced, 6 late catches). The round 3 follow-up commit cleanly addresses the round 2 findings; remaining concerns center on v1/v2 inconsistencies (descriptive properties, error class, dependency-name arity) and API correctness of the new Dependency records.

I'll be honest about the 6 late catches: most have been latent since round 1 — the structural-form switch fixed the format dimension I flagged but not the arity dimension; the records I suggested have a value-semantics issue I missed; the metric_view.* merge has been on the v2 path since the original PR. Apologies for not surfacing these earlier.

[cloud-fan]

The v2 path merges metric_view.from.type / metric_view.from.name / metric_view.from.sql / metric_view.where into the property bag here (properties ++ metricView.getProperties), but the v1 path in CreateMetricViewCommand.createMetricViewInSessionCatalog (metricViewCommands.scala:74-78) passes properties to prepareTable without the merge. Result: DESCRIBE TABLE EXTENDED on a session-catalog metric view doesn't show these descriptor rows; on a v2 metric view, it does.

The PR description states the merge happens "into the view's properties bag" without a v1/v2 carve-out — please confirm this is intentional, or merge in v1 too.

[cloud-fan]

The v1 path uses the dedicated UNSUPPORTED_SHOW_CREATE_TABLE.ON_METRIC_VIEW (tables.scala:1219 → showCreateTableNotSupportedOnMetricViewError); this v2 path uses the generic UNSUPPORTED_FEATURE.TABLE_OPERATION. Same user action, two error classes — the dedicated one carries a more actionable message ("not supported on metric view"). showCreateTableNotSupportedOnMetricViewError takes a String; you can format the multipart name the same way the case below does ((catalog.name() +: ident.asMultipartIdentifier).map(quoteIfNeeded).mkString(".")).

[cloud-fan]

TableIdentifier.nameParts returns 1, 2, or 3 parts depending on which of catalog / database are set on the resolved CatalogTable.identifier. The same v1 source table can yield [db, tbl] or [spark_catalog, db, tbl] across runs — the test in MetricViewV2CatalogSuite (the V1 source dependency-extraction test) admits this with parts.length >= 2 && parts.length <= 3. The TableDependency Javadoc (TableDependency.java:32-33) acknowledges it, but downstream consumers comparing dependency names structurally won't be able to rely on stable arity. Same issue applies to the View arm (line 105) and HiveTableRelation arm (line 112).

Consider normalizing v1 sources to always emit 3-part [spark_catalog, db, tbl] (prepending the session-catalog name when missing) so consumers see a stable shape per source kind. The test should then assert exactly 3 parts.

[cloud-fan]

MetricViewHelper.analyzeMetricViewText (called on line 345) already parses the YAML internally via MetricViewPlanner.parseYAML → MetricViewFactory.fromYAML. Re-parsing here duplicates the work. analyzeMetricViewText could return (LogicalPlan, MetricView) (or expose the parsed MetricView via the MetricViewPlaceholder it builds) so the strategy doesn't pay the parse twice.

[cloud-fan]

Extracting r.catalog.asTableCatalog into tableCatalog is a no-op refactor unrelated to metric-view support and is not mentioned in the PR description. Please revert or split into a separate cleanup PR — keeps the metric-view PR focused.

[cloud-fan]

Java records auto-generate equals/hashCode from per-field Objects.equals/Objects.hashCode, which on array fields fall through to Object.equals (reference equality). So DependencyList.of(Dependency.table("a","b")).equals(DependencyList.of(Dependency.table("a","b"))) == false. Same issue on TableDependency.nameParts (TableDependency.java:42) and FunctionDependency.nameParts (FunctionDependency.java:34).

This undermines the "structural multi-part name" intent — consumers can't dedup, compare, or use these as Map keys. Please override equals/hashCode to use Arrays.equals/Arrays.hashCode on each record (and Arrays.deepEquals/Arrays.deepHashCode on DependencyList), or expose List<...> instead of [] to inherit value semantics for free.

[cloud-fan]

Two test comments describe behavior in terms of the in-PR change rather than the current invariant — they age poorly post-merge:

• Lines 253-254: // ... Without the retainMetadatafix toViewHelper.aliasPlan, the metric_view.* keys disappear here. — once merged, there is no "fix" to refer to. Rephrase as a pin: // Pins aliasPlan(retainMetadata=true): metric_view.* keys must survive a column rename with comments.
• Lines 749-752 (DESCRIBE EXTENDED test): // The "Type" row was added alongside this metric-view PR so DescribeV2ViewExec matches v1 parity... — same problem. Rephrase to describe the current invariant being pinned (e.g. DescribeV2ViewExec emits a "Type" row matching v1 parity, so users can distinguish VIEW vs METRIC_VIEW.).

[cloud-fan]

Round 4 review.

Status: All 7 prior findings addressed (SHOW CREATE TABLE error class parity, metric_view.* v1/v2 parity, Dependency records value-equality via List<>, v1 source dep normalization through qualifyV1, single YAML parse, DropTable revert, test comment phrasing), 0 remaining, 6 new (3 newly introduced in 437b71d79e, 3 late catches I missed earlier). The round-3 follow-up cleanly closes the round-3 findings; remaining concerns are mostly small (text-quality, test gap) plus a few design-level questions worth answering inline before merge.

Apologies again for the late catches -- the FunctionDependency unused-API and the cyclic-view-check skip have been latent since round 1, and the DBR terminology leak has been there since the original commit.

Followup-able (not blocking, worth a tracking ticket):

• ALTER VIEW <metric_view> AS SELECT ... is silently accepted by AlterV2ViewExec -- existingView.properties carry PROP_TABLE_TYPE = METRIC_VIEW through, so the view ends up classified as METRIC_VIEW with a plain-SQL body; the next SELECT blows up with INVALID_METRIC_VIEW_YAML. Same hole exists on the v1 path (AlterViewAsCommand.alterPermanentView preserves tableType via viewMeta.copy). Not introduced by this PR, but promoting METRIC_VIEW to a first-class type makes the inconsistency more visible.
• viewDependencies is only populated on the non-session v2 path -- v1 metric views (and v2 metric views read back through V1Table.toCatalogTable) carry null. Probably the right scope cap for this PR, but worth a one-line note in the description so catalog plugin developers know not to rely on it for the v1 surface.
• No test for ALTER VIEW <metric_view> RENAME TO ... even though RenameV2ViewExec is in the strategy and the fixture's renameView is implemented. Coverage gap.

[cloud-fan]

The empty-depParts case maps to None here, which becomes null on ViewInfo.viewDependencies(). Per the Javadoc on ViewInfo#viewDependencies, null means "no dependency list was supplied" while an empty list means "provided but the object has none." For a metric view we always compute deps, so when collectTableDependencies returns empty (e.g. SQLSource("SELECT 1 AS x")) the right value is Some(DependencyList.of()), not None.

Suggested change

	val deps = if (depParts.nonEmpty) {
	Some(DependencyList.of(
	depParts.map(parts => Dependency.table(parts: _*)): _*))
	} else None
	val deps = Some(DependencyList.of(
	depParts.map(parts => Dependency.table(parts: _*)): _*))

[cloud-fan]

FunctionDependency is in the sealed permits list and exposed via the Dependency.function(...) factory below, but no producer in this PR ever emits one -- MetricViewHelper.collectTableDependencies only emits TableDependency. Two options: (a) drop FunctionDependency until it has a producer (the @Evolving annotation is meant to evolve before stabilizing, so adding it later is cheap); (b) keep it as groundwork but mention in the PR description so reviewers don't trip on the dead surface, and add a sentence to this class-level Javadoc noting that consumers may receive only TableDependency instances today.

[cloud-fan]

Missing comma -- as written, "session catalog producers" reads as a compound noun.

Suggested change

	* catalog producers should normalize to {@code [spark_catalog, db, table]} so consumers see
	* catalog, producers should normalize to {@code [spark_catalog, db, table]} so consumers see

[cloud-fan]

analyzeMetricViewText runs verifyTemporaryObjectsNotExists but skips the cyclic-view-reference check that plain CREATE VIEW gets via CheckViewReferences.checkCyclicViewReference (only applied on replace). For CREATE OR REPLACE VIEW <mv> WITH METRICS LANGUAGE YAML AS $$ from: <mv> ... $$ the cycle currently surfaces only on the next SELECT. Pre-existing on the v1 path too (the old createMetricViewInSessionCatalog didn't call it either), so this is a follow-up rather than a blocker for this PR -- but wiring it in here means both v1 and v2 metric-view CREATE paths benefit from one fix.

[cloud-fan]

"DBR" is internal Databricks-Runtime terminology and shouldn't appear in OSS comments.

Suggested change

	// metric_view.* descriptive properties (mirrors DBR SingleSourceMetricView).
	// metric_view.* descriptive properties (mirrors the canonical metric-view property layout).

[cloud-fan]

Sentence is broken: "Pins DescribeV2ViewExec emits a "Type" row matching v1 CatalogTable.toJsonLinkedHashMap parity ..." reads as if Pins is a verb taking DescribeV2ViewExec as its direct object, and "matching X parity" is awkward.

Suggested change

	// emits a "Type" row matching v1 `CatalogTable.toJsonLinkedHashMap` parity, so users
	// can distinguish a plain VIEW from a sub-kind like METRIC_VIEW.
	// "View Current Catalog" / "View Schema Mode" / etc. rows. Pins that `DescribeV2ViewExec`
	// emits a "Type" row for parity with v1 `CatalogTable.toJsonLinkedHashMap`, so users

[cloud-fan]

Round 5 review.

Status: All 6 prior round-4 findings addressed (R4 inline: Some(DependencyList.of()) not None; FunctionDependency unused-API Javadoc note; TableDependency missing-comma typo; "DBR" terminology leak; broken sentence at line 755 -- plus R4 follow-up #3: ALTER VIEW ... RENAME TO test). 1 remaining (R4 inline #4: analyzeMetricViewText still skips CheckViewReferences.checkCyclicViewReference -- accepted as a follow-up rather than a blocker in round 4). 1 new (newly introduced in e980f24427): a small citation-precision nit on the new DataSourceV2Strategy comment (inline below).

The round-5 commit is small and surgical; nothing else from prior rounds regressed. Approving -- the inline below is a nit, not blocking.

[cloud-fan]

a late catch. This API is implemented by users and called by Spark. Var-args API is not recommended here as it requires Spark to do parts: _* which is extra work.

[cloud-fan]

String[] should be better.

[cloud-fan]

Round 6 review.

Status: 2 addressed (R5 nit on DataSourceV2Strategy comment citation, fixed in fd3d8b2; R5 late catch on Dependency var-args API, fixed in f2251d8). 1 remaining (analyzeMetricViewText cyclic-view-reference follow-up — still scoped out per round 4). 4 new (3 newly introduced — DBR leak in new isViewLike Javadoc, misleading "Body kept in sync" line, null-element rejection regression on the new array records; 1 late catch — single-VIEW audit gap at views.scala).

(GitHub's inline-comment endpoint is returning a transient 422 "internal error" for this PR, so the per-line findings are inlined below as a temporary workaround. I'll re-post them properly once the API recovers.)

---

1. DBR terminology leak — interface.scala:466 (newly introduced in 90deee1). "DBR" is internal Databricks-Runtime terminology and shouldn't appear in OSS comments — round 4's inline #2 explicitly flagged this for the test comment, round 5 confirmed it was fixed, but porting the helper from the internal fork brought the term back. Drop the parenthetical or rephrase generically:

-   * METRIC_VIEW. Forks may extend this set (e.g. DBR also includes MATERIALIZED_VIEW and
-   * STREAMING_TABLE), so call sites that need a uniform "is this view-like?" check should
+   * METRIC_VIEW. Forks may extend this set with additional view-like types, so call sites
+   * that need a uniform "is this view-like?" check should

2. Misleading "Body kept in sync" line — interface.scala:773-774 (newly introduced in 90deee1). There's only one body to keep in sync because def isViewLike: Boolean = CatalogTable.isViewLike(tableType) (line 470) delegates to this companion form. The note reads as if there are two parallel bodies a future maintainer needs to update together. Drop the line:

-   * patterns or [[org.apache.spark.sql.catalyst.catalog.SessionCatalog.isView]]). Body kept
-   * in sync with the instance method.
+   * patterns or [[org.apache.spark.sql.catalyst.catalog.SessionCatalog.isView]]).

3. Null-element rejection regression — TableDependency.java:55, FunctionDependency.java:47, DependencyList.java:50 (newly introduced in f2251d8). Subtle regression from the List<String> → String[] switch: the previous List.copyOf(nameParts) rejected null elements with NPE at construction time (immutable-list invariant). nameParts.clone() does not, so Dependency.table(new String[]{null, "tbl"}) now silently constructs a TableDependency whose nameParts()[0] is null, surfacing as an NPE later when a consumer iterates parts. Same pattern in all three records.

Spark's own producer in MetricViewHelper.collectTableDependencies never emits nulls (parts come from analyzed-plan identifiers), but Dependency / DependencyList is @Evolving SPI exposed to third-party catalogs. If you want to preserve the strictness, add a per-element non-null check after the clone in each compact constructor; if relaxing it is fine, document the precondition on the nameParts / dependencies Javadoc. Not blocking either way.

4. views.scala:175 audit miss — single-VIEW audit gap (late catch). The disjunction-style tableType == VIEW || tableType == METRIC_VIEW was migrated to isViewLike in 14 sites, but CreateViewCommand.runCommand at views.scala:175 still uses the single-VIEW form tableMetadata.tableType != CatalogTableType.VIEW. Pre-PR, a metric view had tableType == VIEW so this branch did not fire on cross-kind collision; post-PR, a metric view has tableType == METRIC_VIEW, so CREATE VIEW <existing_metric_view> (no IF NOT EXISTS) now throws unsupportedCreateOrReplaceViewOnTableError ("… on a table") instead of pre-PR viewAlreadyExistsError. The error message no longer matches reality (the existing object is a view, just a metric one). Either migrate to !isViewLike to recover pre-PR behavior, or keep the strict separation but adjust the error class. Related lower-stakes consistency gap: createDataSourceTables.scala:53,154 (assert(table.tableType != CatalogTableType.VIEW)) silently accepts METRIC_VIEW input where the audit's intent reads as "no view-like types here."

[cloud-fan]

Round 7 review.

Status: 4 addressed (all R6 findings: DBR terminology drop in interface.scala:466, "Body kept in sync" line in interface.scala:773-774, null-element precondition documented on the three array-record @param Javadocs, views.scala:175 audit miss + bonus widening of the two assertions in createDataSourceTables.scala). 0 remaining. 2 new -- both late catches of mine: same isViewLike audit-miss pattern at two more sites in HiveExternalCatalog.scala (605 and 854) that I should have caught alongside views.scala:175 in round 6.

(GitHub's inline-comment endpoint was returning a transient 422 last round; trying inline again this round.)

[cloud-fan]

thanks, merging to master/4.x/4.2 (to finalize the ds v2 view api before release)