Skip to content

Merge existing policies on CREATE OR REPLACE (RTAS) - #652

Merged
mkuchenbecker merged 8 commits into
linkedin:mainfrom
mkuchenbecker:mkuchenbecker/r2-rtas-policies-merge
Aug 3, 2026
Merged

Merge existing policies on CREATE OR REPLACE (RTAS)#652
mkuchenbecker merged 8 commits into
linkedin:mainfrom
mkuchenbecker:mkuchenbecker/r2-rtas-policies-merge

Conversation

@mkuchenbecker

@mkuchenbecker mkuchenbecker commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Summary

CREATE OR REPLACE ... AS SELECT (RTAS) silently dropped the table's policies. The replace path rebuilt the policies table property purely from the incoming request, so a replace that omitted policies wiped the existing retention, sharing, PII column tags, replication, and history, even though ordinary user table properties survived.

Fix

Policies are table metadata that a replace must not silently drop. Before the replace properties are built, the existing table's policies are merged with the request's policies. The merge is based on the existing policies, so any plane that the request does not explicitly provide is carried forward from the existing table, and each plane that the request does provide overrides the existing value.

Merge behavior

The table below describes how each policy plane behaves during a replace.

Plane When the request provides it When the request omits it
retention The request value is applied. The existing value is carried forward.
replication The request value is applied. The existing value is carried forward.
history The request value is applied. The existing value is carried forward.
lockState The request value is applied. The existing value is carried forward.
columnTags The request map replaces the existing map in full. This is an overwrite, not a per-key merge. The existing map is carried forward. An omitted field and an empty map are treated the same way.
sharingEnabled Not applicable, because this is a primitive boolean and a provided value cannot be distinguished from an omitted one. The existing value is carried forward.

Two consequences follow from this behavior. First, because column tags use overwrite semantics and an empty map is treated the same as an omitted field, a replace cannot clear all column tags. Clearing tags is done with ALTER TABLE ... MODIFY COLUMN ... UNSET TAG. Second, because sharingEnabled is a primitive boolean with no unset state, its value is always preserved across a replace. Sharing is changed with ALTER TABLE ... SET POLICY (SHARING=...).

Spark RTAS has no policy clause, so it always sends a request with no policies, and the entire existing policies object is carried forward unchanged. A partial policy payload can only arrive from a client that calls the REST API directly.

This behavior is consistent with the intent of RTAS, which should preserve table properties so that a replace does not require re-granting access to the same entity.

Testing Done

The REST level partial payload path is exercised through RepositoryTest, which is the layer that can send a partial Policies object. Spark cannot reach this path because it always sends a request with no policies.

  • testReplaceMergesExistingPolicies replaces a table without policies and asserts that the retention policy survives.
  • testReplaceAppliesRequestedPolicies asserts that a retention policy provided on the request is applied.
  • testReplaceWithPartialPoliciesPreservesSharing sends a partial payload containing only retention and asserts that sharingEnabled stays true while the new retention is applied.
  • testReplaceWithPartialPoliciesPreservesOmittedPlanes overrides only retention and asserts that the omitted history plane is carried forward.
  • testReplaceWithPartialPoliciesPreservesColumnTags sends a payload that provides retention but omits column tags, and asserts that the existing column tag is carried forward.
  • testReplaceOverwritesColumnTags sends a new column tag map and asserts that it replaces the existing map in full, dropping the previous tag.

Black box coverage is exercised through RtasPolicyPreservationTest against an embedded OpenHouse server driven by Spark SQL. It asserts that retention, sharing, the PII column tag, and history all survive a REPLACE TABLE ... AS SELECT.

The existing SnapshotsControllerTest.testPutSnapshotsReplaceCommit still passes, which confirms that a replace on a table that never had policies still yields none. ./gradlew :services:tables:test and :integrations:spark:spark-3.1:openhouse-spark-itest:catalogTest pass on JDK 17, and Spotless is clean on the module.

