pratham76 commented on code in PR #58656:
URL: https://github.com/apache/spark/pull/58656#discussion_r4114427431
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/subquery.scala:
##########
@@ -158,20 +158,23 @@ object RewritePredicateSubquery extends Rule[LogicalPlan]
with PredicateHelper {
// Filter the plan by applying left semi and left anti joins.
withSubquery.foldLeft(newFilter) {
case (p, Exists(sub, _, _, conditions, subHint)) =>
- val (joinCond, outerPlan) = rewriteExistentialExpr(conditions, p)
- val join = buildJoin(outerPlan,
rewriteDomainJoinsIfPresent(outerPlan, sub, joinCond),
+ val (joinCond, outerPlan, newSub) =
+ rewriteExistentialExprInJoinCondition(conditions, p, sub)
Review Comment:
Confirmed. I reproduced it in a Catalyst test rather than through SQL, since
`DeduplicateRelations` removes the ExprId overlap from every analyzed query
I could write: with the
outer and the subquery plan sharing `a`, a nested subquery that references
only the inner query is
reported as `NESTED_SUBQUERY_REFERENCING_OUTER_AND_INNER_QUERY`.
I took the second half of your instruction — "or otherwise ensure routing
uses the deduplicated
subplan" — rather than moving the deduplication, for the reason in your own
Risks line. For the
EXISTS arms the two are not equivalent: `dedupSubqueryOnSelfJoin` aliases
the subquery plan's `a` to
a fresh ExprId while the hoisted `conditions` still say `a`, and since the
semi join's left side is
the outer plan, which still produces `a`, the condition would rebind to the
outer attribute. That
turns a correlated predicate into a self-reference silently, with no error,
which is worse than the
misclassification it would fix. The IN arms escape it only because they
rebuild `inConditions` from
`dedupSub.output`; the hoisted `conditions` they pass along are equally
exposed.
So the classification now behaves as though the subplan had been
deduplicated:
`effectivelyReferencesPlanOnly` attributes an attribute that both plans
produce to the subquery
plan, because a nested subquery resolves in the scope of the subquery it
sits in, and only an
attribute the outer plan alone produces counts as an outer reference. That
is the classification the
IN arms get from their deduplicated plan, with no change to deduplication or
to the errors it
raises.
Against your success criteria:
* *"A genuinely both-side correlated nested subquery is still rejected"* —
holds, and the
`SubquerySuite` case still fails if the rejection is removed.
* *"Routed to that plan despite pre-dedup ExprId overlap"* — holds for the
routing: the existence
join is built on the subquery plan and the nested subquery is no longer
misclassified. The query
still does not succeed, though. `buildJoin` hands the semi join condition
to
`dedupSubqueryOnSelfJoin`, which has always rejected a condition naming a
duplicated attribute, so
it ends at `_LEGACY_ERROR_TEMP_1212` — the pre-existing behaviour dongjoon
describes in the
dedup-anchor comment. The user-visible change is a better error, not a
working query.
Making that shape succeed is the single preparation step you describe, and
it needs the hoisted
condition rewritten along with the plan, which changes a pre-existing error
path for all four arms.
I have kept it out so this PR stays a fix for the attribute-routing bug.
Happy to do it here if you
want it in scope; otherwise I will file it with the repro from the new test.
The new test is in `RewriteSubquerySuite`, covering a self-correlated EXISTS
with a nested subquery
under ExprId overlap; it fails with the 0A000 misclassification if the
classification change is
reverted.
--
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]
