peter-toth commented on code in PR #58979:
URL: https://github.com/apache/spark/pull/58979#discussion_r4143828989
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/planmerging/MergeSubplansSuite.scala:
##########
@@ -2927,9 +2927,10 @@ class MergeSubplansSuite extends PlanTest {
test("SPARK-40259: merge DSv2 scans that report empty key-grouped
partitioning or ordering") {
// A source implementing SupportsReportPartitioning/SupportsReportOrdering
but reporting nothing
- // yields Some(Nil), not None (V2ScanPartitioningAndOrdering sets the
field unconditionally). An
- // empty report carries no partitioning/ordering to drop, so the merge
should still proceed --
- // the gate tests the inner Seq, not the Option. The fused plan is the
plain column union.
+ // yields Some(Nil), not None (V2ScanPartitioningAndOrdering sets the
field unless a reported
+ // column cannot be resolved). An empty report carries no
partitioning/ordering to drop, so the
+ // merge should still proceed -- the gate tests the inner Seq, not the
Option. The fused plan is
+ // the plain column union.
Review Comment:
**Finding 3.** The rewritten parenthetical is accurate for the ordering pass
only. The partitioning pass never yields `Some(Nil)`:
- an `UnknownPartitioning` report gives `None`
(`V2ScanPartitioningAndOrdering.scala:78`);
- a `KeyGroupedPartitioning` with no keys gives `None` too, because
`Some(Nil).filter(_.exists(...))` is `None`
(`V2ScanPartitioningAndOrdering.scala:77`, whose own comment says "likewise
when the source reported no key at all").
That has been the case since SPARK-59248, so the `keyGroupedPartitioning =
Some(Nil)` half of this test is a state the rule does not produce. Something
like:
```suggestion
// A source implementing SupportsReportOrdering but reporting no
ordering yields Some(Nil), not
// None (V2ScanPartitioningAndOrdering sets the ordering unless a
reported column cannot be
// resolved). The partitioning pass turns an empty report into None
instead, so the rule never
// produces the keyGroupedPartitioning = Some(Nil) case below. An empty
report carries no
// partitioning/ordering to drop, so the merge should still proceed --
the gate tests the inner
// Seq, not the Option. The fused plan is the plain column union.
```
--
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]
