peter-toth commented on code in PR #59999:
URL: https://github.com/apache/spark/pull/59999#discussion_r4436667256
##########
common/utils/src/main/resources/error/error-conditions.json:
##########
@@ -8383,6 +8383,12 @@
],
"sqlState" : "22023"
},
+ "SUBQUERY_IN_CHECK_CONSTRAINT" : {
Review Comment:
**Finding 1.** This error rejects DDL that `master` accepts. I ran these on
the base commit with a V2 catalog, and each one succeeds and stores the
constraint:
- `CREATE TABLE t (id INT, CONSTRAINT c1 CHECK (id > (SELECT 0 AS a)))`
- `REPLACE TABLE t (id INT, CONSTRAINT c1 CHECK (id > (SELECT 0 AS a)))`
- `ALTER TABLE t ADD CONSTRAINT c1 CHECK (id > (SELECT 0 AS a))`
`CREATE TABLE` also accepts `CHECK (EXISTS (SELECT 1 AS a))`. SPARK-60777
added a migration guide entry for the same kind of change, so this one needs
one too. Something like this, after the SPARK-60777 entries:
```
- Since Spark 4.4, `CREATE TABLE`, `REPLACE TABLE`, and `ALTER TABLE ... ADD
CONSTRAINT` on a Data Source V2 table fail with `SUBQUERY_IN_CHECK_CONSTRAINT`
when the condition of a CHECK constraint contains a subquery. Previously, most
of these statements failed with `INTERNAL_ERROR`, but a subquery that needed no
resolution, such as `id > (SELECT 0 AS a)`, was accepted and every write
evaluated it again (SPARK-60781). Existing constraints that contain a subquery
are not rejected.
```
That entry would also replace the "Only `master` was run." sentence in the
description, which is hard to follow.
##########
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:
**Finding 3.** Every CHECK constraint is built by
`AstBuilder.visitCheckConstraint` (`AstBuilder.scala:7189`). That covers
table-level and column-level CHECK in `CREATE TABLE`, `REPLACE TABLE` and
`ALTER TABLE`. Finding a subquery needs no resolution, so the check could run
there:
```scala
override def visitCheckConstraint(ctx: CheckConstraintContext):
CheckConstraint =
withOrigin(ctx) {
val condition = getOriginalText(ctx.expr)
val check = CheckConstraint(
child = expression(ctx.booleanExpression()),
condition = condition)
check.failIfContainsSubquery()
check
}
```
I tried it with this case and the `CheckAnalysis` case removed.
`CheckConstraintSuite` passes, including both new tests with their query
context. What it changes:
- One call replaces both hooks. Neither this ordering comment nor the one
about the child `Filter` is needed.
- A subquery in a lambda gets the same error in every statement. Today
`CHECK (exists(array(id), v -> v > (SELECT 0)))` fails with
`SUBQUERY_IN_CHECK_CONSTRAINT` in `CREATE TABLE`, but with
`UNSUPPORTED_SUBQUERY_EXPRESSION_CATEGORY.HIGHER_ORDER_FUNCTION` in `ALTER
TABLE`.
The cost is a semantic check in the parser, which wins over every analysis
error. That includes a missing table (finding 2) and
`UNSUPPORTED_FEATURE.TABLE_OPERATION` for `ALTER TABLE` on a V1 table. There is
a precedent, though. `CheckConstraint.withUserProvidedCharacteristic` already
throws a `ParseException` from the parser for `NOT ENFORCED`.
##########
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:
**Finding 4.** Minor: this check sits after `toV2Constraint`, while its
sibling `failIfCallsUserDefinedFunction` sits above it. Moving it up next to
that one keeps the two `failIf*` checks together.
##########
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:
**Finding 2.** This traversal exists to report a missing INSERT target
before the errors in its query (the comment at `CheckAnalysis.scala:393`). For
`ALTER TABLE`, this case now does the opposite. I ran `ALTER TABLE
non_part_test_catalog.ns.missing ADD CONSTRAINT c CHECK (id > (SELECT 0))`:
- base: `TABLE_OR_VIEW_NOT_FOUND`
- this PR: `SUBQUERY_IN_CHECK_CONSTRAINT`
Matching on the relation under the `Filter` keeps the old order. With the
change below, the missing table reports `TABLE_OR_VIEW_NOT_FOUND` again, and
both new tests still pass. The `FROM missing` case of the test still gets the
new error, because that relation is inside the subquery, not under the `Filter`.
```suggestion
// 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. A
// missing table is still reported first, by the bottom-up traversal.
case AddCheckConstraint(Filter(_, relation), check) if
relation.resolved =>
check.failIfContainsSubquery()
```
A test case with a missing table would pin the order. It fails without the
guard. The sentence about a missing table in the description would go too.
--
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]
