pratham76 commented on code in PR #58656:
URL: https://github.com/apache/spark/pull/58656#discussion_r4072209586
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/subquery.scala:
##########
@@ -414,7 +433,8 @@ object RewritePredicateSubquery extends Rule[LogicalPlan]
with PredicateHelper {
existenceJoin, newCondition, subHint)
introducedAttrs += exists
exists
- case Not(InSubquery(values, ListQuery(sub, _, _, _, conditions,
subHint))) =>
+ case sq @ Not(InSubquery(values, ListQuery(sub, _, _, _, conditions,
subHint)))
+ if canRewrite(sq) =>
val exists = AttributeReference("exists", BooleanType, nullable =
false)()
// Deduplicate conflicting attributes if any.
val newSub = dedupSubqueryOnSelfJoin(newPlan, sub, Some(values))
Review Comment:
Confirmed, and the asymmetry is real. Building the state directly in a
Catalyst test -- the analyzer's `DeduplicateRelations` removes it from every
SQL query I could write, which is why no existing test covers it a nested
sub-query that references only the sub-query it is nested in is reported as
referencing both plans:
```
UNSUPPORTED_SUBQUERY_EXPRESSION_CATEGORY.NESTED_SUBQUERY_REFERENCING_OUTER_AND_INNER_QUERY
```
exactly as you predicted, because the single shared attribute satisfies both
sides of the classification.
I did not move `dedupSubqueryOnSelfJoin` ahead of the routing. Doing that
hits the risk in your own note -- "Moving deduplication without rewriting
correlated condition references can create ambiguous or stale join expressions"
-- and here it is not hypothetical. Deduplication aliases the sub-query plan's
`a` to a new ExprId, while the hoisted condition still says `a`. The semi
join's left side is the outer plan, which still produces `a`, so the condition
would silently bind to the outer attribute instead of the sub-query's: a
correlated predicate quietly turned into a self-reference, with no error. The
IN arms avoid this only because they rebuild `inConditions` from
`dedupSub.output`; the hoisted `conditions` they pass through have the same
exposure. So the classification is made collision-proof instead.
`effectivelyReferencesPlanOnly` attributes an attribute that *both* plans
produce to the sub-query plan, on the grounds that a nested sub-query resolves
in the scope of the sub-query it sits in, so a shared ExprId is that
sub-query's own. The routing then matches what the IN arms get from a
deduplicated plan, without touching deduplication or the error it raises.
A second finding while testing this: in that state the rewrite cannot
complete either way. `buildJoin` passes the semi join condition to
`dedupSubqueryOnSelfJoin`, which has always rejected a condition naming a
duplicated attribute, so the query ends at `_LEGACY_ERROR_TEMP_1212`
(`conflictingAttributesInJoinConditionError`) -- the pre-existing behaviour you
describe in the comment on the dedup anchor. A correlated EXISTS's hoisted
condition always names sub-plan attributes, so any shared ExprId the condition
mentions lands there. The routing fix therefore changes *which* error a user
sees, from a new 0A000 that misdescribes the query to the long-standing
conflict error that names the duplicated attribute, rather than making the
query work. The new test in `RewriteSubquerySuite` pins that: it asserts the
conflict error, and fails with
`NESTED_SUBQUERY_REFERENCING_OUTER_AND_INNER_QUERY` if the classification is
reverted. I have not tried to make that shape succeed. That needs dedupl
ication that rewrites the hoisted condition along with the plan, which is your
"single preparation step" and changes a pre-existing error path for all four
arms. Happy to take it on, here or separately, if you want it in scope.
--
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]
