[Kernel] Block unsupported catalogManaged enablement / ICT disablement on catalogManaged table scenarios #5100

scottsand-db · 2025-08-18T21:20:06Z

🥞 Stacked PR

Use this link to review incremental changes.

stack/kernel_block_replace_while_enabling_catalogManaged [Files changed]

Which Delta project/connector is this regarding?

Description

Block setting catalogManaged=supported during REPLACE operation. Also block a few other invalid operations, like disabling ICT on a catalogManaged table, whether it is during an UPDATE or a REPLACE.

How was this patch tested?

New UT.

Does this PR introduce any user-facing changes?

No.

kernel/kernel-api/src/main/java/io/delta/kernel/internal/TransactionMetadataFactory.java

xzhseh

LGTM, left some nits + clarification quesitons.

kernel/kernel-api/src/main/java/io/delta/kernel/internal/tablefeatures/TableFeatures.java

.../kernel-api/src/main/java/io/delta/kernel/internal/ReplaceTableTransactionBuilderV2Impl.java

xzhseh · 2025-08-19T16:26:39Z

.../kernel-api/src/main/java/io/delta/kernel/internal/ReplaceTableTransactionBuilderV2Impl.java

+        {
+          add(TableConfig.COLUMN_MAPPING_MAX_COLUMN_ID.getKey());
+          add(TableConfig.IN_COMMIT_TIMESTAMPS_ENABLED.getKey());
+          // TODO are there any other table properties we should preserve?


I'm wondering do we have any available docs that track the interactions between different table features <-> DDL commands? If not, should we start consolidating one to align on the intended semantics?

Hey @xzhseh -- can you give some examples of such interactions? Here is a concrete example of two table features (ccv2, ICT) and REPLACE. Any others? Agreed we should track this.

Hey @xzhseh -- can you give some examples of such interactions? Here is a concrete example of two table features (ccv2, ICT) and REPLACE. Any others? Agreed we should track this.

E.g., CCv2's specific semantics with CLONE / REPLACE / CREATE, etc.

For CLONE we won't preserve CCv2 unless explicitly specified.

For REPLACE we retain the existing CCv2 / non-CCv2 properties.

Also there are other cases when handling dependent features such as ICT...

And for ICT and other table features they have their own specific treatments w.r.t. these DDL commands that should be explicitly documented / tracked.

I see. Thanks for elaborating! I think each component of the code should track this, at the very least. For example in this PR, for ICT + REPLACE, we document this here: https://github.com/delta-io/delta/pull/5156/files#diff-d3436360ac15655035f35b01d8e7c2bc27db976db99b04b1709ed90d64be372aR54-R61

We should at the very least do that. Also, if you happen to have a list of these such semantics in your head, perhaps we should work to document them in some README.md somewhere?

cc @nicklan and @raveeram-db for some context

Also, if you happen to have a list of these such semantics in your head, perhaps we should work to document them in some README.md somewhere?

IMO there would be two ways to do it:

Maintain a delta kernel table features <> DDL commands track list somewhere as the source of truth for all table feature implementations in kernel.

Or, in a more general (and breaking) way, update the delta protocol to include the specific DDL-related semantics of each table feature in its corresponding section.

kernel/kernel-api/src/main/java/io/delta/kernel/internal/TransactionMetadataFactory.java

...s/src/test/scala/io/delta/kernel/defaults/catalogManaged/CatalogManagedEnablementSuite.scala

.../kernel-api/src/main/java/io/delta/kernel/internal/ReplaceTableTransactionBuilderV2Impl.java

...s/src/test/scala/io/delta/kernel/defaults/catalogManaged/CatalogManagedEnablementSuite.scala

scottsand-db · 2025-09-04T21:07:57Z

Pausing this. Want to merge #5156 first.

scottsand-db · 2025-09-05T18:51:49Z

...s/src/test/scala/io/delta/kernel/defaults/catalogManaged/CatalogManagedEnablementSuite.scala

      operationType = "UPDATE",
      initialTableProperties = Map("delta.enableInCommitTimestamps" -> "false"),
      transactionProperties = Map("delta.feature.catalogOwned-preview" -> "supported"),
      expectedSuccess = true,
      expectedIctEnabled = true,
      expectedCatalogManagedSupported = true),
