Skip to content

[SPARK-58604][SQL] Preserve viewDependencies when rebuilding a View for ALTER VIEW - #57802

Closed
szehon-ho wants to merge 3 commits into
apache:masterfrom
szehon-ho:spark-view-deps-alter
Closed

[SPARK-58604][SQL] Preserve viewDependencies when rebuilding a View for ALTER VIEW#57802
szehon-ho wants to merge 3 commits into
apache:masterfrom
szehon-ho:spark-view-deps-alter

Conversation

@szehon-ho

Copy link
Copy Markdown
Member

What changes were proposed in this pull request?

CatalogV2Util.viewInfoBuilderFrom seeds a View.Builder from an existing view so ALTER VIEW execs can override the one field that changes and leave everything else untouched. It copies the schema, properties, query text, SQL configs, current namespace, current catalog, query column names and schema mode — but not the typed viewDependencies field.

This PR carries viewDependencies through, consistent with how the other nullable fields (currentCatalog, schemaMode) are already handled.

Why are the changes needed?

All three callers of viewInfoBuilderFrom are metadata-only mutations that do not change the view body:

  • AlterV2ViewSetPropertiesExec (ALTER VIEW ... SET TBLPROPERTIES)
  • AlterV2ViewUnsetPropertiesExec (ALTER VIEW ... UNSET TBLPROPERTIES)
  • AlterV2ViewSchemaBindingExec (ALTER VIEW ... WITH SCHEMA ...)

Each rebuilds the payload and calls ViewCatalog.replaceView, so after any of them the catalog receives a View whose viewDependencies() is null and the previously recorded dependency list is silently lost. Dependency lists are a first-class field on View rather than an encoded string property precisely because their nested structure does not round-trip through flat properties, so a catalog has no other way to recover them.

This is reachable today through metric views, which are the only producer of dependencies (CreateV2MetricViewExec). Creating a metric view records its source tables, and a subsequent property change drops them:

CREATE VIEW mv WITH METRICS LANGUAGE YAML AS $$ ... $$;  -- dependencies recorded
ALTER VIEW mv SET TBLPROPERTIES ('k' = 'v');             -- dependencies now null

Does this PR introduce any user-facing change?

No, in the sense that no released version is affected — the View API and these ALTER VIEW execs are new in the unreleased line, so this is a fix within master rather than a change in behavior users have depended on. Catalogs that persist view lineage will now keep it across a metadata-only ALTER VIEW instead of seeing it cleared.

How was this patch tested?

Two new tests, both confirmed to fail before the fix and pass after:

  • CatalogV2UtilSuite: viewInfoBuilderFrom preserves a dependency list, and leaves an absent one absent (guards the null path). The first fails without the fix.
  • MetricViewV2CatalogSuite: end-to-end ALTER VIEW <metric_view> SET TBLPROPERTIES against the recording RelationCatalog, asserting the replacement payload still carries the source-table dependency. Without the fix this fails with viewDependencies() being null.

Full suites pass locally: CatalogV2UtilSuite (10 tests) and MetricViewV2CatalogSuite (32 tests). ./dev/lint-scala passes.

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

Generated-by: Cursor (Opus 5)

…or ALTER VIEW

CatalogV2Util.viewInfoBuilderFrom copies every field of an existing View
except the typed viewDependencies, so the three metadata-only ALTER VIEW
execs that rebuild through it hand ViewCatalog.replaceView a payload with
no dependency list. Because dependencies are a first-class field rather
than an encoded property, the catalog cannot recover them.

Generated-by: Cursor (Opus 5)
@szehon-ho

szehon-ho commented Aug 6, 2026

Copy link
Copy Markdown
Member Author

@cloud-fan could you take a look? If the v2 view path isn't your area, I'd appreciate a pointer to a better reviewer.

Some context on why this matters outside Spark. I hit this while reviewing Apache Iceberg's Spark 4.2 support (apache/iceberg#14984). Iceberg implements RelationCatalog and is working out how much of its own view DDL handling it can hand back to Spark's native v2 view execs, and this bug is one of the reasons it can't delegate the ALTER path yet: a catalog that persists view lineage silently loses it on any metadata-only ALTER VIEW. Because viewDependencies is a typed field on View rather than an encoded string property, there is no way for the catalog to reconstruct it after the fact.

@cloud-fan cloud-fan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0 blocking, 0 non-blocking, 0 nits.
The metadata-copy fix is consistent with the existing View contract and is covered at both the helper and ALTER VIEW integration boundaries.

Verification

I traced all three callers of viewInfoBuilderFrom to ViewCatalog.replaceView, checked the nullable View.viewDependencies() contract and builder behavior, and verified that the tests cover both null preservation and the metric-view CREATE-to-ALTER replacement path. Tests were not run as part of this review.

@cloud-fan cloud-fan closed this in 4a471e4 Aug 6, 2026
cloud-fan pushed a commit that referenced this pull request Aug 6, 2026
…or ALTER VIEW

### What changes were proposed in this pull request?

`CatalogV2Util.viewInfoBuilderFrom` seeds a `View.Builder` from an existing view so ALTER VIEW execs can override the one field that changes and leave everything else untouched. It copies the schema, properties, query text, SQL configs, current namespace, current catalog, query column names and schema mode — but not the typed `viewDependencies` field.

This PR carries `viewDependencies` through, consistent with how the other nullable fields (`currentCatalog`, `schemaMode`) are already handled.

### Why are the changes needed?

All three callers of `viewInfoBuilderFrom` are metadata-only mutations that do not change the view body:

