[Spark] Support dropping the CHECK constraints table feature #2987
Conversation
tomvanbussel
force-pushed
the
drop-constraints-feature
branch
from
16d455d
to
643c881
Compare
tomvanbussel
force-pushed
the
drop-constraints-feature
branch
from
643c881
to
01dd813
Compare
| "DELTA_CANNOT_DROP_CHECK_CONSTRAINT_FEATURE" : { | ||
| "message" : [ | ||
| "Cannot drop the CHECK constraints table feature.", | ||
| "The following constraints must be dropped first: <constraints>." |
There was a problem hiding this comment.
I was thinking that this a only a writer feature which means that can be safely ignored by readers that do not understand it. In that sense, dropping the feature should be allowed even if there are constraints in the table. They just need to be ignored. However, we do no need add a guard (if it is not already there) to only check constraints when the feature is enabled/supported. That is for writers that understand the feature.
On the other hand, if we do want to force this behavior it means the feature cannot be removed until the history is truncated. History validation and check is only done at the moment for all reader+writer features but there are plans to allow it for selected writer features.
There was a problem hiding this comment.
To add to that, it is good to drop constraints before dropping the feature but we should not check the history if there are constraints, i.e. actionUsesFeature.
There was a problem hiding this comment.
Requiring users to drop all constraints first before allowing the feature to be dropped is intentional. Otherwise, some users might not realize that dropping the checkConstraints feature will drop all check constraints on their table, or they may not realize that there are check constraints defined on their table. In general it's a good principle that dropping (or adding) a feature should never affect the logical state of a table, only the physical state of a table. For example: Dropping the DV feature does not undelete rows, and dropping the CM feature does not revert name changes of columns.
We do not require all constraints to be removed from the history (see the added test), only that the current state of the table does not contain any check constraints. I have implemented actionUsesFeature, but AFAIK it is never called right now, as historyContainsFeature is only called for reader features (and this is a writer feature).
| @@ -49,6 +49,13 @@ object Constraints { | |||
| /** A SQL expression to check for when writing out data. */ | |||
| case class Check(name: String, expression: Expression) extends Constraint | |||
|
|
|||
| def getCheckConstraintNames(metadata: Metadata): Seq[String] = { | |||
There was a problem hiding this comment.
nit: Brackets can be omitted.
There was a problem hiding this comment.
I prefer to keep the brackets here, as this is a multi-line method. This helps avoid unhappy surprises in the future when modifying the code. The rest of this class also use brackets for multi-line methods (even when not needed).
spark/src/test/scala/org/apache/spark/sql/delta/schema/CheckConstraintsSuite.scala
Show resolved
Hide resolved
| override def removeFeatureTracesIfNeeded(): Boolean = { | ||
| val checkConstraintNames = Constraints.getCheckConstraintNames(table.initialSnapshot.metadata) | ||
| if (checkConstraintNames.isEmpty) return false | ||
| throw DeltaErrors.cannotDropCheckConstraintFeature(checkConstraintNames) |
There was a problem hiding this comment.
I hope in the future we have a nice way of showing a warning message before the execution of commands like this. So the user can understand about the consequences of the command before proceeding. Now we are forced to make him drop the constraints one by one. Not the best CUJ.
There was a problem hiding this comment.
Agreed that dropping the constraints one-by-one is not great. On the other hand, I do like asking the user to explicitly drop these constraints themselves. A warning can be easily missed/skipped. Perhaps we can add a ALTER TABLE ... DROP CONSTRAINTS syntax in the future that allows dropping multiple constraints at once?
spark/src/main/scala/org/apache/spark/sql/delta/PreDowngradeTableFeatureCommand.scala
Show resolved
Hide resolved
spark/src/main/scala/org/apache/spark/sql/delta/TableFeature.scala
Outdated
Show resolved
Hide resolved
Which Delta project/connector is this regarding?
Description
This PR adds support for dropping the
checkConstraintstable feature using theALTER TABLE ... DROP FEATUREcommand. It throws an error if the table still contains CHECK constraints (as dropping a feature should never change the logical state of a table).How was this patch tested?
Added a test to
CheckConstraintsSuiteDoes this PR introduce any user-facing changes?
Yes,
ALTER TABLE ... DROP FEATURE checkConstraintsis now supported.