-    CatalogManagedEnablementTestCase(


Just moved this up. Also, I clean up some of the test case names to be more succinct and clearer

scottsand-db · 2025-09-05T18:51:55Z

...s/src/test/scala/io/delta/kernel/defaults/catalogManaged/CatalogManagedEnablementSuite.scala

+      expectedCatalogManagedSupported = false),
+
+    // ===== REPLACE cases =====
+    CatalogManagedTestCase(


New test cases

allisonport-db

LGTM

xzhseh · 2025-09-08T22:29:52Z

kernel/kernel-api/src/main/java/io/delta/kernel/internal/TransactionMetadataFactory.java

    if (!txnExplicitlyDisablesICT) {
      return;
    }

+    // Case 2: Txn is explicitly disabling ICT on a catalogManaged table. Throw.


For kernel - is it true that we don't currently have a generic interface that automatically prevents dependent features being turned off? The current implementation LGTM but we may want to generalize it a bit more..

We don't have that yet, yes.

We are able to say which table features require other table features --

delta/kernel/kernel-api/src/main/java/io/delta/kernel/internal/tablefeatures/TableFeatures.java

Line 92 in a0826e3

return Collections.singleton(IN_COMMIT_TIMESTAMP_W_FEATURE);

But this has to do with the protocol, which is different from the enablement flag.

It would be nice to be able to additionally specify that not only does this protocol feature X require another protocol Y to be supported, but Y must also be enabled, and then you cannot disable Y, too.

But this has to do with the protocol, which is different from the enablement flag.

Yeah unless we also introduce the metadata enablement flag <> table feature mappings.. E.g., we know table feature X is automatically supported/disabled when the X-enablement-flag is turned on/off.

xzhseh · 2025-09-08T22:46:07Z

...s/src/test/scala/io/delta/kernel/defaults/catalogManaged/CatalogManagedEnablementSuite.scala

@@ -19,25 +19,21 @@ package io.delta.kernel.defaults.catalogManaged
 import scala.collection.JavaConverters._


Not a blocker for this PR: Since we are also including DDL commands <> CCv2 semantics, I feel like we need a separate CatalogManagedPropertySuite to test all the interactions. Also we need to check for ucTableId as we currently store it in delta log.

+1 on having a CatalogManagedPropertySuite! Will add that to the tracker.

Also we need to check for ucTableId as we currently store it in delta log.

Great question / callout! That is UC specific -- and is completely outside the scope of Delta Kernel! That would be done in the delta-unity JAR (in this repo) -- but do remember that Kernel never cares what type of catalogManaged table/catalog (UC, glue, etc.) it is reading/talking to.

For example, we should add some checks + tests in delta-unity that prevent us from removing the uc table ID property perhaps?

That is UC specific -- and is completely outside the scope of Delta Kernel!

Ah yes, let's add those UTs in delta-unity then.

For example, we should add some checks + tests in delta-unity that prevent us from removing the uc table ID property perhaps?

Exactly. We should throw exception(s) whenever someone tries to (explicitly) modify ucTableId via any DDL commands.

...s/src/test/scala/io/delta/kernel/defaults/catalogManaged/CatalogManagedEnablementSuite.scala

scottsand-db changed the title ~~done first pass~~ [Kernel] Block setting catalogManaged=supported during REPLACE operation Aug 18, 2025

scottsand-db requested review from allisonport-db and emkornfield August 18, 2025 21:20

scottsand-db self-assigned this Aug 18, 2025

allisonport-db reviewed Aug 18, 2025

View reviewed changes

kernel/kernel-api/src/main/java/io/delta/kernel/internal/TransactionMetadataFactory.java Show resolved Hide resolved

scottsand-db mentioned this pull request Aug 18, 2025

[Kernel] Implement catalogManaged table feature in Kernel #4573

Open

scottsand-db requested review from allisonport-db and xzhseh August 18, 2025 22:42

xzhseh approved these changes Aug 19, 2025

View reviewed changes

scottsand-db force-pushed the stack/kernel_block_replace_while_enabling_catalogManaged branch from 7e57e6e to dd5e30d Compare September 2, 2025 15:54

scottsand-db requested a review from dhruvarya-db September 2, 2025 15:55

scottsand-db force-pushed the stack/kernel_block_replace_while_enabling_catalogManaged branch from dd5e30d to cc725a8 Compare September 2, 2025 16:14

allisonport-db reviewed Sep 2, 2025

View reviewed changes

.../kernel-api/src/main/java/io/delta/kernel/internal/ReplaceTableTransactionBuilderV2Impl.java Outdated Show resolved Hide resolved

...s/src/test/scala/io/delta/kernel/defaults/catalogManaged/CatalogManagedEnablementSuite.scala Show resolved Hide resolved

scottsand-db requested review from allisonport-db and xzhseh September 4, 2025 16:19

xzhseh approved these changes Sep 4, 2025

View reviewed changes

...s/src/test/scala/io/delta/kernel/defaults/catalogManaged/CatalogManagedEnablementSuite.scala Show resolved Hide resolved

scottsand-db changed the title ~~[Kernel] Block setting catalogManaged=supported during REPLACE operation~~ [Merge after #5156] [Kernel] Block setting catalogManaged=supported during REPLACE operation Sep 4, 2025

scottsand-db marked this pull request as draft September 4, 2025 21:59

scottsand-db force-pushed the stack/kernel_block_replace_while_enabling_catalogManaged branch from cc725a8 to 4b900a0 Compare September 5, 2025 18:49

scottsand-db changed the title ~~[Merge after #5156] [Kernel] Block setting catalogManaged=supported during REPLACE operation~~ [Kernel] Block setting catalogManaged=supported during REPLACE operation Sep 5, 2025

scottsand-db marked this pull request as ready for review September 5, 2025 18:50

scottsand-db commented Sep 5, 2025

View reviewed changes

scottsand-db added 4 commits September 5, 2025 18:54

done first pass

3e5cb75

second pass

d39f57b

respond to pr comments

5f47761

add more test cases; fix ICT disablement logic

f161792

scottsand-db requested a review from raveeram-db September 5, 2025 18:54

scottsand-db changed the title ~~[Kernel] Block setting catalogManaged=supported during REPLACE operation~~ [Kernel] Block unsupported catalogManaged enablement / ICT disablement on catalogManaged table scenarios Sep 5, 2025

scottsand-db force-pushed the stack/kernel_block_replace_while_enabling_catalogManaged branch from 4b900a0 to f161792 Compare September 5, 2025 18:56

scottsand-db requested a review from xzhseh September 5, 2025 18:56

allisonport-db approved these changes Sep 8, 2025

View reviewed changes

xzhseh approved these changes Sep 8, 2025

View reviewed changes

scottsand-db merged commit 337456f into delta-io:master Sep 9, 2025
27 checks passed

		@@ -19,25 +19,21 @@ package io.delta.kernel.defaults.catalogManaged
		import scala.collection.JavaConverters._

[Kernel] Block unsupported catalogManaged enablement / ICT disablement on catalogManaged table scenarios #5100

[Kernel] Block unsupported catalogManaged enablement / ICT disablement on catalogManaged table scenarios #5100

Uh oh!

Conversation

scottsand-db commented Aug 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🥞 Stacked PR

Which Delta project/connector is this regarding?

Description

How was this patch tested?

Does this PR introduce any user-facing changes?

Uh oh!

Uh oh!

xzhseh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

scottsand-db commented Sep 4, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

allisonport-db left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scottsand-db Sep 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

scottsand-db commented Aug 18, 2025 •

edited

Loading

scottsand-db Sep 8, 2025 •

edited

Loading