szehon-ho commented on code in PR #56472:
URL: https://github.com/apache/spark/pull/56472#discussion_r3417849974
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveTableSpec.scala:
##########
@@ -95,6 +95,18 @@ object ResolveTableSpec extends Rule[LogicalPlan] {
messageParameters = Map("checkCondition" -> check.condition)
)
}
+ // Spark currently only supports session/temporary SQL variables
+ // (TempVariableManager); every VariableReference is therefore
session-scoped and
+ // invalid in a persisted CHECK constraint. If persistent SQL
variables are added
+ // later, tighten this to fire only on session-scoped variables.
+ check.child.foreach {
+ case v: VariableReference =>
+ throw QueryCompilationErrors
+ .notAllowedToCreateCheckConstraintReferencingTempVarError(
+ Option(check.userProvidedName).getOrElse(""),
Review Comment:
For an inline (unnamed) constraint, `userProvidedName` is null here, so
`objName` renders as empty backticks and the message reads `Cannot create the
persistent object `` of the type CHECK CONSTRAINT ...`. `TableConstraint.name`
would synthesize a name, but it depends on `tableName`, which may not be
populated yet on the create path, so passing `""` is the safe choice. Just
flagging that the resulting message is a bit awkward -- might be worth a short
comment noting the empty name is intentional.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryCompilationErrors.scala:
##########
@@ -3578,6 +3578,18 @@ private[sql] object QueryCompilationErrors extends
QueryErrorsBase with Compilat
"tempObjName" -> toSQLId(varName)))
}
+ def notAllowedToCreateCheckConstraintReferencingTempVarError(
+ constraintName: String,
+ varName: Seq[String]): Throwable = {
+ new AnalysisException(
+ errorClass = "INVALID_TEMP_OBJ_REFERENCE",
Review Comment:
Reusing `INVALID_TEMP_OBJ_REFERENCE` fits the `obj`/`tempObj` framing
nicely, but the trailing remediation in the shared template -- "...or make the
persistent object <objName> temporary" -- doesn't really apply to a CHECK
constraint (a constraint can't be made temporary). It's a reasonable trade-off
vs. adding a new error class, but worth being ready to defend the choice over
the generated-column approach (`UNSUPPORTED_EXPRESSION_GENERATED_COLUMN`) from
SPARK-57360.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala:
##########
@@ -947,6 +947,20 @@ trait CheckAnalysis extends LookupCatalog with
QueryErrorsBase with PlanToString
messageParameters = Map("checkCondition" ->
a.checkConstraint.condition)
)
+ // Spark currently only supports session/temporary SQL variables
+ // (TempVariableManager); every VariableReference is therefore
session-scoped and
+ // invalid in a persisted CHECK constraint. If persistent SQL
variables are added
+ // later, tighten this to fire only on session-scoped variables.
+ case a: AddCheckConstraint =>
+ a.checkConstraint.child.foreach {
+ case v: VariableReference =>
+ throw QueryCompilationErrors
Review Comment:
Unlike `NON_DETERMINISTIC_CHECK_CONSTRAINT` just above (whose tests assert
an `ExpectedContext` fragment), this error carries no query origin, so it won't
point at the offending SQL fragment. That matches the existing view-based
`INVALID_TEMP_OBJ_REFERENCE` usages, so it's defensible -- just noting it's
slightly less precise for the user.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveTableSpec.scala:
##########
@@ -95,6 +95,18 @@ object ResolveTableSpec extends Rule[LogicalPlan] {
messageParameters = Map("checkCondition" -> check.condition)
)
}
+ // Spark currently only supports session/temporary SQL variables
+ // (TempVariableManager); every VariableReference is therefore
session-scoped and
+ // invalid in a persisted CHECK constraint. If persistent SQL
variables are added
+ // later, tighten this to fire only on session-scoped variables.
+ check.child.foreach {
Review Comment:
Minor style: `foreach` + throw-on-first-match works, but `collectFirst {
case v: VariableReference => v }.foreach { v => throw ... }` reads a bit more
idiomatically and makes the "first offending variable" intent explicit. Same
applies to the `AddCheckConstraint` block in `CheckAnalysis.scala`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
