Allow restricting table features to auto-update protocol versions #1660

xupefei · 2023-03-23T08:14:53Z

Description

This PR allows marking a metadata table feature as "auto-update capable", such that during ALTER TABLE when its metadata requirements are satisfied, the table's protocol will be automatically and silently bumped to the required version.

Note that this mechanism only applies to "metadata requirements". 'delta.feature.featureName' = 'supported' table prop will still auto-update the table's protocol to support table features.

Also, this mechanism only affects existing tables. For new tables, the protocol is always set to the min version that satisfies all enabled features, aka. all features are auto-update capable.

For compatibility, legacy table features (features supported by Protocol(2, 6)) are always auto-update capable. Specifically, Column Mapping implements its own mechanism to block the usage without protocol version bumps.

How was this patch tested?

New tests.

Does this PR introduce any user-facing changes?

No. This PR affects only feature developers.

c27kwan

Hi, pushing parts of my review + questions. Will review the rest later. :)

c27kwan · 2023-03-29T08:57:21Z

core/src/main/resources/error/delta-error-classes.json

+      "Your table schema requires the following table feature(s) that require manual enablement: <features>.",
+      "",
+      "To do this, run the following command for each of features listed above:",
+      "  ALTER TABLE table_name SET TBLPROPERTIES ('delta.feature.feature_name' = 'supported')",
+      "Replace \"table_name\" and \"feature_name\" with real values.",
+      "",
+      "Required Delta protocol: <requiredVersion>",
+      "Current Delta protocol: <currentVersion>"
+    ],


Q: Does that mean we now allow users to specify support for all table features? I thought they were not meant to be user-facing.

Nevermind, they are allowed to be user-facing. So a user can specify support for DVs like this or add CDC support and change the metadata at the same time, if they wish 🤔

A feature can also be controlled exclusively by table features, such as TimestampNtz: #1626. In this case the config must be user-facing.

core/src/main/resources/error/delta-error-classes.json

c27kwan · 2023-03-29T09:00:29Z

core/src/main/resources/error/delta-error-classes.json

+      "Your table schema requires the following table feature(s) that require manual enablement: <features>.",
+      "",
+      "To do this, run the following command for each of features listed above:",
+      "  ALTER TABLE table_name SET TBLPROPERTIES ('delta.feature.feature_name' = 'supported')",


Table name can be passed in to this error message?

Seems not trivial because we have only the Delta Log instance which is based on a path. The name is already lost at this stage.

core/src/main/resources/error/delta-error-classes.json

core/src/main/scala/org/apache/spark/sql/delta/OptimisticTransaction.scala

c27kwan · 2023-03-29T09:19:26Z

core/src/main/scala/org/apache/spark/sql/delta/OptimisticTransaction.scala

+   * ([[FeatureAutomaticallyEnabledByMetadata.automaticallyUpdateProtocolOfExistingTables]] is set
+   * to `true`), or being supported in the same transaction via a table property.
+   */
+  private def assertTableFeaturesAutomaticallySupported(


Based on the description, rename to assertTableFeaturesSupportedByProtocol? It doesn't necessarily only check automatic supports.

How about assertRequiredFeaturesCanBeSupportedByProtocol 😄?

core/src/test/scala/org/apache/spark/sql/delta/DeltaTimestampNTZSuite.scala

core/src/main/scala/org/apache/spark/sql/delta/OptimisticTransaction.scala

c27kwan · 2023-03-29T09:28:09Z

core/src/main/scala/org/apache/spark/sql/delta/TableFeature.scala

+ * When the feature's metadata requirements are satisfied for <b>new tables</b>, or for
+ * <b>existing tables when [[automaticallyUpdateProtocolOfExistingTables]] set to `true`</b>, the


Do we allow html annotations in our doc comments? I thought it was just markdown?

I believe so... Someone could correct me if I'm wrong

c27kwan

Here's the other half of the review. Looks good, just some more nits and questions.

core/src/main/scala/org/apache/spark/sql/delta/actions/actions.scala

c27kwan · 2023-03-29T11:39:02Z

core/src/test/scala/org/apache/spark/sql/delta/DeltaProtocolVersionSuite.scala

+    Map(TestReaderWriterMetadataNoAutoUpdateFeature.TABLE_PROP_KEY -> "true"),
+    tableProtocol =
+      Protocol(TABLE_FEATURES_MIN_READER_VERSION, TABLE_FEATURES_MIN_WRITER_VERSION),
+    expectedExceptionClass = Some("DELTA_FEATURES_REyQUIRE_MANUAL_ENABLEMENT"))


nit: AFAICT, for this test function this is always the same exception? Can we change it to always intercept the right exception and only make it more generic when it is necessary? Class string matching is more prone to mistakes where we make a typo and it compiles, but the test fails.

I discovered I didn't do assert around intercept[] so the test can pass even with the typo 😂

core/src/test/scala/org/apache/spark/sql/delta/DeltaProtocolVersionSuite.scala

c27kwan · 2023-03-29T11:52:21Z

