dongjoon-hyun commented on code in PR #59999:
URL: https://github.com/apache/spark/pull/59999#discussion_r4436785183
##########
common/utils/src/main/resources/error/error-conditions.json:
##########
@@ -8383,6 +8383,12 @@
],
"sqlState" : "22023"
},
+ "SUBQUERY_IN_CHECK_CONSTRAINT" : {
Review Comment:
Added the entry after the SPARK-60777 ones, and removed that sentence from
the description. I kept your wording and added one sentence for a subquery in a
lambda function, which keeps its error in `ALTER TABLE`.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala:
##########
@@ -417,6 +417,11 @@ trait CheckAnalysis extends LookupCatalog with
QueryErrorsBase with PlanToString
// Here we perform additional checks for them.
checkIfSelfReferenceIsPlacedCorrectly(unionLoop, unionLoop.id)
+ // The child `Filter` holds the same condition. Check it first so that
an error of the
+ // subquery in the `Filter` does not hide that the constraint cannot
have a subquery.
+ case a: AddCheckConstraint =>
+ a.checkConstraint.failIfContainsSubquery()
Review Comment:
Applied the guard and added the test case. `ALTER TABLE` on a missing table
reports `TABLE_OR_VIEW_NOT_FOUND` again, and the test fails without the guard.
`REPLACE TABLE` on a missing table still gets the new error first, because that
check runs when the command is executed.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveTableSpec.scala:
##########
@@ -61,6 +61,14 @@ object ResolveTableSpec extends Rule[LogicalPlan] {
input: LogicalPlan,
tableSpec: TableSpecBase,
withNewSpec: TableSpecBase => LogicalPlan): LogicalPlan = tableSpec
match {
+ // Check this before `childrenResolved`: `ResolveSubquery` skips these
commands, so the subquery
+ // may never resolve, and `toV2Constraint` below would drop one that needs
no resolution.
+ case u: UnresolvedTableSpec if
u.constraints.exists(SubqueryExpression.hasSubquery) =>
Review Comment:
I ran this too, and `CheckConstraintSuite` passes with it, but I kept the
checks in the analyzer. In the parser, the new error would also win over a
missing table, so finding 2 could not be fixed, and over
`UNSUPPORTED_FEATURE.TABLE_OPERATION` for a V1 table and the existing error for
constraints on a pipeline dataset. `EXPLAIN ALTER TABLE` would also start to
throw a `ParseException`. The other checks of the condition, such as
`NON_DETERMINISTIC_CHECK_CONSTRAINT` and
`USER_DEFINED_FUNCTION_IN_CHECK_CONSTRAINT`, are in the analyzer too. The
lambda case stays as it is, and the description and the migration guide now
mention it.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/constraints.scala:
##########
@@ -167,6 +167,15 @@ case class CheckConstraint(
.build()
}
+ /** Fails the analysis if the condition contains a subquery, which `Check`
does not allow. */
+ def failIfContainsSubquery(): Unit = {
Review Comment:
Moved it next to `failIfCallsUserDefinedFunction`.
--
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]
