HyukjinKwon commented on code in PR #59310:
URL: https://github.com/apache/spark/pull/59310#discussion_r4266401739
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala:
##########
@@ -644,6 +644,13 @@ trait CheckAnalysis extends LookupCatalog with
QueryErrorsBase with PlanToString
s"Cannot resolve the runtime replaceable expression
${toSQLExpr(e)}. " +
s"The replacement is unresolved: ${toSQLExpr(e.replacement)}.")
+ // Only the Spark Connect planner creates UnresolvedPlanId (a
DataFrame subquery by plan
+ // id), and only its WithRelations transform resolves it. A survivor
means the client
+ // never attached the referenced plan to the request, so report an
invalid Spark Connect
+ // plan rather than an INTERNAL_ERROR.
+ case u: UnresolvedPlanId =>
Review Comment:
**Non-blocking (P2):** This case treats every surviving `UnresolvedPlanId`
as a plan the client never attached. Some valid requests that do attach the
plan also end up here.
`SparkConnectPlanner.transformWithRelations` transforms the root relation
first and only afterwards substitutes the attached reference plans for the plan
ids. A few root transforms build eagerly analyzed classic Datasets over the
root's expressions before that substitution happens:
- `transformStatSampleBy` calls classic `sampleBy`, which runs
`df.filter(...)`.
- `transformAsOfJoin` calls classic `joinAsOf`, which analyzes the join in
`resolveSelfJoinCondition`.
The Python client does attach the subquery references for both.
`StatSampleBy` and `AsOfJoin` in `pyspark/sql/connect/plan.py` collect them
into `WithRelations`. For example,
`df.sampleBy(F.when(F.col("id").isin(sub_df), "a").otherwise("b"), {"a": 0.5})`
sends a valid request, but eager analysis still sees the unsubstituted plan id.
With this case, the user gets `CONNECT_INVALID_PLAN.SUBQUERY_PLAN_NOT_ATTACHED`
saying the plan is not attached to the request, which is not true. On master
the same request fails with `INTERNAL_ERROR`, which at least classifies a
server-side ordering problem correctly. The comment above makes the same "a
survivor means the client never attached the referenced plan" claim.
**Recommended change:** In SparkConnectPlanner.transformWithRelations,
collect the reference plan ids from the proto before transforming the root. If
transforming the root throws CONNECT_INVALID_PLAN.SUBQUERY_PLAN_NOT_ATTACHED
for a plan id that is among those references, rethrow it as an internal error
that says the attached subquery plan was analyzed before it could be resolved.
This keeps the merge-target INTERNAL_ERROR classification for that server-side
failure. Correct the CheckAnalysis comment so it no longer claims that a
survivor always means the client omitted the plan. Add Connect planner tests
for both outcomes.
**Why this works:** SUBQUERY_PLAN_NOT_ATTACHED escapes the planner only when
no enclosing WithRelations attached the plan id. When the id was attached, the
eager analysis failure is reported as a server-side internal error, so the new
condition's message is true whenever a client sees it.
**Scope:** Planner-side reclassification of eager-analysis failures for
attached plan ids, plus a CheckAnalysis comment correction and Connect planner
tests.
**Compatibility:** Valid Connect subquery plans and the existing
no-WithRelations classification are unchanged.
**Risks:** Matching on the error condition and its planId message parameter
couples the planner to the condition's parameter name; a rename must update
both.
**Constraints:** Keep CheckAnalysis as the detection point and keep
SUBQUERY_PLAN_NOT_ATTACHED, its SQLSTATE 56K00, its planId parameter, and the
expression origin unchanged for unattached ids. Do not change the existing
missing-relation assertion or the substitution logic for valid plans.
**Success:** A request whose subquery plan id is not attached by any
WithRelations still fails with CONNECT_INVALID_PLAN.SUBQUERY_PLAN_NOT_ATTACHED,
SQLSTATE 56K00, and the planId parameter. A request whose plan id is attached
in an enclosing WithRelations but analyzed eagerly during the root transform no
longer reports SUBQUERY_PLAN_NOT_ATTACHED; it fails with an internal error that
names the attached plan id and the early analysis. The CheckAnalysis comment no
longer states that every surviving UnresolvedPlanId means the client never
attached the plan.
**Verification:**
- **Regression:** A WithRelations request whose root is a stat sampleBy (or
as-of join) using an IN subquery whose plan id is among the attached references
does not fail with SUBQUERY_PLAN_NOT_ATTACHED and instead reports the internal
error naming that plan id.
- **Behavior:** An IN subquery plan id with no enclosing WithRelations still
fails analysis with SUBQUERY_PLAN_NOT_ATTACHED, SQLSTATE 56K00, and the planId
parameter.
- **Compatibility:** Valid WithRelations subquery plans whose references are
substituted normally keep producing the same analyzed results, and a
WithRelations missing a referenced id keeps its existing missing-relation
failure.
- **Inspection:** The CheckAnalysis comment describes a survivor as an
unattached plan in the normal case without claiming it is the only cause.
--
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]