core/src/test/scala/org/apache/spark/sql/delta/DeltaProtocolVersionSuite.scala

+            s"ALTER TABLE delta.`${dir.getCanonicalPath}` SET TBLPROPERTIES (" +
+              s"  '${TestReaderWriterMetadataAutoUpdateFeature.TABLE_PROP_KEY}' = 'true'," +
+              s"  '${TestWriterMetadataNoAutoUpdateFeature.TABLE_PROP_KEY}' = 'true')")
+        }


It's definitely interesting to test the combination of settings multiple features together. Do we already have test for the combination of two different auto-update features (e.g. CDC and Column Mapping)? A test for auto-update feature with the same feature delta.feature.xx = true/enabled/supported being set manually? For an auto-update feature that is set to not supported(e.g. default in the session), make sure it doesn't accidentally do a protocol update?

I added tests from this matrix:

legacy vs TF protocol

non-auto-update feature vs auto-update feature

enabled via metadata vs via delta.feature.xxx

Also the following:

a non-auto-update feature enabled via both metadata and delta.feature.xxx

a non-auto-update feature, an auto-update feature, and some legacy features enabled via metadata

the above with a session default

Should be enough.

core/src/test/scala/org/apache/spark/sql/delta/DeltaProtocolVersionSuite.scala

core/src/main/scala/org/apache/spark/sql/delta/OptimisticTransaction.scala

core/src/main/scala/org/apache/spark/sql/delta/actions/TableFeatureSupport.scala

core/src/main/scala/org/apache/spark/sql/delta/actions/actions.scala

core/src/main/scala/org/apache/spark/sql/delta/OptimisticTransaction.scala

core/src/main/scala/org/apache/spark/sql/delta/actions/actions.scala

…e-feature

core/src/main/scala/org/apache/spark/sql/delta/actions/actions.scala

core/src/test/scala/org/apache/spark/sql/delta/DeltaProtocolVersionSuite.scala

core/src/main/scala/org/apache/spark/sql/delta/actions/actions.scala

bart-samwel

Just nits left from me. Please still process those!

implementation

e12e2c9

c27kwan reviewed Mar 29, 2023

View reviewed changes

address most comments

ab6f68e

allisonport-db reviewed Mar 30, 2023

View reviewed changes

core/src/main/scala/org/apache/spark/sql/delta/OptimisticTransaction.scala Outdated Show resolved Hide resolved

bart-samwel reviewed Mar 30, 2023

View reviewed changes

xupefei added 3 commits March 31, 2023 15:12

address comments

8f0bcd7

address comments

d61ba90

even better test

6b1532a

xupefei requested review from c27kwan, allisonport-db and bart-samwel March 31, 2023 13:01

c27kwan reviewed Mar 31, 2023

View reviewed changes

xupefei added 2 commits April 3, 2023 15:27

Merge branch 'master' of github.com:delta-io/delta into tf-auto-updat…

ee2bf6e

…e-feature

address comments

b156872

xupefei requested a review from c27kwan April 3, 2023 10:01

bart-samwel reviewed Apr 3, 2023

View reviewed changes

xupefei added 2 commits April 3, 2023 22:48

address comments

9b500a2

address comments

fe7361f

xupefei requested a review from bart-samwel April 3, 2023 14:57

bart-samwel reviewed Apr 3, 2023

View reviewed changes

core/src/main/scala/org/apache/spark/sql/delta/actions/actions.scala Outdated Show resolved Hide resolved

bart-samwel reviewed Apr 3, 2023

View reviewed changes

core/src/main/scala/org/apache/spark/sql/delta/actions/actions.scala Outdated Show resolved Hide resolved

bart-samwel approved these changes Apr 3, 2023

View reviewed changes

address comments

74d39a9

scottsand-db closed this in 73d0ff1 Apr 13, 2023

xupefei deleted the tf-auto-update-feature branch April 19, 2023 19:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow restricting table features to auto-update protocol versions #1660

Allow restricting table features to auto-update protocol versions #1660

xupefei commented Mar 23, 2023

c27kwan left a comment

c27kwan Mar 29, 2023

c27kwan Mar 29, 2023

xupefei Mar 29, 2023

c27kwan Mar 29, 2023

xupefei Mar 29, 2023

c27kwan Mar 29, 2023

xupefei Mar 29, 2023

c27kwan Mar 29, 2023

xupefei Mar 29, 2023

c27kwan left a comment

c27kwan Mar 29, 2023

xupefei Mar 29, 2023

c27kwan Mar 29, 2023

xupefei Mar 31, 2023

bart-samwel left a comment

		* When the feature's metadata requirements are satisfied for <b>new tables</b>, or for
		* <b>existing tables when [[automaticallyUpdateProtocolOfExistingTables]] set to `true`</b>, the

Allow restricting table features to auto-update protocol versions #1660

Allow restricting table features to auto-update protocol versions #1660

Conversation

xupefei commented Mar 23, 2023

Description

How was this patch tested?

Does this PR introduce any user-facing changes?

c27kwan 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

c27kwan 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

bart-samwel left a comment

Choose a reason for hiding this comment