RTAS re-materialized the policies table property only from the request, so a
replace that omitted policies silently wiped retention, sharing, PII column
tags, replication, and history. Policies are table metadata and must survive a
replace: merge the existing table's policies with the request's, each plane the
request provides winning and every omitted plane carried forward.

Co-authored-by: Copilot <[email protected]>
mkuchenbecker and others added 2 commits July 22, 2026 16:50
Capture the merge contract: each plane merged, omitted planes carried forward,
provided planes win (partial merge), planes can be added via RTAS, columnTags
empty-vs-nonempty, and sharingEnabled takes the request value. Adds a unit test
over mergePolicies and an e2e test that RTAS applies a requested policy.

Co-authored-by: Copilot <[email protected]>
Replace the mergePolicies unit test with black-box tests that drive real Spark
SQL against a real embedded OpenHouse server (OpenHouseSparkITest): set policies
via ALTER TABLE ... SET POLICY, run CREATE OR REPLACE TABLE ... AS SELECT, and
assert via SHOW TBLPROPERTIES that retention, sharing, PII column tags, and
history survive the replace.

Also fix RTASTest.testRTAS, which asserted the old buggy behavior (policies
wiped on RTAS); it now asserts the history policy is preserved.

Co-authored-by: Copilot <[email protected]>
@mkuchenbecker
mkuchenbecker marked this pull request as ready for review July 23, 2026 21:11
cbb330
cbb330 previously requested changes Jul 23, 2026
@mkuchenbecker

Copy link
Copy Markdown
Contributor Author

PTAL @cbb330 , we can add a tripwire to make sure this is updated if we want to guard against silent loss - #659

@mkuchenbecker
mkuchenbecker dismissed cbb330’s stale review July 31, 2026 18:25

No re-review within a week; requested change is staged as a separate PR for review.

mkuchenbecker and others added 2 commits July 31, 2026 14:51
…ge semantics

The per-plane policy merge for CREATE OR REPLACE built the result from the
request, so every plane except sharingEnabled was correctly carried forward
from the existing table while sharingEnabled -- a primitive boolean with no
"unset" state -- was always reset to whatever the request happened to carry.
A non-Spark client sending a partial policies payload (e.g. retention only)
would silently disable sharing on a table that had it enabled: exactly the
policy-loss class this change set out to fix, for the one plane it missed.

Base the merge builder on the existing policies instead of the request, so any
plane the request does not explicitly override -- including sharingEnabled --
is carried forward. The five object planes keep their explicit
request-wins-if-provided behavior; sharingEnabled is now preserved from the
existing table across a replace (change it via ALTER TABLE ... SET POLICY
(SHARING=...)).

Document the merge contract on mergePolicies, including the columnTags
overwrite semantics (a non-empty request map replaces the existing map
wholesale; an absent/empty map preserves it, so clearing all tags is not
expressible through a replace).

Add REST-level (H2 RepositoryTest) tests for the partial-payload path, which
Spark RTAS cannot exercise (Spark always sends policies == null): sharing
preserved on a partial payload, an omitted plane (history) carried forward
while retention is overridden, and columnTags overwrite dropping a prior tag.

Co-authored-by: Copilot <[email protected]>
mergePolicies special-cases columnTags with MapUtils.isEmpty so that a
non-empty replace payload that omits column tags preserves the existing tags
rather than wiping them. That branch was not exercised: the existing tests
covered the non-empty overwrite path and the fully-null request short-circuit,
but not a non-null request that omits column tags while the table has them.

Add a RepositoryTest case that creates a table with a col1 PII tag, replaces
it with a payload providing only retention, and asserts the col1 tag is
carried forward while the new retention is applied.

Co-authored-by: Copilot <[email protected]>

@kamanavishnu kamanavishnu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM!

@mkuchenbecker
mkuchenbecker merged commit 8cb1728 into linkedin:main Aug 3, 2026
1 check passed
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