Commit 8ee49ca
committed
[SPARK-59285][SQL] Hold a KeyedPartitioning's shared partition layout in one value
### What changes were proposed in this pull request?
`KeyedPartitioning` grows a `KeyLayout`, and everything its members share moves into it.
**One value for what a partitioning's members share.** `KeyLayout(partitionKeys, dataTypes, isGrouped, isCollapsed, mayContainUnknownPartitionKeys)` holds everything about the partitions a `KeyedPartitioning` describes except the expressions naming them, so `KeyedPartitioning` is `(expressions, layout)`. The members of a `PartitioningCollection` name one layout with their own expressions and share the object by reference, so:
- the collection's invariant is one `eq` on the layout in place of a clause per shared field, and it now covers `isGrouped`, which the field-by-field check left out;
- `fromPartitionings` merges one canonical layout instead of interning the keys and ORing two flags, and refuses a member that describes another key space, which interning would otherwise retype;
- `KeyedShuffleSpec.createPartitioning` has one thing to decide rather than three, the unknown-keys marker it must set;
- `GroupPartitionsExec`'s `PartitionGrouping` is the layout it will report plus the child partitions each of its own is built from.
**One derivation of a key schema's ordering.** `InternalRowComparableWrapper`'s `Factory` also answers with the `ordering` it holds, and `KeyedPartitioning.apply` takes a `sortKeys` flag that sorts with it. `DataSourceV2ScanExecBase` used to derive an ordering of its own to sort the splits and then hand the keys to `apply`, which derived the type list and the ordering again. It now passes `sortKeys = true`, so one factory answers for the types, the ordering and the wrappers. That is dongjoon-hyun's point on #58523 (#58523 (comment)), which needed this PR's shape to fix cleanly.
**One answer for the key types, including where no key row is left.** `keyDataTypes` reads the layout rather than sampling the first key row and falling back to the partition expressions. A layout is given the types its keys were built at, at the four places one is built. `EnsureRequirements` therefore drops the exception SPARK-59176 added for a side with no key row, since the layout answers for it.
### Why are the changes needed?
Two things, one structural and one a defect the structure hides.
**The shared part of a `KeyedPartitioning` is four fields that every member of a collection has to agree on by hand.** `checkKeyedPartitioningInvariant` compares them clause by clause, and it left `isGrouped` out. Four places put a partitioning's expressions over keys they did not build, and each has to carry the shared fields forward correctly:
1. `GroupPartitionsExec.outputPartitioning` reports the keys `EnsureRequirements` merged, and picks its member with `collectFirst`, which need not be the member the planner merged from.
2. `PartitioningPreservingUnaryExecNode.projectKeyedPartitionings` projects `kps.head` once and stamps every alias alternative onto it with `copy(expressions = ...)`.
3. `KeyedShuffleSpec.createPartitioning` puts the other child's expressions over these keys.
4. `KeyedPartitioning.concat`, for a `UnionExec`.
Sharing one layout by reference is what makes all four correct rather than merely lucky, and it turns the invariant into one `eq`. It also makes the field count stop mattering: SPARK-59050's `mayContainUnknownPartitionKeys` is the fifth shared field, and with the layout it is one more `copy` argument rather than one more clause in the invariant, one more OR in `fromPartitionings` and one more argument at every copy site.
**A side with no key row answers from its partition expressions, and after a both-sides reduce that is a type no key of it holds.** The reduce leaves keys that are `r1(f1(x))` = `r2(f2(x))`, a space neither transform names, so the reported expression is marked and its own type is the un-reduced one. SPARK-59176 worked around it by leaving such a side out of the co-partition type check, which left the check not checking for the shape most likely to need it, and every other reader of `keyDataTypes` still getting the wrong answer. The layout now carries what the reduce produced, so the workaround goes.
The types are on the layout rather than derived, because a partitioning whose partitions were all pruned has no row to read them off and its expressions do not describe a reduced key space. They are on the *layout* rather than on `KeyedPartitioning`, because that is what makes them shared: two members that share a layout share its keys, so the collection's `eq` covers them, and no consumer can pair one member's rows with another member's types.
Two alternatives were tried and dropped. An independent `keyDataTypes` field on `KeyedPartitioning` has to be decided at each of the four sites above, and two of them mix members, which is how it produced two reachable regressions in review. A `TypedKeys(dataTypes, keys)` value object does not settle it either, since two members can still hold two different pairs.
Sharing by reference is a constraint as well as a convenience, and one site had to change for it. `ShuffledJoin`'s marker clearing used to rewrite each member on its own; with the layout that would give each a different one, so it now builds one cleared layout for the whole input.
The scan sorts through `apply` rather than sorting before it, because the ordering it wants is the one its keys are compared at. Deriving it separately worked only because a generated comparator is name-blind, so the raw and the erased type lists happen to give the same order. Taking it off the factory makes that an identity rather than a coincidence.
No plan string changes. `KeyedPartitioning.stringArgs` prints the layout's contents where the value object would print, and deliberately leaves the key types out of that list: they have their field names, nullability and metadata erased (SPARK-59187), so printing them would put a struct field named `0` into a plan that appears nowhere in the query.
### Does this PR introduce _any_ user-facing change?
No. It adds no behaviour of its own beyond making a pruned side report its own key types truthfully.
### How was this patch tested?
Four new tests, plus SPARK-59176's two existing ones, which now pass with its exception removed.
Ablation: with the exception removed and `keyDataTypes` derived from the rows and expressions again, "SPARK-59176: a leg reduced onto no key at all still joins" fails with the error SPARK-59176 was filed for.
- `DistributionSuite`, "fromPartitionings refuses a member that disagrees on isGrouped", for the layout itself.
- `GroupPartitionsExecSuite`, "a reduced key space's type reaches the reported partitioning with no key left": a both-sides reduce onto `LongType` under a `DateType` transform, with keys and without.
- `KeyGroupedPartitioningSuite`, "two sides whose partitions were all pruned are not one layout": two legs pruned to nothing, one `identity(id)` on `LongType` and one `bucket(4, id)` on `IntegerType`, joined and then joined again through a FULL OUTER that brings real keys in. It asserts that no node reports two key spaces as one layout, that the plan passes `ValidateRequirements`, and the answer.
- `KeyGroupedPartitioningSuite`, "two legs whose struct field names differ are still co-partitioned", the shape where two exact type lists differ while the space does not.
SPARK-59050's two collection tests now assert on the layout reference rather than on a marker clause of their own, which is the same guarantee reached by the structure instead of by a check.
`KeyGroupedPartitioningSuite`, `KeyGroupedPartitioningRuntimeFilterSuite`, `GroupPartitionsExecSuite`, `EnsureRequirementsSuite`, `ProjectedOrderingAndPartitioningSuite`, `DataSourceV2CatalystRuntimeFilterSuite`, `PlannerSuite`, `DataFrameSetOperationsSuite`, `DistributionSuite`, `ShuffleSpecSuite`, `TransformExpressionSuite` and `InternalRowComparableWrapperSuite`, `DataSourceV2Suite`, 561 tests. Scalastyle and scalafmt clean.
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Opus 5)1 parent b22b611 commit 8ee49ca
14 files changed
Lines changed: 454 additions & 252 deletions
File tree
- sql
- catalyst/src
- main/scala/org/apache/spark/sql/catalyst/plans/physical
- test/scala/org/apache/spark/sql/catalyst
- core/src
- main/scala/org/apache/spark/sql/execution
- datasources/v2
- exchange
- joins
- test/scala/org/apache/spark/sql
- connector
- execution
- datasources/v2
- exchange
Lines changed: 197 additions & 130 deletions
Large diffs are not rendered by default.
Lines changed: 37 additions & 19 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
446 | 446 | | |
447 | 447 | | |
448 | 448 | | |
449 | | - | |
| 449 | + | |
450 | 450 | | |
451 | 451 | | |
452 | 452 | | |
| |||
457 | 457 | | |
458 | 458 | | |
459 | 459 | | |
460 | | - | |
| 460 | + | |
461 | 461 | | |
462 | | - | |
| 462 | + | |
463 | 463 | | |
464 | 464 | | |
465 | 465 | | |
| |||
470 | 470 | | |
471 | 471 | | |
472 | 472 | | |
473 | | - | |
| 473 | + | |
474 | 474 | | |
475 | 475 | | |
476 | 476 | | |
| |||
485 | 485 | | |
486 | 486 | | |
487 | 487 | | |
488 | | - | |
489 | | - | |
490 | | - | |
491 | | - | |
| 488 | + | |
| 489 | + | |
| 490 | + | |
| 491 | + | |
| 492 | + | |
492 | 493 | | |
493 | 494 | | |
494 | 495 | | |
495 | | - | |
| 496 | + | |
496 | 497 | | |
497 | 498 | | |
498 | 499 | | |
499 | 500 | | |
500 | 501 | | |
501 | 502 | | |
502 | | - | |
| 503 | + | |
| 504 | + | |
503 | 505 | | |
504 | 506 | | |
505 | 507 | | |
| |||
509 | 511 | | |
510 | 512 | | |
511 | 513 | | |
512 | | - | |
513 | | - | |
| 514 | + | |
| 515 | + | |
514 | 516 | | |
515 | 517 | | |
516 | 518 | | |
| |||
520 | 522 | | |
521 | 523 | | |
522 | 524 | | |
523 | | - | |
| 525 | + | |
524 | 526 | | |
525 | | - | |
526 | | - | |
527 | | - | |
| 527 | + | |
| 528 | + | |
| 529 | + | |
528 | 530 | | |
529 | 531 | | |
530 | 532 | | |
531 | | - | |
| 533 | + | |
532 | 534 | | |
533 | 535 | | |
534 | 536 | | |
| |||
542 | 544 | | |
543 | 545 | | |
544 | 546 | | |
545 | | - | |
| 547 | + | |
546 | 548 | | |
547 | 549 | | |
548 | 550 | | |
| |||
551 | 553 | | |
552 | 554 | | |
553 | 555 | | |
| 556 | + | |
| 557 | + | |
| 558 | + | |
| 559 | + | |
| 560 | + | |
| 561 | + | |
| 562 | + | |
| 563 | + | |
| 564 | + | |
| 565 | + | |
| 566 | + | |
| 567 | + | |
| 568 | + | |
| 569 | + | |
| 570 | + | |
| 571 | + | |
554 | 572 | | |
555 | 573 | | |
556 | 574 | | |
557 | 575 | | |
558 | 576 | | |
559 | | - | |
| 577 | + | |
560 | 578 | | |
561 | 579 | | |
562 | 580 | | |
| |||
Lines changed: 5 additions & 5 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
547 | 547 | | |
548 | 548 | | |
549 | 549 | | |
550 | | - | |
| 550 | + | |
551 | 551 | | |
552 | 552 | | |
553 | 553 | | |
| |||
602 | 602 | | |
603 | 603 | | |
604 | 604 | | |
605 | | - | |
| 605 | + | |
606 | 606 | | |
607 | 607 | | |
608 | 608 | | |
609 | 609 | | |
610 | | - | |
| 610 | + | |
611 | 611 | | |
612 | 612 | | |
613 | 613 | | |
| |||
636 | 636 | | |
637 | 637 | | |
638 | 638 | | |
639 | | - | |
| 639 | + | |
640 | 640 | | |
641 | 641 | | |
642 | 642 | | |
| |||
682 | 682 | | |
683 | 683 | | |
684 | 684 | | |
685 | | - | |
| 685 | + | |
686 | 686 | | |
687 | 687 | | |
688 | 688 | | |
| |||
Lines changed: 10 additions & 14 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
98 | 98 | | |
99 | 99 | | |
100 | 100 | | |
101 | | - | |
102 | | - | |
| 101 | + | |
| 102 | + | |
103 | 103 | | |
104 | 104 | | |
105 | 105 | | |
106 | 106 | | |
107 | 107 | | |
108 | | - | |
109 | | - | |
| 108 | + | |
| 109 | + | |
110 | 110 | | |
111 | 111 | | |
112 | 112 | | |
| |||
136 | 136 | | |
137 | 137 | | |
138 | 138 | | |
139 | | - | |
140 | | - | |
141 | | - | |
142 | | - | |
143 | 139 | | |
144 | | - | |
145 | | - | |
| 140 | + | |
| 141 | + | |
146 | 142 | | |
147 | 143 | | |
148 | 144 | | |
149 | | - | |
150 | | - | |
151 | | - | |
152 | | - | |
| 145 | + | |
| 146 | + | |
| 147 | + | |
| 148 | + | |
153 | 149 | | |
154 | 150 | | |
155 | 151 | | |
| |||
Lines changed: 9 additions & 4 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
952 | 952 | | |
953 | 953 | | |
954 | 954 | | |
955 | | - | |
956 | | - | |
957 | | - | |
958 | | - | |
| 955 | + | |
| 956 | + | |
| 957 | + | |
| 958 | + | |
| 959 | + | |
| 960 | + | |
| 961 | + | |
| 962 | + | |
959 | 963 | | |
960 | 964 | | |
| 965 | + | |
961 | 966 | | |
962 | 967 | | |
963 | 968 | | |
| |||
Lines changed: 8 additions & 6 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
19 | 19 | | |
20 | 20 | | |
21 | 21 | | |
22 | | - | |
| 22 | + | |
23 | 23 | | |
24 | 24 | | |
25 | 25 | | |
| |||
104 | 104 | | |
105 | 105 | | |
106 | 106 | | |
107 | | - | |
108 | | - | |
109 | | - | |
110 | | - | |
111 | | - | |
| 107 | + | |
| 108 | + | |
| 109 | + | |
| 110 | + | |
| 111 | + | |
| 112 | + | |
| 113 | + | |
112 | 114 | | |
113 | 115 | | |
114 | 116 | | |
| |||
0 commit comments