AveryQi115 commented on code in PR #57614:
URL: https://github.com/apache/spark/pull/57614#discussion_r3686184155
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/DecorrelateInnerQuery.scala:
##########
@@ -725,9 +725,21 @@ object DecorrelateInnerQuery extends PredicateHelper {
if (partitionFields.isEmpty) {
// Underlying subquery has no predicates connecting inner and
outer query.
// In this case, limit can be computed over the inner query
directly.
+ // The ORDER BY was peeled off the Sort above; re-apply it as a
global Sort below
+ // the limit so that ORDER BY ... LIMIT (and ORDER BY ... LIMIT
... OFFSET) is
+ // order-preserving. Otherwise the ordering is dropped and the
limit returns an
+ // arbitrary (non-deterministic) row.
+ val orderedChild =
+ if (ordering.nonEmpty && !SQLConf.get.getConf(
+
SQLConf.DECORRELATE_LIMIT_OFFSET_LEGACY_INCORRECT_ORDER_HANDLING_ENABLED)) {
+ Sort(replaceOuterReferences(ordering, outerReferenceMap),
global = true, newChild)
Review Comment:
Thanks for the suggestion. I understand your concern.
Cases as `ORDER BY t2c * t1b` which has outer references in a pure sort
operator is blocked by the anlayzer currently. Evidence:
```
ValidateSubqueryExpression.scala
- checkCorrelationsInSubquery.checkPlan, line 429 - 431:
case s: Sort =>
failOnInvalidOuterReference(s)
checkPlan(s.child, aggregated, canContainOuter)
- Sort is documented as Category 2: "allowed anywhere in a correlated
subquery so long as they do not host outer references" (lines 422 - 424).
- failOnInvalidOuterReference (line: 295) collects p.expressions containing
outer refs and, since canHostOuter returns true only for Filter/Project/Join
(lines 285 - 291) never Sort, throws
UNSUPPORTED_SUBQUERY_EXPRESSION_CATEGORY.CORRELATED_REFERENCE.
```
So such cases are never allowed. Attributes in the order by can only refer
to inner attributes or the generated domain attributes.
I understand that a more general solution is better. If we support order by
containing outer references in the future, the optimizer can correctly handle
it. But a more general solution requires larger changes since we never collect
outer references from sort operator before.
I can either:
1. keep this pr as surgical fix for the target situation. Add a guard to
make sure no outer references are within the order by expressions. Add a golden
test to make sure such cases are blocked by the analyzer. Raise a jira ticket
for supporting outer references in order by expressions in the future.
2. Use a more general solution to collect outer references from sort
operators too. Change our rewrite for both limit/offset + sort and offset +
sort and add more testcases. (add a flag to make such pattern supported in the
analyzer checks)
Do you have any insights/preference for these 2 solutions?
--
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]
