xumingming commented on PR #58229:
URL: https://github.com/apache/spark/pull/58229#issuecomment-6061172491
Hi @peter-toth, thanks for both findings — both fixed on the branch.
**Finding 8:** Done as suggested — `ConvertViewToMaterializedCTE` now runs
first in the `FinishAnalysis` batch; your repro is added to the query suite
(plus a hinted variant, pinned on the plan), and the class doc is updated.
**Finding 9:** We replaced the check rather than adding the two cases, and
would like to discuss.
Before patching we swept other candidate wrappers (your repro, bodies over
`v1 = ... ORDER BY id DESC`) — with your two cases already applied, these
**still convert and change rows**:
```
SELECT DISTINCT id FROM v1 off:
52,53,61 on: 19,61,14
SELECT id FROM v1 TABLESAMPLE(50 PERCENT) off:
86,83,82 on: 84,85,82
WITH c AS (SELECT id FROM t ORDER BY id DESC) SELECT id FROM c off:
50,51,52 on: 19,61,14
(SELECT id FROM t ORDER BY id DESC) UNION ALL (...) off:
99,99,98 on: 90,90,86
SELECT v1.id FROM v1 JOIN t ON v1.id = t.id off:
50,51,52 on: 67,65,73
```
So the pass-through list is whack-a-mole (`Offset` was brand-new in 4.0).
Since the shuffle sits above the whole definition, we went to the semantic
criterion instead — reject conversion whenever any `Sort` is reachable through
operator `children` from the body root:
```scala
private def hasMainRegionSort(plan: LogicalPlan): Boolean =
plan.isInstanceOf[Sort] || plan.children.exists(hasMainRegionSort)
```
Subquery-expression sorts are not `children`, are skipped, and genuinely
can't affect the body's exposed rows (pinned by a test: `IN (SELECT ... ORDER
BY ...)` bodies still convert). All shapes above are covered by this one
criterion.
Cost: a few safe-by-luck shapes don't convert; benefit: the
silent-wrong-results class is closed for future operators, not just today's
list. Both suites green, your two bodies + the sweep shapes added as regression
tests.
--
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]
