You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "yadavay-amzn (via GitHub)" <gi...@apache.org> on 2026/06/12 17:57:31 UTC
[PR] [SPARK-57379][SQL] Block temporary variables in CHECK constraint expressions [spark]
yadavay-amzn opened a new pull request, #56472:
URL: https://github.com/apache/spark/pull/56472
### What changes were proposed in this pull request?
Block references to temporary (session) variables in CHECK constraint expressions, on both the `CREATE`/`REPLACE TABLE` path and the `ALTER TABLE ... ADD CONSTRAINT` path. A CHECK constraint that references a session variable is now rejected at analysis time with `INVALID_TEMP_OBJ_REFERENCE`.
The check mirrors the existing non-deterministic-constraint validation and the sibling fix for generated columns (SPARK-57360): it detects a `VariableReference` anywhere in the constraint expression via `containsPattern(VARIABLE_REFERENCE)` and raises `INVALID_TEMP_OBJ_REFERENCE` (the same error class already used when a persistent view references a temporary variable).
### Why are the changes needed?
A CHECK constraint is persisted with the table, but a temporary/session variable is session-scoped and will not exist in other sessions, making the persisted constraint invalid. Today both paths silently accept it:
```sql
DECLARE OR REPLACE VARIABLE my_var INT DEFAULT 5;
-- Both currently succeed and persist `CHECK (val > my_var)`:
CREATE TABLE t (id INT, val INT, CONSTRAINT c1 CHECK (val > my_var)) USING parquet;
CREATE TABLE t2 (id INT, val INT) USING parquet;
ALTER TABLE t2 ADD CONSTRAINT c2 CHECK (val > my_var);
```
Spark already blocks the analogous case for persistent views via `INVALID_TEMP_OBJ_REFERENCE`; this extends the same protection to CHECK constraints.
### Does this PR introduce _any_ user-facing change?
Yes. Creating or altering a table with a CHECK constraint that references a temporary variable now fails with `INVALID_TEMP_OBJ_REFERENCE` instead of silently persisting an invalid constraint. No previously-valid constraint is affected.
### How was this patch tested?
New tests in `CheckConstraintSuite` covering `CREATE TABLE`, `REPLACE TABLE`, and `ALTER TABLE ADD CONSTRAINT`, including a nested reference (`CHECK (i > CAST(my_var AS BIGINT))`) to confirm the detection is recursive, plus existing constraint tests as a regression guard. Verified the tests fail without the fix.
### Was this patch authored or co-authored using generative AI tooling?
Yes.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57379][SQL] Block temporary variables in CHECK constraint expressions [spark]
Posted by "yadavay-amzn (via GitHub)" <gi...@apache.org>.
yadavay-amzn commented on code in PR #56472:
URL: https://github.com/apache/spark/pull/56472#discussion_r3422893440
##########
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:
Switched both call sites to `child.collectFirst` style
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57379][SQL] Block temporary variables in CHECK constraint expressions [spark]
Posted by "uros-b (via GitHub)" <gi...@apache.org>.
uros-b commented on code in PR #56472:
URL: https://github.com/apache/spark/pull/56472#discussion_r3443310236
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala:
##########
@@ -947,6 +947,21 @@ trait CheckAnalysis extends LookupCatalog with QueryErrorsBase with PlanToString
messageParameters = Map("checkCondition" -> a.checkConstraint.condition)
)
+ // Reject session/temp variables in persisted CHECK constraints.
+ case a: AddCheckConstraint =>
+ a.checkConstraint.child.collectFirst {
+ case v: VariableReference => v
+ }.foreach { v =>
+ v.failAnalysis(
Review Comment:
Nit about code duplication: the ~13-line variable-detection block is copy-pasted into both ResolveTableSpec.scala (lines ~98-112) and CheckAnalysis.scala (lines ~950-962). Please consider using a shared helper (e.g. on CheckConstraint or a small util) to reduce drift risk if the message/logic ever changes.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57379][SQL] Block temporary variables in CHECK constraint expressions [spark]
Posted by "szehon-ho (via GitHub)" <gi...@apache.org>.
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57379][SQL] Block temporary variables in CHECK constraint expressions [spark]
Posted by "yadavay-amzn (via GitHub)" <gi...@apache.org>.
yadavay-amzn commented on code in PR #56472:
URL: https://github.com/apache/spark/pull/56472#discussion_r3422863571
##########
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
Review Comment:
Done
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57379][SQL] Block temporary variables in CHECK constraint expressions [spark]
Posted by "yadavay-amzn (via GitHub)" <gi...@apache.org>.
yadavay-amzn commented on code in PR #56472:
URL: https://github.com/apache/spark/pull/56472#discussion_r3422945485
##########
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:
confirmed both `userProvidedName` and `tableName` are null at this point on the CREATE path with no name-synthesis helper available, so "" is the only safe value.
Kept it and added a short comment noting the empty name is intentional, as you suggested.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57379][SQL] Block temporary variables in CHECK constraint expressions [spark]
Posted by "yadavay-amzn (via GitHub)" <gi...@apache.org>.
yadavay-amzn commented on PR #56472:
URL: https://github.com/apache/spark/pull/56472#issuecomment-4722148404
Thanks @szehon-ho for the feedback.
Addressed all comments (replies are inline).
On `INVALID_TEMP_OBJ_REFERENCE`, I kept it to match the existing persistent-view precedent for the same "persistent object references a temporary object" category and to avoid growing the error-class surface.
You're right that the shared template's "...or make the persistent object temporary" remediation doesn't map cleanly to a constraint so if you'd prefer a dedicated *_CHECK_CONSTRAINT error class instead, I'm happy to switch.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57379][SQL] Block temporary variables in CHECK constraint expressions [spark]
Posted by "yadavay-amzn (via GitHub)" <gi...@apache.org>.
yadavay-amzn commented on PR #56472:
URL: https://github.com/apache/spark/pull/56472#issuecomment-4850076913
Thanks for the reviews. Addressed @uros-b's dedup nit by extracting CheckConstraint.rejectTempVariables into the companion object, with both call sites delegating to it (error class and message parameters preserved; CheckConstraintSuite green). @szehon-ho @uros-b this should be ready for another look when you have a moment.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57379][SQL] Block temporary variables in CHECK constraint expressions [spark]
Posted by "yadavay-amzn (via GitHub)" <gi...@apache.org>.
yadavay-amzn commented on PR #56472:
URL: https://github.com/apache/spark/pull/56472#issuecomment-5075982328
@uros-b @szehon-ho Could you please take another look when you have the chance?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57379][SQL] Block temporary variables in CHECK constraint expressions [spark]
Posted by "yadavay-amzn (via GitHub)" <gi...@apache.org>.
yadavay-amzn commented on code in PR #56472:
URL: https://github.com/apache/spark/pull/56472#discussion_r3423146293
##########
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:
That's a good point. I switched both throws to `failAnalysis` on the offending `VariableReference`, so the error now carries the query context and points at the variable (e.g. `my_var`), consistent with the adjacent `NON_DETERMINISTIC_CHECK_CONSTRAINT`. Updated the tests to assert the `ExpectedContext`, and removed the now-unused `QueryCompilationErrors` helper.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57379][SQL] Block temporary variables in CHECK constraint expressions [spark]
Posted by "szehon-ho (via GitHub)" <gi...@apache.org>.
szehon-ho commented on code in PR #56472:
URL: https://github.com/apache/spark/pull/56472#discussion_r3417822891
##########
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
Review Comment:
nit: the comment is too wordy, i would simplify it (ie, no need to mention the permanent variables that dont exist yet)
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/command/v2/CheckConstraintSuite.scala:
##########
@@ -78,6 +78,93 @@ class CheckConstraintSuite extends QueryTest with CommandSuiteBase with DDLComma
}
}
+ test("SPARK-57379: Temporary variable in check constraint -- alter table") {
+ withNamespaceAndTable("ns", "tbl", nonPartitionCatalog) { t =>
+ sql(s"CREATE TABLE $t (i INT) $defaultUsing")
+ withSessionVariable("my_var") {
+ sql("DECLARE OR REPLACE VARIABLE my_var INT DEFAULT 1")
+ Seq(
+ s"ALTER TABLE $t ADD CONSTRAINT c1 CHECK (i > my_var)",
+ s"ALTER TABLE $t ADD CONSTRAINT c1 CHECK (i > CAST(my_var AS BIGINT))"
+ ).foreach { query =>
+ val error = intercept[AnalysisException] {
+ sql(query)
+ }
+ checkError(
+ exception = error,
+ condition = "INVALID_TEMP_OBJ_REFERENCE",
+ sqlState = "42K0F",
+ parameters = Map(
+ "obj" -> "CHECK CONSTRAINT",
+ "objName" -> "`c1`",
+ "tempObj" -> "VARIABLE",
+ "tempObjName" -> "`my_var`")
+ )
+ }
+ }
+ }
+ }
+
+ test("SPARK-57379: Temporary variable in check constraint -- create table") {
+ withSessionVariable("my_var") {
+ sql("DECLARE OR REPLACE VARIABLE my_var INT DEFAULT 1")
+ Seq(
+ ("CREATE TABLE t(i INT CHECK (i > my_var))", "``"),
+ ("CREATE TABLE t(i INT, CONSTRAINT c1 CHECK (i > my_var))", "`c1`"),
+ ("REPLACE TABLE t(i INT CHECK (i > my_var))", "``"),
+ ("REPLACE TABLE t(i INT, CONSTRAINT c1 CHECK (i > my_var))", "`c1`"),
+ ("CREATE TABLE t(i INT CHECK (i > CAST(my_var AS BIGINT)))", "``")
+ ).foreach { case (query, expectedObjName) =>
+ withTable("t") {
+ val error = intercept[AnalysisException] {
+ sql(query)
+ }
+ checkError(
+ exception = error,
+ condition = "INVALID_TEMP_OBJ_REFERENCE",
+ sqlState = "42K0F",
+ parameters = Map(
+ "obj" -> "CHECK CONSTRAINT",
+ "objName" -> expectedObjName,
+ "tempObj" -> "VARIABLE",
+ "tempObjName" -> "`my_var`")
+ )
+ }
+ }
+ }
+ }
+
+ test("SPARK-57379: Temporary variable in check constraint -- qualified variable name") {
+ withNamespaceAndTable("ns", "tbl", nonPartitionCatalog) { t =>
+ sql(s"CREATE TABLE $t (i INT) $defaultUsing")
+ withSessionVariable("my_var") {
+ sql("DECLARE OR REPLACE VARIABLE my_var INT DEFAULT 1")
+ val error = intercept[AnalysisException] {
+ sql(s"ALTER TABLE $t ADD CONSTRAINT c1 CHECK (i > session.my_var)")
+ }
+ checkError(
+ exception = error,
+ condition = "INVALID_TEMP_OBJ_REFERENCE",
+ sqlState = "42K0F",
+ parameters = Map(
+ "obj" -> "CHECK CONSTRAINT",
+ "objName" -> "`c1`",
+ "tempObj" -> "VARIABLE",
+ "tempObjName" -> "`session`.`my_var`")
+ )
+ }
+ }
+ }
+
+ test("SPARK-57379: Temporary variable in check constraint -- happy path without variable") {
+ withNamespaceAndTable("ns", "tbl", nonPartitionCatalog) { t =>
+ sql(s"CREATE TABLE $t (i INT, CONSTRAINT c1 CHECK (i > 0)) $defaultUsing")
+ sql(s"ALTER TABLE $t ADD CONSTRAINT c2 CHECK (i > -1)")
+ }
+ }
+
+ // CTAS/RTAS cannot carry CHECK constraints -- the parser rejects them -- so no coverage needed.
Review Comment:
remove the comment
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57379][SQL] Block temporary variables in CHECK constraint expressions [spark]
Posted by "yadavay-amzn (via GitHub)" <gi...@apache.org>.
yadavay-amzn commented on code in PR #56472:
URL: https://github.com/apache/spark/pull/56472#discussion_r3503161842
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala:
##########
@@ -947,6 +947,21 @@ trait CheckAnalysis extends LookupCatalog with QueryErrorsBase with PlanToString
messageParameters = Map("checkCondition" -> a.checkConstraint.condition)
)
+ // Reject session/temp variables in persisted CHECK constraints.
+ case a: AddCheckConstraint =>
+ a.checkConstraint.child.collectFirst {
+ case v: VariableReference => v
+ }.foreach { v =>
+ v.failAnalysis(
Review Comment:
Extracted CheckConstraint.rejectTempVariables in the companion object - both call sites (ResolveTableSpec and CheckAnalysis) now delegate to it, so the detection logic and the error parameters live in one place.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57379][SQL] Block temporary variables in CHECK constraint expressions [spark]
Posted by "yadavay-amzn (via GitHub)" <gi...@apache.org>.
yadavay-amzn commented on PR #56472:
URL: https://github.com/apache/spark/pull/56472#issuecomment-4702943923
cc @szehon-ho (filed the JIRA) -- mirrors the structural placement of your SPARK-57360 fix, but uses `INVALID_TEMP_OBJ_REFERENCE` instead of `UNSUPPORTED_EXPRESSION_GENERATED_COLUMN` since the persistent-view-references-temp-variable error already covers this conceptual category. Would appreciate a look when convenient.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57379][SQL] Block temporary variables in CHECK constraint expressions [spark]
Posted by "yadavay-amzn (via GitHub)" <gi...@apache.org>.
yadavay-amzn commented on code in PR #56472:
URL: https://github.com/apache/spark/pull/56472#discussion_r3422859723
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/command/v2/CheckConstraintSuite.scala:
##########
@@ -78,6 +78,93 @@ class CheckConstraintSuite extends QueryTest with CommandSuiteBase with DDLComma
}
}
+ test("SPARK-57379: Temporary variable in check constraint -- alter table") {
+ withNamespaceAndTable("ns", "tbl", nonPartitionCatalog) { t =>
+ sql(s"CREATE TABLE $t (i INT) $defaultUsing")
+ withSessionVariable("my_var") {
+ sql("DECLARE OR REPLACE VARIABLE my_var INT DEFAULT 1")
+ Seq(
+ s"ALTER TABLE $t ADD CONSTRAINT c1 CHECK (i > my_var)",
+ s"ALTER TABLE $t ADD CONSTRAINT c1 CHECK (i > CAST(my_var AS BIGINT))"
+ ).foreach { query =>
+ val error = intercept[AnalysisException] {
+ sql(query)
+ }
+ checkError(
+ exception = error,
+ condition = "INVALID_TEMP_OBJ_REFERENCE",
+ sqlState = "42K0F",
+ parameters = Map(
+ "obj" -> "CHECK CONSTRAINT",
+ "objName" -> "`c1`",
+ "tempObj" -> "VARIABLE",
+ "tempObjName" -> "`my_var`")
+ )
+ }
+ }
+ }
+ }
+
+ test("SPARK-57379: Temporary variable in check constraint -- create table") {
+ withSessionVariable("my_var") {
+ sql("DECLARE OR REPLACE VARIABLE my_var INT DEFAULT 1")
+ Seq(
+ ("CREATE TABLE t(i INT CHECK (i > my_var))", "``"),
+ ("CREATE TABLE t(i INT, CONSTRAINT c1 CHECK (i > my_var))", "`c1`"),
+ ("REPLACE TABLE t(i INT CHECK (i > my_var))", "``"),
+ ("REPLACE TABLE t(i INT, CONSTRAINT c1 CHECK (i > my_var))", "`c1`"),
+ ("CREATE TABLE t(i INT CHECK (i > CAST(my_var AS BIGINT)))", "``")
+ ).foreach { case (query, expectedObjName) =>
+ withTable("t") {
+ val error = intercept[AnalysisException] {
+ sql(query)
+ }
+ checkError(
+ exception = error,
+ condition = "INVALID_TEMP_OBJ_REFERENCE",
+ sqlState = "42K0F",
+ parameters = Map(
+ "obj" -> "CHECK CONSTRAINT",
+ "objName" -> expectedObjName,
+ "tempObj" -> "VARIABLE",
+ "tempObjName" -> "`my_var`")
+ )
+ }
+ }
+ }
+ }
+
+ test("SPARK-57379: Temporary variable in check constraint -- qualified variable name") {
+ withNamespaceAndTable("ns", "tbl", nonPartitionCatalog) { t =>
+ sql(s"CREATE TABLE $t (i INT) $defaultUsing")
+ withSessionVariable("my_var") {
+ sql("DECLARE OR REPLACE VARIABLE my_var INT DEFAULT 1")
+ val error = intercept[AnalysisException] {
+ sql(s"ALTER TABLE $t ADD CONSTRAINT c1 CHECK (i > session.my_var)")
+ }
+ checkError(
+ exception = error,
+ condition = "INVALID_TEMP_OBJ_REFERENCE",
+ sqlState = "42K0F",
+ parameters = Map(
+ "obj" -> "CHECK CONSTRAINT",
+ "objName" -> "`c1`",
+ "tempObj" -> "VARIABLE",
+ "tempObjName" -> "`session`.`my_var`")
+ )
+ }
+ }
+ }
+
+ test("SPARK-57379: Temporary variable in check constraint -- happy path without variable") {
+ withNamespaceAndTable("ns", "tbl", nonPartitionCatalog) { t =>
+ sql(s"CREATE TABLE $t (i INT, CONSTRAINT c1 CHECK (i > 0)) $defaultUsing")
+ sql(s"ALTER TABLE $t ADD CONSTRAINT c2 CHECK (i > -1)")
+ }
+ }
+
+ // CTAS/RTAS cannot carry CHECK constraints -- the parser rejects them -- so no coverage needed.
Review Comment:
Done
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57379][SQL] Block temporary variables in CHECK constraint expressions [spark]
Posted by "yadavay-amzn (via GitHub)" <gi...@apache.org>.
yadavay-amzn commented on code in PR #56472:
URL: https://github.com/apache/spark/pull/56472#discussion_r3423152848
##########
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:
Kept `INVALID_TEMP_OBJ_REFERENCE` to match the persistent-view precedent and avoid growing the error class surface; happy to switch to a dedicated *_CHECK_CONSTRAINT class if you'd prefer.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org