[Spark] Improve Delta Protocol Transitions #2848

andreaschat-db · 2024-04-04T12:20:49Z

Which Delta project/connector is this regarding?

Description

Currently, protocol transitions can be hard to manage. A few examples:

It is hard to predict the output of certain operations.
Once a legacy protocol transitions to a Table Features protocol it is quite hard to transition back to a legacy protocol.
Adding a feature in a protocol and then removing it might lead to a different protocol.
Adding an explicit feature to a legacy protocol always leads to a table features protocol although it might not be necessary.
Dropping features from legacy protocols is not supported. As a result, the order the features are dropped matters.
Default protocol versions are ignored in some cases.
Enabling table features by default results in feature loss in legacy protocols.
CREATE TABLE ignores any legacy versions set if there is also a table feature in the definition.

This PR proposes several protocol transition improvements in order to simplify user journeys. The high level proposal is the following:

Two protocol representations with singular operational semantics. This means that we have two ways to represent a protocol: a) The legacy representation and b) the table features representation. The latter representation is more powerful than the former, i.e the table features representation can represent all legacy protocols but the opposite is not true. This is followed by three simple rules:

All operations should be allowed to be performed on both protocol representations and should yield equivalent results.
The result should always be represented with the weaker form when possible.
Conversely, if the result of an operation on a legacy protocol cannot be represented with the legacy representation, use the Table Features representation.

The PR introduces the following behavioural changes:

Now all protocol operations are followed by denormalisation and then normalisation. Up to now, normalisation would only be performed after dropping a features.
Legacy features can now be dropped directly from a legacy protocol. The result is represented with table features if it cannot be represented with a legacy protocol.
Operations on table feature protocols now take into account the default versions. For example, enabling deletion vectors on table results to protocol (3, 7, AppendOnly, Invariants, DeletionVectors).
Operations on table feature protocols now take into account any protocol versions set on the table. For example, creating a table with protocol (1, 3) and deletion vectors results to protocol (3, 7, AppendOnly, Invariants, CheckConstraints, DeletionVectors).
It is not possible now to have a table features protocol without table features. For example, creating a table with (3, 7) and no table features is now normalised to (1, 1).
Column Mapping can now be automatically enabled on legacy protocols when the mode is changed explicitly.

How was this patch tested?

Added DeltaProtocolTransitionsSuite. Also modified existing tests in DeltaProtocolVersionSuite.

Does this PR introduce any user-facing changes?

Yes.

xupefei · 2024-04-04T13:01:02Z

One question. Consider the following command sequence:

CREATE TABLE x TBLPROPERTIES (
    delta.feature.RemovableReaderWriterFeature = 'supported',
    delta.feature.ChangeDataFeedTableFeature = 'supported')

ALTER TABLE x DROP FEATURE RemovableReaderWriterFeature

ALTER TABLE x SET TBLPROPERTIES (
  'delta.minReaderVersion' = 1,
  'delta.minWriterVersion' = 4)

Should the table get (1,4) in the end? I think it should as we automatically add all legacy features in the 3rd command.

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

andreaschat-db · 2024-04-04T13:16:35Z

One question. Consider the following command sequence:
CREATE TABLE x TBLPROPERTIES (
    delta.feature.RemovableReaderWriterFeature = 'supported',
    delta.feature.ChangeDataFeedTableFeature = 'supported')
ALTER TABLE x DROP FEATURE RemovableReaderWriterFeature
ALTER TABLE x SET TBLPROPERTIES (
  'delta.minReaderVersion' = 1,
  'delta.minWriterVersion' = 4) 
Should the table get (1,4) in the end? I think it should as we automatically add all legacy features in the 3rd command.

Very good point. Command 2 will result to Protocol(3, 7, ChangeDataFeedTableFeature). Then, command 3 will result to Protocol(3, 7, ChangeDataFeedTableFeature + rest features in (1, 4)). Dropping one of the existing legacy features cannot result in (1, 4) because the legacy features list won't match exactly. To get out of this and downgrade to (1, 4) the user will have to add a feature to the table and then drop it... I could address this in a separate PR in the future since there is a way out of it and it is a rare case, i.e., downgrade the protocol versions of a table with NO table features but with table feature versions.

zsxwing · 2024-04-05T04:00:09Z

Asking the user to set delta.minReaderVersion and delta.minWriterVersion is not user-friendly. This means users need to understand how to map a feature to minReaderVersion/minWriterVersion.

larsk-db

LGTM

larsk-db · 2024-04-05T09:43:27Z

Asking the user to set delta.minReaderVersion and delta.minWriterVersion is not user-friendly. This means users need to understand how to map a feature to minReaderVersion/minWriterVersion.

True...but it's even worse if they have to do that and add each feature individually. At least this way they can go "ok, my connector supports (x, y), let me set this on the table and then downgrade.

felipepessoto · 2024-04-07T03:35:15Z

For me it seems the existing behavior you described is better:

“For example, consider creating a table with Protocol(3, 7, RemovableReaderWriterFeature, ChangeDataFeed) and then dropping RemovableReaderWriterFeature. The resulting protocol will be Protocol(3, 7, ChangeDataFeed) instead of Protocol(1, 4)”

Protocol 3,7 besides being newer has less requirements. Meaning if you “downgrade” to 1,4 you are making your table less compatible to clients that doesn’t support all the features from writer v4, but supports v7 + CDC

bart-samwel

Regarding the user journey: this journey works when the user thinks of this ahead of time. Should we also support the reverse order:

DROP FEATURE
Set the protocol versions to (x, y).
I.e, should we "normalize" the protocol versions to a legacy protocol version whenever someone alters the protocol, not just on DROP FEATURE?

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

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

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

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

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

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

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

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

c27kwan · 2024-07-10T09:20:50Z

spark/src/test/scala/io/delta/tables/DeltaTableSuite.scala

      assert(log.update().protocol === Protocol(1, 7)
-        .merge(Protocol(1, 2)).withFeature(TestWriterFeature))
+        .withFeature(TestWriterFeature).merge(Protocol(1, 2)))
      table.addFeatureSupport(TestReaderWriterFeature.name)
      assert(
        log.update().protocol === Protocol(3, 7)
-          .merge(Protocol(1, 2))
-          .withFeatures(Seq(TestWriterFeature, TestReaderWriterFeature)))
+          .withFeatures(Seq(TestWriterFeature, TestReaderWriterFeature))
+          .merge(Protocol(1, 2)))


nit: it wasn't clear to me why this change was necessary. It seems a bit error-prone to write the expected result this way. Let's change it to be an explicit Protocol (without merging two protocol objects).

Yes I agree. I changed it. Unfortunately, we use merge very often in the tests for validating results and it would be a pain to change all of them.

The order matters because of a check in withFeatures that requires table feature versions when adding a table feature. Merge would now normalize (1, 7) + (1, 2) to (1, 1). So, (1, 1).withFeature would produce an error.

c27kwan · 2024-07-10T09:27:27Z

spark/src/test/scala/org/apache/spark/sql/delta/DeltaColumnMappingSuite.scala

+           |'$minReaderKey' = '3',
+           |'$minWriterKey' = '7',
+           |'${DeltaConfigs.ENABLE_DELETION_VECTORS_CREATION.key}' = 'true'


You shouldn't need this change as part of this PR, right? I initially made the minReaderKey 2 to expose a bug: a655bed

Setting table feature versions without a table feature is now always normalised to (1, 1). I modified the test to capture your initial intention.

spark/src/main/scala/org/apache/spark/sql/delta/DeltaColumnMapping.scala

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

c27kwan · 2024-07-10T09:35:01Z

.../src/test/scala/org/apache/spark/sql/delta/columnmapping/DropColumnMappingFeatureSuite.scala

+         |        'delta.minReaderVersion' = '3',
+         |        'delta.minWriterVersion' = '7')


Why did we need to update this test? the minReaderVersion and minWriterVersion are "suggestions" and are ignored anyways since we enable DVs which will make this table have (3,7) in the end anyways. Are we enforcing users' minReaderVersion and minWriterVersion after this change?

This is changed so the protocol always ends up to (1, 1) in testDroppingColumnMapping independently of whether we enable DVs or not. As a result, the validation at the end of verifyDropFeatureTruncateHistory does not need to change whether this is a table features or not.

With the new semantics there are 2 cases:

Enabling columnMapping on a legacy protocol results to (2, 5).

Enabling columnMapping on a table features protocol results to (2, 7, ColumnMapping).

c27kwan · 2024-07-10T09:43:52Z

spark/src/test/scala/org/apache/spark/sql/delta/DeltaTableFeatureSuite.scala

  }

  test("protocol upgrade compatibility") {
    assert(Protocol(1, 1).canUpgradeTo(Protocol(1, 1)))
    assert(Protocol(1, 1).canUpgradeTo(Protocol(2, 1)))
-    assert(!Protocol(1, 2).canUpgradeTo(Protocol(1, 1)))
-    assert(!Protocol(2, 2).canUpgradeTo(Protocol(2, 1)))
    assert(


I'm a bit concerned that this needed to be removed in this PR. Intuitively, (1, 2) -> (1,1) is not an "upgrade" so as a developer I expect this to return false. Have we changed the definition of canUpgradeTo to mean canTransitionTo?

This change here is orthogonal and it is not needed by anything else done in this PR. The idea came up in early iterations of this PR with @bart-samwel to simplify canUpgradeTo.

spark/src/main/scala/org/apache/spark/sql/delta/sources/DeltaSQLConf.scala

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

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

spark/src/main/scala/org/apache/spark/sql/delta/sources/DeltaSQLConf.scala

spark/src/test/scala-spark-master/org/apache/spark/sql/delta/DeltaVariantSuite.scala

larsk-db · 2024-07-10T14:47:35Z

spark/src/test/scala/org/apache/spark/sql/delta/DeltaErrorsSuite.scala

@@ -25,6 +25,7 @@ import java.util.Locale
 import scala.sys.process.Process

 // scalastyle:off import.ordering.noEmptyLine
+// scalastyle:off line.size.limit


This seems unnecessary when only deleting stuff?

The issue with that one is that was contained in the piece of code I deleted and it turns out it is needed by the rest of the code below... I think what happened is that there is a bug below were the style is not turned back on. So the style was not enforced when follow up code was added.

Ah, but then, don't add it at the top, but just scope it where it's needed, please? Or is just sooooo many places that that would be crazy?

I am afraid there are 38 errors :(.

That are all non-consecutive?

I am afraid they are not.

spark/src/test/scala/org/apache/spark/sql/delta/schema/InvariantEnforcementSuite.scala

scottsand-db · 2024-07-11T16:03:58Z

There are spark master test failures:

https://github.com/delta-io/delta/actions/runs/9891350186/job/27321586977?pr=2848

[info] CreateCheckpointSuite:
[info] - commits containing adds and removes, and no previous checkpoint
[info] - commits containing adds, and no previous checkpoint
[info] - commits containing adds and removes, and a previous checkpoint created using Spark (actions/perfile): 1000000
[info] - commits containing adds and removes, and a previous checkpoint created using Spark (actions/perfile): 3
[info] - commits containing adds, and a previous checkpoint created using Spark (actions/perfile): 1000000
[info] - commits containing adds, and a previous checkpoint created using Spark (actions/perfile): 3
[info] - commits with metadata updates
[info] - commits with protocol updates *** FAILED ***
[info]   == Results ==
[info]   
[info]   == Expected Answer - 1 ==
[info]   ([2,2])
[info]   
[info]   == Result - 1 ==
[info]   ([1,2]) (TestUtils.scala:386)

andreaschat-db · 2024-07-11T17:18:46Z

There are spark master test failures:

https://github.com/delta-io/delta/actions/runs/9891350186/job/27321586977?pr=2848

[info] CreateCheckpointSuite:
[info] - commits containing adds and removes, and no previous checkpoint
[info] - commits containing adds, and no previous checkpoint
[info] - commits containing adds and removes, and a previous checkpoint created using Spark (actions/perfile): 1000000
[info] - commits containing adds and removes, and a previous checkpoint created using Spark (actions/perfile): 3
[info] - commits containing adds, and a previous checkpoint created using Spark (actions/perfile): 1000000
[info] - commits containing adds, and a previous checkpoint created using Spark (actions/perfile): 3
[info] - commits with metadata updates
[info] - commits with protocol updates *** FAILED ***
[info]   == Results ==
[info]   
[info]   == Expected Answer - 1 ==
[info]   ([2,2])
[info]   
[info]   == Result - 1 ==
[info]   ([1,2]) (TestUtils.scala:386)

The PR is not ready to be merged. The particular failure is fixed by #3356.

xupefei reviewed Apr 4, 2024

View reviewed changes

spark/src/test/scala/org/apache/spark/sql/delta/DeltaProtocolVersionSuite.scala Outdated Show resolved Hide resolved

xupefei reviewed Apr 4, 2024

View reviewed changes

spark/src/test/scala/org/apache/spark/sql/delta/DeltaProtocolVersionSuite.scala Outdated Show resolved Hide resolved

xupefei reviewed Apr 4, 2024

View reviewed changes

spark/src/test/scala/org/apache/spark/sql/delta/DeltaProtocolVersionSuite.scala Outdated Show resolved Hide resolved

andreaschat-db requested a review from xupefei April 4, 2024 13:44

larsk-db approved these changes Apr 5, 2024

View reviewed changes

bart-samwel reviewed Apr 11, 2024

View reviewed changes

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

andreaschat-db force-pushed the addLegacyFeature branch from a8e526e to 7ce67d0 Compare April 16, 2024 12:45

andreaschat-db requested a review from bart-samwel April 17, 2024 07:53

bart-samwel reviewed Apr 23, 2024

View reviewed changes

andreaschat-db force-pushed the addLegacyFeature branch from 3c1ffa6 to 09c8065 Compare May 8, 2024 11:19

andreaschat-db requested a review from bart-samwel May 10, 2024 05:21

bart-samwel reviewed May 10, 2024

View reviewed changes

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

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

andreaschat-db mentioned this pull request May 13, 2024

[Spark] Protocol version downgrade in the presence of table features #2841

Merged

5 tasks

andreaschat-db force-pushed the addLegacyFeature branch from 09c8065 to 387a9b5 Compare May 14, 2024 12:50

andreaschat-db requested a review from bart-samwel May 15, 2024 06:32

bart-samwel reviewed May 17, 2024

View reviewed changes

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

andreaschat-db requested a review from bart-samwel May 21, 2024 08:33

bart-samwel reviewed May 28, 2024

View reviewed changes

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

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

andreaschat-db force-pushed the addLegacyFeature branch 2 times, most recently from b50aa8f to 613fc3c Compare June 27, 2024 09:17

andreaschat-db added 2 commits June 30, 2024 07:07

Squash.

c88061c

Improve normalization

ee781d6

andreaschat-db force-pushed the addLegacyFeature branch from 613fc3c to ee781d6 Compare June 30, 2024 05:08

andreaschat-db added 2 commits June 30, 2024 09:11

Add protocol denormalization

7c4e1af

Protocol version defaults + suite fixes

ce5e24e

andreaschat-db added 2 commits July 9, 2024 09:52

Move protocol transition tests

9ee0a8c

Revert CreateCheckpointSuite fix

b5d106d

andreaschat-db requested review from xupefei and larsk-db July 9, 2024 10:56

andreaschat-db added 2 commits July 9, 2024 13:44

Fix import ordering

a0a6933

Added a few more tests with column mapping

2f7492a

c27kwan reviewed Jul 10, 2024

View reviewed changes

Address Carmen's comments

c132361

andreaschat-db requested a review from c27kwan July 10, 2024 14:14

larsk-db reviewed Jul 10, 2024

View reviewed changes

andreaschat-db added 2 commits July 11, 2024 09:58

Address Lar's comments

1c253cd

More fixes.

56c79b2

andreaschat-db added 8 commits July 12, 2024 14:32

Fix

8d30a28

Trigger CI

29ba78f

Fix

8587944

Fix forTableFeature

de8f6e0

Minor test fixes

f448004

Improvements batch 3.

c9d734b

More improvements.

02ac929

Nits.

43b8f73

andreaschat-db requested a review from bart-samwel July 16, 2024 14:46

andreaschat-db added 5 commits July 16, 2024 17:01

Scala style.

e573cda

Nit

476a7fa

nit

2aa81eb

Trigger CI

8a849ce

Trigger CI

27f9e01

allisonport-db merged commit 669dca9 into delta-io:master Jul 17, 2024
10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Spark] Improve Delta Protocol Transitions #2848

[Spark] Improve Delta Protocol Transitions #2848

andreaschat-db commented Apr 4, 2024 •

edited

Loading

xupefei commented Apr 4, 2024

andreaschat-db commented Apr 4, 2024

zsxwing commented Apr 5, 2024

larsk-db left a comment

larsk-db commented Apr 5, 2024

felipepessoto commented Apr 7, 2024

bart-samwel left a comment

c27kwan Jul 10, 2024

andreaschat-db Jul 10, 2024 •

edited

Loading

c27kwan Jul 10, 2024

andreaschat-db Jul 10, 2024

c27kwan Jul 10, 2024

andreaschat-db Jul 10, 2024

c27kwan Jul 10, 2024

andreaschat-db Jul 10, 2024

larsk-db Jul 10, 2024

andreaschat-db Jul 10, 2024

larsk-db Jul 11, 2024

andreaschat-db Jul 11, 2024

larsk-db Jul 11, 2024

andreaschat-db Jul 11, 2024

scottsand-db commented Jul 11, 2024

andreaschat-db commented Jul 11, 2024

		\| 'delta.minReaderVersion' = '3',
		\| 'delta.minWriterVersion' = '7')

[Spark] Improve Delta Protocol Transitions #2848

[Spark] Improve Delta Protocol Transitions #2848

Conversation

andreaschat-db commented Apr 4, 2024 • edited Loading

Which Delta project/connector is this regarding?

Description

How was this patch tested?

Does this PR introduce any user-facing changes?

xupefei commented Apr 4, 2024

andreaschat-db commented Apr 4, 2024

zsxwing commented Apr 5, 2024

larsk-db left a comment

Choose a reason for hiding this comment

larsk-db commented Apr 5, 2024

felipepessoto commented Apr 7, 2024

bart-samwel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreaschat-db Jul 10, 2024 • 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

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 commented Jul 11, 2024

andreaschat-db commented Jul 11, 2024

andreaschat-db commented Apr 4, 2024 •

edited

Loading

andreaschat-db Jul 10, 2024 •

edited

Loading