-
Notifications
You must be signed in to change notification settings - Fork 1.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Spark] Protocol version downgrade in the presence of table features #2841
[Spark] Protocol version downgrade in the presence of table features #2841
Conversation
spark/src/main/scala/org/apache/spark/sql/delta/actions/TableFeatureSupport.scala
Outdated
Show resolved
Hide resolved
spark/src/main/scala/org/apache/spark/sql/delta/actions/TableFeatureSupport.scala
Outdated
Show resolved
Hide resolved
spark/src/main/scala/org/apache/spark/sql/delta/actions/TableFeatureSupport.scala
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm!
spark/src/main/scala/org/apache/spark/sql/delta/actions/TableFeatureSupport.scala
Outdated
Show resolved
Hide resolved
spark/src/main/scala/org/apache/spark/sql/delta/actions/TableFeatureSupport.scala
Outdated
Show resolved
Hide resolved
spark/src/main/scala/org/apache/spark/sql/delta/actions/TableFeatureSupport.scala
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Is the description correct? Reader v1 doesn’t support DV |
I think the intent of "can be downgraded" was an implicit "after removing the DV feature". |
spark/src/main/scala/org/apache/spark/sql/delta/actions/TableFeatureSupport.scala
Outdated
Show resolved
Hide resolved
spark/src/main/scala/org/apache/spark/sql/delta/actions/TableFeatureSupport.scala
Outdated
Show resolved
Hide resolved
spark/src/main/scala/org/apache/spark/sql/delta/actions/TableFeatureSupport.scala
Outdated
Show resolved
Hide resolved
spark/src/main/scala/org/apache/spark/sql/delta/actions/TableFeatureSupport.scala
Outdated
Show resolved
Hide resolved
spark/src/main/scala/org/apache/spark/sql/delta/actions/TableFeatureSupport.scala
Outdated
Show resolved
Hide resolved
spark/src/main/scala/org/apache/spark/sql/delta/actions/TableFeatureSupport.scala
Outdated
Show resolved
Hide resolved
spark/src/main/scala/org/apache/spark/sql/delta/actions/TableFeatureSupport.scala
Outdated
Show resolved
Hide resolved
spark/src/main/scala/org/apache/spark/sql/delta/actions/TableFeatureSupport.scala
Outdated
Show resolved
Hide resolved
d9d7845
to
3a4db9e
Compare
spark/src/main/scala/org/apache/spark/sql/delta/actions/TableFeatureSupport.scala
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One final nit. Let's ship it!
Which Delta project/connector is this regarding?
Description
This PR adds support for protocol versions downgrade when table features exist in the protocol. The downgraded protocol versions should be the minimum required to support all available table features. For example,
Protocol(3, 7, DeletionVectors, RowTracking)
can be downgraded toProtocol(1, 7, RowTracking)
after removing the DV feature.How was this patch tested?
Added new UTs in DeltaProtocolVersionSuite. Furthermore, existing UTs cover a significant part of the functionality. These these are the following:
Does this PR introduce any user-facing changes?
Yes. Dropping a table feature from a table with multiple features may now result to a Protocol versions downgrade. For example,
Protocol(3, 7, DeletionVectors, RowTracking)
can now be downgraded toProtocol(1, 7, RowTracking)
.