- `AlterV2ViewSetPropertiesExec` (`ALTER VIEW ... SET TBLPROPERTIES`)
- `AlterV2ViewUnsetPropertiesExec` (`ALTER VIEW ... UNSET TBLPROPERTIES`)
- `AlterV2ViewSchemaBindingExec` (`ALTER VIEW ... WITH SCHEMA ...`)

Each rebuilds the payload and calls `ViewCatalog.replaceView`, so after any of them the catalog receives a `View` whose `viewDependencies()` is `null` and the previously recorded dependency list is silently lost. Dependency lists are a first-class field on `View` rather than an encoded string property precisely because their nested structure does not round-trip through flat properties, so a catalog has no other way to recover them.

This is reachable today through metric views, which are the only producer of dependencies (`CreateV2MetricViewExec`). Creating a metric view records its source tables, and a subsequent property change drops them:

```sql
CREATE VIEW mv WITH METRICS LANGUAGE YAML AS $$ ... $$;  -- dependencies recorded
ALTER VIEW mv SET TBLPROPERTIES ('k' = 'v');             -- dependencies now null
```

### Does this PR introduce _any_ user-facing change?

No, in the sense that no released version is affected — the `View` API and these ALTER VIEW execs are new in the unreleased line, so this is a fix within master rather than a change in behavior users have depended on. Catalogs that persist view lineage will now keep it across a metadata-only ALTER VIEW instead of seeing it cleared.

### How was this patch tested?

Two new tests, both confirmed to fail before the fix and pass after:

- `CatalogV2UtilSuite`: `viewInfoBuilderFrom` preserves a dependency list, and leaves an absent one absent (guards the null path). The first fails without the fix.
- `MetricViewV2CatalogSuite`: end-to-end `ALTER VIEW <metric_view> SET TBLPROPERTIES` against the recording `RelationCatalog`, asserting the replacement payload still carries the source-table dependency. Without the fix this fails with `viewDependencies()` being `null`.

Full suites pass locally: `CatalogV2UtilSuite` (10 tests) and `MetricViewV2CatalogSuite` (32 tests). `./dev/lint-scala` passes.

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

Generated-by: Cursor (Opus 5)

Closes #57802 from szehon-ho/spark-view-deps-alter.

Authored-by: Szehon Ho <szehon.apache@gmail.com>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
(cherry picked from commit 4a471e4)
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
cloud-fan pushed a commit that referenced this pull request Aug 6, 2026
…or ALTER VIEW

### What changes were proposed in this pull request?

`CatalogV2Util.viewInfoBuilderFrom` seeds a `View.Builder` from an existing view so ALTER VIEW execs can override the one field that changes and leave everything else untouched. It copies the schema, properties, query text, SQL configs, current namespace, current catalog, query column names and schema mode — but not the typed `viewDependencies` field.

This PR carries `viewDependencies` through, consistent with how the other nullable fields (`currentCatalog`, `schemaMode`) are already handled.

### Why are the changes needed?

All three callers of `viewInfoBuilderFrom` are metadata-only mutations that do not change the view body:

- `AlterV2ViewSetPropertiesExec` (`ALTER VIEW ... SET TBLPROPERTIES`)
- `AlterV2ViewUnsetPropertiesExec` (`ALTER VIEW ... UNSET TBLPROPERTIES`)
- `AlterV2ViewSchemaBindingExec` (`ALTER VIEW ... WITH SCHEMA ...`)

Each rebuilds the payload and calls `ViewCatalog.replaceView`, so after any of them the catalog receives a `View` whose `viewDependencies()` is `null` and the previously recorded dependency list is silently lost. Dependency lists are a first-class field on `View` rather than an encoded string property precisely because their nested structure does not round-trip through flat properties, so a catalog has no other way to recover them.

This is reachable today through metric views, which are the only producer of dependencies (`CreateV2MetricViewExec`). Creating a metric view records its source tables, and a subsequent property change drops them:

```sql
CREATE VIEW mv WITH METRICS LANGUAGE YAML AS $$ ... $$;  -- dependencies recorded
ALTER VIEW mv SET TBLPROPERTIES ('k' = 'v');             -- dependencies now null
```

### Does this PR introduce _any_ user-facing change?

No, in the sense that no released version is affected — the `View` API and these ALTER VIEW execs are new in the unreleased line, so this is a fix within master rather than a change in behavior users have depended on. Catalogs that persist view lineage will now keep it across a metadata-only ALTER VIEW instead of seeing it cleared.

### How was this patch tested?

Two new tests, both confirmed to fail before the fix and pass after:

- `CatalogV2UtilSuite`: `viewInfoBuilderFrom` preserves a dependency list, and leaves an absent one absent (guards the null path). The first fails without the fix.
- `MetricViewV2CatalogSuite`: end-to-end `ALTER VIEW <metric_view> SET TBLPROPERTIES` against the recording `RelationCatalog`, asserting the replacement payload still carries the source-table dependency. Without the fix this fails with `viewDependencies()` being `null`.

Full suites pass locally: `CatalogV2UtilSuite` (10 tests) and `MetricViewV2CatalogSuite` (32 tests). `./dev/lint-scala` passes.

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

Generated-by: Cursor (Opus 5)

Closes #57802 from szehon-ho/spark-view-deps-alter.

Authored-by: Szehon Ho <szehon.apache@gmail.com>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
(cherry picked from commit 4a471e4)
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
@cloud-fan

Copy link
Copy Markdown
Contributor

Merge Summary:

Posted by merge_spark_pr.py

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants