[SPARK-51834][SQL] Support end-to-end table constraint management #50631
Conversation
|
cc @aokolnychyi |
| val constraint = tableConstraint.toV2Constraint(isCreateTable = false) | ||
| val validatedTableVersion = table match { | ||
| case t: ResolvedTable if constraint.enforced() => | ||
| t.table.currentVersion() |
There was a problem hiding this comment.
Created a follow-up https://issues.apache.org/jira/browse/SPARK-51835 for testing the table version
| "message" : [ | ||
| "The check constraint `<checkCondition>` is non-deterministic. Check constraints must only contain deterministic expressions." | ||
| ], | ||
| "sqlState" : "42621" |
There was a problem hiding this comment.
The error code seems consistent with DB2 and what we use for generated columns, +1.
| @@ -18,11 +18,12 @@ | |||
| package org.apache.spark.sql.catalyst.analysis | |||
|
|
|||
| import org.apache.spark.SparkThrowable | |||
| import org.apache.spark.sql.catalyst.expressions.{Expression, Literal} | |||
| import org.apache.spark.sql.catalyst.expressions._ | |||
There was a problem hiding this comment.
What is the agreement in the community on wildcard imports? Are they permitted after a given number of elements are imported directly?
There was a problem hiding this comment.
As per https://github.com/databricks/scala-style-guide?tab=readme-ov-file#imports,
"Avoid using wildcard imports, unless you are importing more than 6 entities"
| Some(LocalRelation(attributeList)) | ||
| } | ||
|
|
||
| private def analyzeConstraints( |
There was a problem hiding this comment.
Is there any other way to do this? Can we restructure the plan so that the analyzer naturally resolves these expressions? I like that we pivoted to DefaultValueExpression for default values, rather than using a custom analyzer.
There was a problem hiding this comment.
I can't think of any other way.
https://github.com/apache/spark/blob/master/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/ResolveDefaultColumnsUtil.scala#L335
This is similar to what we did for column default.
There was a problem hiding this comment.
Can we do something similar to what @cloud-fan did for OverwriteByExpression in SPARK-33412?
There was a problem hiding this comment.
My worry is that we added DefaultValueExpression to eventually get rid of the custom analyzer and optimizer for default values. It would be great not to add more dependencies on it.
There was a problem hiding this comment.
There are some differences here. In a create table statement:
- column default value and option CANNOT reference columns
- constraint CAN reference columns
Using a default analyzer with a dummy relation is simple, and it can include all other analysis batches other than the main Resolution batch.
| val validateStatus = if (isCreateTable) { | ||
| Constraint.ValidationStatus.UNVALIDATED | ||
| } else { | ||
| Constraint.ValidationStatus.VALID |
There was a problem hiding this comment.
Is the idea here that we always validate existing data in ALTER?
There was a problem hiding this comment.
Yes for check constraint
| @@ -112,6 +117,27 @@ case class CheckConstraint( | |||
| with TableConstraint { | |||
| // scalastyle:on line.size.limit | |||
|
|
|||
| def toV2Constraint(isCreateTable: Boolean): Constraint = { | |||
There was a problem hiding this comment.
I wonder if the input param should be related to the validation status, rather than to whether it is create or alter. For instance, we can make validation optional in ALTER.
There was a problem hiding this comment.
ok, how about let's make all the validate status as UNVALIDATED in this PR? Once we support enforcing check constraint, we can have more discussions on this one
| case _ => | ||
| null | ||
| } | ||
| Seq(TableChange.addConstraint(constraint, validatedTableVersion)) |
There was a problem hiding this comment.
CHECK constraints must optionally validate existing data in ALTER.
Am I right this PR doesn't have this? What would be our plan?
There was a problem hiding this comment.
must optionally validate
Make sense. Do you mean CHECK ... NOT ENFOCED?
There was a problem hiding this comment.
ENFORCED/NOT ENFORCED impacts subsequent writes. I was referring to ALTER TABLE ... ADD CONSTRAINT that must scan the existing data.
What changes were proposed in this pull request?
Support end-to-end table constraint management:
Why are the changes needed?
Allow users to define and modify table constraints in connectors that support them.
Does this PR introduce any user-facing change?
No, it is for DSV2 framework.
How was this patch tested?
New UT
Was this patch authored or co-authored using generative AI tooling?
No