[Kernel] Add table feature manipulation utilities in the Protocol #4157

vkorukanti · 2025-02-14T09:35:24Z

Description

Utilities for

finding the implicitly and explicitly enabled table features
adding a feature to the protocol and creating an upgraded protocol

How was this patch tested?

Unittests.

scottsand-db

Looking good! Left some comments.

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

scottsand-db · 2025-02-14T17:13:26Z

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

-      List<String> readerFeatures,
-      List<String> writerFeatures) {
+      Set<String> readerFeatures,
+      Set<String> writerFeatures) {
    this.minReaderVersion = minReaderVersion;
    this.minWriterVersion = minWriterVersion;
    this.readerFeatures = readerFeatures;


Is there a benefit to using null?

I think I'd prefer, at the very least, that callers of getReaderFeatures and getWriterFeatures do not need to perform null checks. Perhaps storing null internally is fine (and then just expose the empty set)? But perhaps storing an empty Set would be better?

Storing an empty list, but when we are serializing to Row for writing to Delta log, written as null.

What about doing this the other way around, store null in the feature sets and return empty sets when invoking protocol.readerAndWriterFeatures? I believe this is what we do in delta.

scottsand-db · 2025-02-14T17:15:22Z

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

+  /**
+   * Get the set of features that are implicitly enabled by the protocol. Features are implicitly
+   * enabled if they reader and/or writer version is less than the versions that support the
+   * explicit features specifying in `readerFeatures` and `writerFeatures` sets.


nit: specified instead of specifying

Can you also give an example? IMO a succinct clear example is worth 1000 words of description. Ditto for other methods -- short examples would make it super clear.

scottsand-db · 2025-02-14T17:17:09Z

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

+   * enabled if they are present in the `readerFeatures` and/or `writerFeatures` sets.
+   */
+  public Set<TableFeature> getExplicitlyEnabledFeatures() {
+    // TODO: Should we throw an exception if we encounter a feature that is not known to Kernel yet?


That wouldn't happen in this method, right? That should probably happen when we construct the protocol?

i.e. if Delta Kernel is pointed to a table and the table has Delta Table readerWriter Feature foo that Kernel doesn't support, we should throw?

and again on the write path we should check if there's a writer only feature that we don't support?

Let me know if I'm missing something -- I'd figure we were already doing that today

Changed this code to go through TableFeatures::getTableFeature so that way we always through error for unknown features.

The case is: read the protocol from the delta log, and you ask for the Protocol from all enabled features and process further (check if it is supported or not). We should throw an error here for an unknown feature.

scottsand-db · 2025-02-14T17:18:51Z

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

+   * enabled if they reader and/or writer version is less than the versions that support the
+   * explicit features specifying in `readerFeatures` and `writerFeatures` sets.
+   */
+  public Set<TableFeature> getImplicitlyEnabledFeatures() {


btw -- is it implicitly "enabled" or implicitly "supported"? I think the latter, right?

I changed it to say supported everywhere.

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

anoopj · 2025-02-19T01:22:00Z

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

@@ -155,9 +155,9 @@ public Transaction build(Engine engine) {
      if (!newWriterFeatures.isEmpty()) {
        logger.info("Automatically enabling writer features: {}", newWriterFeatures);
        shouldUpdateProtocol = true;
-        List<String> oldWriterFeatures = protocol.getWriterFeatures();
+        Set<String> oldWriterFeatures = protocol.getWriterFeatures();


Not a comment, but a question on the existing code for my understanding: TableFeatures.extractAutomaticallyEnabledWriterFeatures looks for writer features. So what is the mechanism to enable reader-writer features such as timestampNtz?

Here we are just looking for the writer features (because Kernel has no support for any readerwriter feature yet).

This will be changed to TableFeatures.extractAutomaticallyEnabledFeatures which enables both readerWriter and writerOnly features, and also have methods to compute the new Protocol

Shall we change it already to TableFeatures.extractAutomaticallyEnabledFeatures and add an assertion that there is no reader+writer feature atm with FeatureAutomaticallyEnabledByMetadata trait? That way we know we won't forget that later on.

anoopj · 2025-02-19T01:22:56Z

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

-      List<String> readerFeatures,
-      List<String> writerFeatures) {
+      Set<String> readerFeatures,
+      Set<String> writerFeatures) {
    this.minReaderVersion = minReaderVersion;
    this.minWriterVersion = minWriterVersion;
    this.readerFeatures = readerFeatures;


scottsand-db

Looks great! Just a couple of questions/comments -- after this, will approve. Thanks!

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

scottsand-db · 2025-02-20T16:57:46Z

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

+   *       already supports 'invariants' implicitly.
+   *   <li>current protocol is (1, 7, writerFeature='rowTracking,domainMetadata' and the new feature
+   *       to add is 'appendOnly' -> (1, 7, writerFeature='rowTracking,domainMetadata,appendOnly')
+   *   <li>current protocol is (1, 7, writerFeature='rowTracking,domainMetadata' and the new feature


Question: so how do we (kernel developers) "upgrade" a protocol from (1, 7) to then include column mapping?

Here are the steps:

start with Protocol(3, 7)

add the required features (in this case columnMapping) and normalize it

Check if the current protocol is upgradable to the protocol created in (2) - i.e does the currentProtocol already supports what is needed (implicitly or explicitly)

If yes, we can return currentProtocol

Otherwise see what new features we are adding and can they be auto upgradeable based on the table feature properties.

This method will be in the next PR (link here)

scottsand-db · 2025-02-20T17:00:02Z

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

+   */
+  public Protocol denormalized() {
+    // Denormalization can only be applied to legacy protocols.
+    if (supportsWriterFeatures) {


I think it would be nice if we had a separate method (with some documentation) that explained the relationship between supportsWriterFeatures and is-legacy-protocol?

private def isLegacyProtocol() = supportsWriterFeatures

This would make it clearer in the code, and then you could also add some detail/comments in one spot that explaiend why?

added utilities

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

andreaschat-db

@vkorukanti thank you for doing this. I left a couple of comments.

andreaschat-db · 2025-02-20T14:29:36Z

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

    this.minReaderVersion = minReaderVersion;
    this.minWriterVersion = minWriterVersion;
-    this.readerFeatures = readerFeatures;
-    this.writerFeatures = writerFeatures;
+    this.readerFeatures = requireNonNull(readerFeatures, "readerFeatures cannot be null");


I am confused about this. Isn't supposed kernel to be able to support legacy features?

andreaschat-db · 2025-02-20T15:04:39Z

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

+    if (o == null || getClass() != o.getClass()) {
+      return false;
+    }
+    Protocol protocol = (Protocol) o;


Rename to otherProtocol?

andreaschat-db · 2025-02-20T15:35:10Z

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

+  /////////////////////////////////////////////////////////////////////////////////////////////////
+  /**
+   * Get the set of features that are implicitly supported by the protocol. Features are implicitly
+   * supported if the reader and/or writer version is less than the versions that supports the


Suggested change

* supported if the reader and/or writer version is less than the versions that supports the

* may have implicit features. The implicit feature set is determined by collecting the legacy features with reader and writer versions less or equal to the protocol's reader and writer versions.

andreaschat-db · 2025-02-20T15:35:29Z

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

+  /// Public methods related to table features interaction with the protocol                    ///
+  /////////////////////////////////////////////////////////////////////////////////////////////////
+  /**
+   * Get the set of features that are implicitly supported by the protocol. Features are implicitly


Suggested change

* Get the set of features that are implicitly supported by the protocol. Features are implicitly

* Get the set of features that are implicitly supported by the protocol. Only legacy protocols

andreaschat-db · 2025-02-20T15:38:50Z

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

+    return supportedFeatures;
+  }
+
+  /** Create a new {@link Protocol} object with the given {@link TableFeature} supported. */


Suggested change

/** Create a new {@link Protocol} object with the given {@link TableFeature} supported. */

/** Create a new {@link Protocol} object with the given {@link TableFeature} iterable supported. */

andreaschat-db · 2025-02-21T09:54:54Z

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

@@ -384,8 +393,29 @@ public static TableFeature getTableFeature(String featureName) {
    return tableFeature;
  }

+  /** Does reader version supports explicitly specifying reader feature set in protocol? */


Personally, I find the comment in delta more clean:
Check if this protocol is capable of adding features into its readerFeatures field.. Same for the writer features below.

andreaschat-db · 2025-02-21T09:55:32Z

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

+                }
+              });
+    } else {
+      // ensure we don't get (minReaderVersion, minWriterVersion) that satisfy the readerWriter


Nit: Ensure we don't etc

andreaschat-db · 2025-02-21T10:16:11Z

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

-      List<String> readerFeatures,
-      List<String> writerFeatures) {
+      Set<String> readerFeatures,
+      Set<String> writerFeatures) {
    this.minReaderVersion = minReaderVersion;
    this.minWriterVersion = minWriterVersion;
    this.readerFeatures = readerFeatures;


What about doing this the other way around, store null in the feature sets and return empty sets when invoking protocol.readerAndWriterFeatures? I believe this is what we do in delta.

andreaschat-db · 2025-02-21T10:29:02Z

kernel/kernel-api/src/test/scala/io/delta/kernel/internal/actions/ProtocolSuite.scala

+  }
+
+  // Tests for `normalized
+  Seq(


Question: Do we have DELTA_PROTOCOL_DEFAULT_READER_VERSION and DELTA_PROTOCOL_DEFAULT_WRITER_VERSION in kernel?

andreaschat-db · 2025-02-21T10:56:14Z

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

+   */
+  public Protocol withFeature(TableFeature feature) {
+    // Add required dependencies of the feature
+    Protocol protocolWithDependencies = withFeatures(feature.requiredFeatures());


Just a note, withFeatures(feature.requiredFeatures()) won't denormalize the protocol. So, if there is a depedent feature with higher reader/writer versions that won't be reflected in protocolWithDependencies . Perhaps we should add a comment it about it here. I tried to fix this in the past but other work got into the way.

vkorukanti added the kernel label Feb 14, 2025

scottsand-db requested changes Feb 14, 2025

View reviewed changes

vkorukanti mentioned this pull request Feb 14, 2025

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

Open

vkorukanti force-pushed the protocolUtils branch from 70962de to d213b1b Compare February 14, 2025 19:28

anoopj reviewed Feb 19, 2025

View reviewed changes

vkorukanti added 2 commits February 19, 2025 15:19

[Kernel] Add table feature manipulation utilities in the Protocol

6144131

add tests/reviews

1822011

vkorukanti force-pushed the protocolUtils branch from d213b1b to 1822011 Compare February 20, 2025 11:20

vkorukanti added 2 commits February 20, 2025 03:23

fmt

5a48ccf

header

f5dd189

scottsand-db requested changes Feb 20, 2025

View reviewed changes

vkorukanti added 2 commits February 20, 2025 10:33

review/fix tests

808a771

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

0537993

vkorukanti requested a review from scottsand-db February 20, 2025 18:34

scottsand-db approved these changes Feb 20, 2025

View reviewed changes

vkorukanti added 2 commits February 20, 2025 11:03

doc fix

50b1e1c

docs fix

b1a9d0d

vkorukanti merged commit c612006 into delta-io:master Feb 20, 2025
21 checks passed

andreaschat-db reviewed Feb 21, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Kernel] Add table feature manipulation utilities in the Protocol #4157

[Kernel] Add table feature manipulation utilities in the Protocol #4157

vkorukanti commented Feb 14, 2025 •

edited

Loading

scottsand-db left a comment

scottsand-db Feb 14, 2025

anoopj Feb 19, 2025

vkorukanti Feb 19, 2025

andreaschat-db Feb 21, 2025

scottsand-db Feb 14, 2025

scottsand-db Feb 14, 2025

vkorukanti Feb 20, 2025

scottsand-db Feb 14, 2025

vkorukanti Feb 19, 2025

anoopj Feb 19, 2025

vkorukanti Feb 19, 2025

andreaschat-db Feb 21, 2025

anoopj Feb 19, 2025

scottsand-db left a comment

scottsand-db Feb 20, 2025

vkorukanti Feb 20, 2025

scottsand-db Feb 20, 2025

vkorukanti Feb 20, 2025

andreaschat-db left a comment

andreaschat-db Feb 20, 2025

andreaschat-db Feb 20, 2025

andreaschat-db Feb 20, 2025

andreaschat-db Feb 20, 2025

andreaschat-db Feb 20, 2025

andreaschat-db Feb 21, 2025

andreaschat-db Feb 21, 2025

andreaschat-db Feb 21, 2025

andreaschat-db Feb 21, 2025

andreaschat-db Feb 21, 2025

	* supported if the reader and/or writer version is less than the versions that supports the
	* may have implicit features. The implicit feature set is determined by collecting the legacy features with reader and writer versions less or equal to the protocol's reader and writer versions.

	* Get the set of features that are implicitly supported by the protocol. Features are implicitly
	* Get the set of features that are implicitly supported by the protocol. Only legacy protocols

	/** Create a new {@link Protocol} object with the given {@link TableFeature} supported. */
	/** Create a new {@link Protocol} object with the given {@link TableFeature} iterable supported. */

[Kernel] Add table feature manipulation utilities in the Protocol #4157

[Kernel] Add table feature manipulation utilities in the Protocol #4157

Conversation

vkorukanti commented Feb 14, 2025 • edited Loading

Description

How was this patch tested?

scottsand-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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreaschat-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

vkorukanti commented Feb 14, 2025 •

edited

Loading