pratham76 commented on PR #58656:
URL: https://github.com/apache/spark/pull/58656#issuecomment-5853675261
@cloud-fan @dongjoon-hyun @LuciferYang all review comments are addressed,
could
you take another look? Below are some of the points where i would require
your inputs to take a decision. PTAL. Thanks!
1. **@cloud-fan — the preparation step for self-join deduplication
(`r4068594134`).** Routing now
classifies as if the subplan had been deduplicated, which is the part I
could do without
rewriting the hoisted condition. The colliding EXISTS shape still ends at
the pre-existing
`_LEGACY_ERROR_TEMP_1212` from `buildJoin` rather than succeeding. Do you
want the full
preparation step — deduplication plus condition rewriting, across all
four arms — in this PR, or
as a follow-up?
2. **@dongjoon-hyun — phase 2 accepting on a negation (`r4065967251`).**
Your fail-safe positive
test and cloud-fan's `r4057052746` pull in opposite directions: requiring
a positive reason to
rewrite on the outer plan would re-reject the subquery whose retained
outer attributes no longer
correspond to a condition, which `r4057052746` asked to keep rewriting
there. I documented the
invariant instead. Which of the two would you rather have?
3. **@dongjoon-hyun — the reused-ExprId concern (`r4065967215` (a)).** I
could not build a query
that reaches the optimizer with the conflict intact; every shape I tried,
including your
`c1 IN (SELECT a FROM t1)`, comes out of the analyzer deduplicated, and
the conflict check in
`buildJoin` rejects the rest first. If you have a query that gets there,
I will add it as a test
and fix what it shows.
4. **@dongjoon-hyun — the asymmetry inside `dedupSubqueryOnSelfJoin`** (able
to throw when
`buildJoin` passes the condition, only aliasing when the IN arms call it
without one). Left as is,
being pre-existing and the same work as question 1. Address it here, or
separately?
5. **Everyone — the two new error condition names and their wording.** They
are my choice; happy to
rename or merge them into one sub-condition if you prefer.
6. **@cloud-fan — the follow-up you asked me to track.** Replacing a
three-valued `IN` with a
non-nullable existence bit is already wrong on released 4.0.1 on the
outer and projection routes,
with no correlation involved:
`SELECT a, ((a IN (SELECT col1 FROM t3)) <=> false) FROM t1` returns
`true, true, false` where
PostgreSQL and DuckDB return `false, false, false`. Shall I file that as
a separate JIRA with
these repros and the reconstruction sketch — two existence joins, the
second on
`cond OR isnull(cond)` so multi-column row comparison stays correct, plus
a copy of the subquery
plan with fresh expression ids?
--
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]
