[Kernel] Integrate `TableFeature` framework in read and write path #4159

vkorukanti · 2025-02-14T18:30:42Z

Description

This integrates the end-to-end TableFeatures framework in read and write path.

It also adds support for appending into a table with deletionVectors, v2Checkpoint and 'timestampNtz` are supported.

How was this patch tested?

Existing and new tests.

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

anoopj · 2025-02-19T01:39:16Z

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

+    }
+  }
+
+  // Important note: uses the default implementation of `equals` and `hashCode` methods.


Makes sense, but would it make sense to move this comment to the class javadoc?

anoopj · 2025-02-19T01:41:24Z

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

+  private static class CheckpointV2TableFeature extends TableFeature.ReaderWriterFeature
+      implements FeatureAutoEnabledByMetadata {
+    CheckpointV2TableFeature() {
+      super("v2Checkpoint", /* minReaderVersion = */ 3, /* minWriterVersion = */ 7);


Nit: would it make sense to use the constants like TABLE_FEATURES_MIN_READER_VERSION etc?

hmm. I think keeping constants is fine here, given some of the legacy protocols (e.g. invariants ) have no constants.

anoopj · 2025-02-19T01:48:42Z

kernel/kernel-api/src/main/java/io/delta/kernel/internal/util/CaseInsensitiveMap.java

+ *
+ * @param <V>
+ */
+public class CaseInsensitiveMap<V> implements Map<String, V> {


Would it make sense to call it something like CaseInsensitiveStringMap since the key is always a string?

Sounds good. Will followup in a separate PR. I am also wondering if this is really needed.

anoopj · 2025-02-19T01:49:37Z

kernel/kernel-api/src/main/java/io/delta/kernel/internal/util/CaseInsensitiveMap.java

+ *
+ * @param <V>
+ */
+public class CaseInsensitiveMap<V> implements Map<String, V> {


Would it make sense to implement equals and hashCode?

Same comment as above.

scottsand-db

Looks great! Left some minor comments and asked some questions.

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

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

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

scottsand-db · 2025-02-19T17:12:45Z

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

+            .withFeatures(extractAutomaticallyEnabledFeatures(metadata, current))
+            .normalized();
+
+    if (!required.canUpgradeTo(current)) {


wouldn't we want to see if the currProtocol can be upgraded to the requiredProtocol ?

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

anoopj

LGTM

...el/kernel-api/src/test/scala/io/delta/kernel/internal/tablefeatures/TableFeaturesSuite.scala

kernel/kernel-defaults/src/test/scala/io/delta/kernel/defaults/DeltaTableWritesSuite.scala

…orted

vkorukanti · 2025-02-24T18:35:50Z

kernel/kernel-defaults/src/test/scala/io/delta/kernel/defaults/RowTrackingSuite.scala

@@ -231,30 +231,6 @@ class RowTrackingSuite extends DeltaTableWriteSuiteBase with ParquetSuiteBase {
    }
  }

-  test("Fail if row tracking is supported but domain metadata is not supported") {


cc @qiyuandong-db @johanl-db Removing this based on the offline discussion. This scenario is not possible with the new table feature framework.

vkorukanti · 2025-02-24T18:37:29Z

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

@@ -208,7 +209,11 @@ private static class RowTrackingFeature extends TableFeature.WriterFeature

    @Override
    public boolean metadataRequiresFeatureToBeEnabled(Protocol protocol, Metadata metadata) {
-      return TableConfig.ROW_TRACKING_ENABLED.fromMetadata(metadata);
+      if (TableConfig.ROW_TRACKING_ENABLED.fromMetadata(metadata)) {


cc @qiyuandong-db @johanl-db Disabling this based on the offline discussion, given Kernel doesn't support enabling row tracking through metadata props.

to clarify, this means we don't support enabling on an existing table right? but we do support enabling on a new table? Can we maybe restate this so it's more user friendly? I don't think they'll understand enabling a feature through metadata

Even on the new table is not supported. Currently the way it is set is through passing a protocol action in the Txn.commit along with the file actions.

Okay so it would be correct to say - for users - enabling row tracking is not supported?

@qiyuandong-db @johanl-db please chime in on if there is any other way to enable row tracking currently.

updated the error message

Sorry now I'm confused. We support writing to tables with rowTracking already enabled right? We just don't support enabling rowTracking. But maybe I'm misunderstanding the current status?

I will leave it up to @qiyuandong-db and @johanl-db to update the status. It doesn't make sense to go back and forth on the error message and block this PR.

okay... @johanl-db @qiyuandong-db can you please create a ticket to follow up on this?

Yes, we already have a ticket to track this and we'll want to follow up on it this quarter.

allisonport-db

Looking at the test code now

allisonport-db · 2025-02-21T22:53:19Z

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

-        break;
-      default:
-        throw unsupportedWriterProtocol(tablePath, minWriterVersion);
+  public static Optional<Tuple2<Protocol, Set<TableFeature>>> autoUpgradeProtocolBasedOnMetadata(


Is this what we currently do in Delta Spark? Automatically upgrade to (3, 7) when enabling something like column mapping?

I'm wondering, is it overkill, to do this every write operation? Even if there is no metadata change? (just wondering, I know it's what we are already doing before)

Is this what we currently do in Delta Spark? Automatically upgrade to (3, 7) when enabling something like column mapping?

Goal is to set the minimal protocol version that supports all the supported features or identity columns. If we just set column mapping and not feature from (3,7), then the protocol version is going to be (2, 5) as that's the lowest version that supports column mapping.

I'm wondering, is it overkill, to do this every write operation? Even if there is no metadata change? (just wondering, I know it's what we are already doing before)

There are like <20 table features and running through them takes few ms. Also it is not easy to find what metadata changes are we interested in (this info is encapsulated in the TableFEatures definitions). If we want to avoid calling this, then we need to repeat the same code (find if interested metadata has changes) here.

I assume we have existing tests for all these upgrades somewhere? Do you know where that is?

Goal is to set the minimal protocol version that supports all the supported features or identity columns. If we just set column mapping and not feature from (3,7), then the protocol version is going to be (2, 5) as that's the lowest version that supports column mapping.

Maybe I'm confused but I was thinking that's not what it looks like we do? We have

Protocol required = new Protocol(TABLE_FEATURES_MIN_READER_VERSION, TABLE_FEATURES_MIN_WRITER_VERSION) .withFeatures(extractAutomaticallyEnabledNewFeatures(newMetadata, currentProtocol)) .normalized();

which we use to merge and I see the docs for that are

/** * Merge this protocol with multiple `protocols` to have the highest reader and writer versions * plus all explicitly and implicitly supported features. */

We start with protocol with feature support, add all the features and then normalize it to be the lowest needed protocol version.

It seems like that will keep the table feature protocol though for the scenario like just enabling columnMapping? Since we don't have the other features in the legacy versions.

I'm totally fine with this being the behavior I'm just wondering if this what we do in delta-spark? And want to make sure we test our upgrades

added unittests to clarify the expected output through test examples.

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

allisonport-db · 2025-02-24T22:01:29Z

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

@@ -208,7 +209,11 @@ private static class RowTrackingFeature extends TableFeature.WriterFeature

    @Override
    public boolean metadataRequiresFeatureToBeEnabled(Protocol protocol, Metadata metadata) {
-      return TableConfig.ROW_TRACKING_ENABLED.fromMetadata(metadata);
+      if (TableConfig.ROW_TRACKING_ENABLED.fromMetadata(metadata)) {


to clarify, this means we don't support enabling on an existing table right? but we do support enabling on a new table? Can we maybe restate this so it's more user friendly? I don't think they'll understand enabling a feature through metadata

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

...el/kernel-api/src/test/scala/io/delta/kernel/internal/tablefeatures/TableFeaturesSuite.scala

allisonport-db · 2025-02-25T01:04:28Z

kernel/kernel-defaults/src/test/scala/io/delta/kernel/defaults/DeltaTableFeaturesSuite.scala

+ */
+class DeltaTableFeaturesSuite extends DeltaTableWriteSuiteBase {
+
+  ///////////////////////////////////////////////////////////////////////////


Do we have tests for things like if I try to enable CDF we will not be able to write?

Added the unittests in TableFeaturesSuite.scala for each feature (supported, not enabled) and (supported, enabled) cases.

I haven't added the integration tests. I feel the unittests are good enough to cover.

yeah those are very thorough! Can we add 1 test for 1 feature that's enabled by metadata but not supported by Kernel just to check the E2E integration & path through TransactionBuilderImpl? Like just this CDF enable case will fail

kernel/kernel-defaults/src/test/scala/io/delta/kernel/defaults/DeltaTableWritesSuite.scala

kernel/kernel-defaults/src/test/scala/io/delta/kernel/defaults/InCommitTimestampSuite.scala

allisonport-db

I don't see tests for metadata-based protocol upgrades unless they're just feature-by-feature?

…delta commit files (delta-io#4182) ## Description See for delta-io#3888 for details. Some versions of the Delta-Spark written tables with commit info containing arbitrary value types in the map of s`tring -> string`. This has existed for a while. Update the Kernel default delta commit file reader to always try to parse as a `string` if the value type is a `string` type. This is not ideal, but no other easy ways to handle this. ## How was this patch tested? UT.

allisonport-db

Just a few minor comments.

Also still wondering if we already have tests for the metadata-based automatic upgrades? and if not we should test our upgrade logic

allisonport-db · 2025-02-25T20:39:42Z

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

@@ -208,7 +209,11 @@ private static class RowTrackingFeature extends TableFeature.WriterFeature

    @Override
    public boolean metadataRequiresFeatureToBeEnabled(Protocol protocol, Metadata metadata) {
-      return TableConfig.ROW_TRACKING_ENABLED.fromMetadata(metadata);
+      if (TableConfig.ROW_TRACKING_ENABLED.fromMetadata(metadata)) {


+1 let's make this error message clear and user-friendly

allisonport-db · 2025-02-25T20:45:01Z

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

-        break;
-      default:
-        throw unsupportedWriterProtocol(tablePath, minWriterVersion);
+  public static Optional<Tuple2<Protocol, Set<TableFeature>>> autoUpgradeProtocolBasedOnMetadata(


It seems like that will keep the table feature protocol though for the scenario like just enabling columnMapping? Since we don't have the other features in the legacy versions.

I'm totally fine with this being the behavior I'm just wondering if this what we do in delta-spark? And want to make sure we test our upgrades

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

...el/kernel-api/src/test/scala/io/delta/kernel/internal/tablefeatures/TableFeaturesSuite.scala

allisonport-db · 2025-02-25T21:06:11Z

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

-              add("v2Checkpoint");
-            }
-          });
+    Set<TableFeature> autoEnabledFeatures =


nit: we should decide if this is only newAutoEnabledFeatures (in which case we should only add domain_metadata below if it's not already there) or all autoEnabledFeatures and update the name as well

I think this is fine to be all autoEnabledFeatures right? maybe update extractAutomaticallyEnabledNewFeatures to not remove the existing features

makes more semantic sense - create the protocol we require for this table

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

allisonport-db · 2025-02-25T21:13:02Z

kernel/kernel-defaults/src/test/scala/io/delta/kernel/defaults/DeltaTableFeaturesSuite.scala

+ */
+class DeltaTableFeaturesSuite extends DeltaTableWriteSuiteBase {
+
+  ///////////////////////////////////////////////////////////////////////////


yeah those are very thorough! Can we add 1 test for 1 feature that's enabled by metadata but not supported by Kernel just to check the E2E integration & path through TransactionBuilderImpl? Like just this CDF enable case will fail

allisonport-db

thanks the autoUpgradeProtocolBasedOnMetadata tests help a ton and are super thorough! sorry a few more questions

allisonport-db · 2025-02-26T01:37:33Z

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

@@ -208,7 +209,11 @@ private static class RowTrackingFeature extends TableFeature.WriterFeature

    @Override
    public boolean metadataRequiresFeatureToBeEnabled(Protocol protocol, Metadata metadata) {
-      return TableConfig.ROW_TRACKING_ENABLED.fromMetadata(metadata);
+      if (TableConfig.ROW_TRACKING_ENABLED.fromMetadata(metadata)) {


Sorry now I'm confused. We support writing to tables with rowTracking already enabled right? We just don't support enabling rowTracking. But maybe I'm misunderstanding the current status?

kernel/kernel-api/src/main/java/io/delta/kernel/internal/actions/Protocol.java

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

allisonport-db

LGTM

allisonport-db · 2025-02-26T02:40:08Z

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

@@ -429,7 +429,7 @@ public static Optional<Tuple2<Protocol, Set<TableFeature>>> autoUpgradeProtocolB

    Set<TableFeature> allNeededTableFeatures =
        extractAllNeededTableFeatures(newMetadata, currentProtocol);
-    if (needDomainMetadataSupport && !allNeededTableFeatures.contains(DOMAIN_METADATA_W_FEATURE)) {
+    if (needDomainMetadataSupport) {
      allNeededTableFeatures =


not blocking but why not just allNeededTableFreatures.add(domainMetadata)?

It may not be mutable Set given we are using Streams

allisonport-db · 2025-02-26T02:43:26Z

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

@@ -208,7 +209,11 @@ private static class RowTrackingFeature extends TableFeature.WriterFeature

    @Override
    public boolean metadataRequiresFeatureToBeEnabled(Protocol protocol, Metadata metadata) {
-      return TableConfig.ROW_TRACKING_ENABLED.fromMetadata(metadata);
+      if (TableConfig.ROW_TRACKING_ENABLED.fromMetadata(metadata)) {


okay... @johanl-db @qiyuandong-db can you please create a ticket to follow up on this?

amogh-jahagirdar · 2025-02-27T10:13:22Z

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

-    if (tableProperties.isPresent()) {
-      Map<String, String> validatedProperties =
-          TableConfig.validateDeltaProperties(tableProperties.get());
-      Map<String, String> newProperties =
-          metadata.filterOutUnchangedProperties(validatedProperties);
-
-      ColumnMapping.verifyColumnMappingChange(
-          metadata.getConfiguration(), newProperties, isNewTable);


I'm a bit late to this PR but was this change to do this logic even when there wasn't a table properties change intentional? I think it'll fail the verifyColumnMappingChange check below when it shouldn't , in the case a user didn't define table properties on a subsequent transaction (the new properties will be empty, and then the column mode will be considered as null even though it's set on the current state of the table). Unless I'm missing something

Basically, TLDR if I'm not mistaken I think we still need this validation only in the condition that tableProperties.isPresent

We do want to run through the protocol changes even when the table properties are empty, because there could be schema changes that we want to consider (e.g. the schema may have timestamp_ntz in which case we need to enabled timestampNtz table feature).

However the ColumnMapping.verifyColumnMappingChange( metadata.getConfiguration(), newProperties, isNewTable) should take the entire newConfiguration instead of just taking the changed properties. This was the case even before the this PR. It is possible that the new table properties may not contain the column mapping mode.

I will make a PR to fix this. Also need to check why the tests didn't catch this.

vkorukanti changed the title ~~Tf integration~~ [Kernel] Integrate TableFeature framework in read and write path Feb 14, 2025

vkorukanti added the kernel label Feb 14, 2025

anoopj reviewed Feb 19, 2025

View reviewed changes

scottsand-db requested changes Feb 19, 2025

View reviewed changes

[Kernel] Integrate table features utilities into the read and write path

4aacdbd

vkorukanti force-pushed the tfIntegration branch from 4e60a26 to 745c4a4 Compare February 21, 2025 03:50

vkorukanti added 4 commits February 20, 2025 23:24

updates

8149917

tests

85f8047

more

9836f7c

tests

63d4376

vkorukanti force-pushed the tfIntegration branch from 745c4a4 to 63d4376 Compare February 21, 2025 11:07

allisonport-db self-requested a review February 21, 2025 17:13

vkorukanti added 2 commits February 21, 2025 10:13

add timestamp ntz support

dc03702

fmt

83183a0

scottsand-db requested review from anoopj and scottsand-db February 21, 2025 18:53

anoopj approved these changes Feb 21, 2025

View reviewed changes

fix tests

71b6245

scottsand-db reviewed Feb 21, 2025

View reviewed changes

...el/kernel-api/src/test/scala/io/delta/kernel/internal/tablefeatures/TableFeaturesSuite.scala Show resolved Hide resolved

allisonport-db mentioned this pull request Feb 21, 2025

[Kernel] Public API for setting and getting domain metadatas #4181

Merged

5 tasks

fix tests

87a974e

anoopj reviewed Feb 24, 2025

View reviewed changes

kernel/kernel-defaults/src/test/scala/io/delta/kernel/defaults/DeltaTableWritesSuite.scala Outdated Show resolved Hide resolved

anoopj reviewed Feb 24, 2025

View reviewed changes

kernel/kernel-defaults/src/test/scala/io/delta/kernel/defaults/DeltaTableWritesSuite.scala Outdated Show resolved Hide resolved

vkorukanti added 3 commits February 24, 2025 10:06

disable enabling row tracking through metadata - this is not yet supp…

1ac1ff6

…orted

modify the read/write check to just lookat the reader or writer features

fc560b6

fmt

fe73520

vkorukanti commented Feb 24, 2025

View reviewed changes

vkorukanti added 3 commits February 24, 2025 11:45

Merge remote-tracking branch 'origin/master' into tfIntegration

a641e19

fixes

455f1fc

fmt

5498ef7

allisonport-db reviewed Feb 25, 2025

View reviewed changes

allisonport-db requested changes Feb 25, 2025

View reviewed changes

vkorukanti and others added 2 commits February 25, 2025 00:46

review

30127c3

vkorukanti force-pushed the tfIntegration branch from 9b08caf to 30127c3 Compare February 25, 2025 08:46

few more tests

83c12fc

vkorukanti force-pushed the tfIntegration branch from 318b6bf to 83c12fc Compare February 25, 2025 09:33

vkorukanti requested review from scottsand-db and allisonport-db February 25, 2025 17:59

Merge remote-tracking branch 'origin/master' into tfIntegration

7dfbdb2

allisonport-db requested changes Feb 25, 2025

View reviewed changes

review/tests

e8ff801

vkorukanti force-pushed the tfIntegration branch from 5c96f68 to e8ff801 Compare February 26, 2025 00:16

vkorukanti requested a review from allisonport-db February 26, 2025 00:21

allisonport-db reviewed Feb 26, 2025

View reviewed changes

fix condition and plus others

104192f

allisonport-db approved these changes Feb 26, 2025

View reviewed changes

vkorukanti merged commit 602edad into delta-io:master Feb 26, 2025
21 checks passed

amogh-jahagirdar reviewed Feb 27, 2025

View reviewed changes

[Kernel] Integrate TableFeature framework in read and write path #4159

[Kernel] Integrate TableFeature framework in read and write path #4159

Conversation

vkorukanti commented Feb 14, 2025 • edited Loading

Description

How was this patch tested?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottsand-db left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anoopj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allisonport-db left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allisonport-db left a comment

Choose a reason for hiding this comment

allisonport-db left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allisonport-db left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allisonport-db left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amogh-jahagirdar Feb 27, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

[Kernel] Integrate `TableFeature` framework in read and write path #4159

[Kernel] Integrate `TableFeature` framework in read and write path #4159

vkorukanti commented Feb 14, 2025 •

edited

Loading

amogh-jahagirdar Feb 27, 2025 •

edited

Loading