Skip to content

Commit 00d3243

Browse files
committed
[SPARK-59050][4.1][SQL] SPJ one-side shuffle with out-of-set keys produces wrong results in multi-joins
### What changes were proposed in this pull request? Backport of #58339 to `branch-4.1`, reduced to what this branch's SPJ machinery has. The core fix is unchanged: `KeyGroupedPartitioning` gains `mayContainUnknownPartitionKeys`, set by `KeyGroupedShuffleSpec.createPartitioning` (the one-side shuffle), and `areKeysCompatible` only co-partitions such a side with a subset-keyed (or identically-declared and identically-marked) partner; `satisfies0` gives it the full-key rules only (no subset clustering, no global ordering beyond a single partition); `createShuffleSpec` refuses a narrowing projection of a marked partitioning. There is no `GroupPartitionsExec` on this branch, and neither the union nor the generic projection transforms a `KeyGroupedPartitioning`, so the marker needs no propagation beyond the partitioning itself and no clearing at `ShuffledJoin`: consumers of a joined collection read it with `exists`, so a spurious marked member cannot cost a sound storage-partitioned join. This branch also has no alignment for differing partition counts between two keyed sides, so a second join whose sides differ in `numPartitions` one-side-shuffles the join output onto the larger layout, with or without the marker; the three `master` tests that expect a direct storage-partitioned join there pin this branch's two-exchange shape instead. None of them regressed relative to the pre-fix plan. The commit message carries the full change description with the complete backport notes. ### Why are the changes needed? A storage-partitioned join whose preserved side is one-side-shuffled (`spark.sql.sources.v2.bucketing.shuffle.enabled`, shipping since 4.0.0) can silently lose matches in a following storage-partitioned join, with no error. Reproduction in #58339. ### Does this PR introduce _any_ user-facing change? Yes: it fixes a wrong-results bug in `spark.sql.sources.v2.bucketing.shuffle.enabled`. Storage-partitioned joins whose preserved side is one-side-shuffled against a smaller keyed side, followed by another storage-partitioned join on a larger key set, now fall back to shuffles and return correct results. ### How was this patch tested? The ported suites pass on `branch-4.1`: `KeyGroupedPartitioningSuite` (12 `SPARK-59050` tests), `ShuffleSpecSuite` (3 new tests), `DistributionSuite`, `EnsureRequirementsSuite`, `ValidateRequirementsSuite`. ### Was this patch authored or co-authored using generative AI tooling? Yes. Generated-by: Claude Code. Closes #58571 from ulysses-you/spj-part-mismatch-4.1. Authored-by: Xiduo You <ulyssesyou18@gmail.com> Signed-off-by: Xiduo You <ulyssesyou@apache.org>
1 parent 073e269 commit 00d3243

8 files changed

Lines changed: 800 additions & 21 deletions

File tree

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/physical/partitioning.scala

Lines changed: 99 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -374,13 +374,34 @@ case class CoalescedHashPartitioning(from: HashPartitioning, partitions: Seq[Coa
374374
* @param originalPartitionValues the original input partition values before any grouping has been
375375
* applied, must be in ascending order, and may contain duplicated
376376
* values
377+
* @param mayContainUnknownPartitionKeys Whether the data may contain rows whose partition key is
378+
* not among the declared `partitionValues`. `KeyGroupedPartitioner`
379+
* routes such rows by a deterministic hash when a side is
380+
* re-shuffled onto this partitioning (see
381+
* `KeyGroupedShuffleSpec.createPartitioning`), so co-location holds
382+
* for whole keys only: two marked partitionings declaring the
383+
* same keys in the same order and using the same partition
384+
* function per position still pair (equal undeclared keys hash
385+
* to the same partition), but a row of an undeclared key sits in
386+
* the partition of some other declared key and need not be
387+
* co-located with rows sharing only a subset of its columns.
388+
* `satisfies` and `KeyGroupedShuffleSpec.areKeysCompatible`
389+
* therefore accept a marked partitioning only for full-key
390+
* clustering, never for a subset of its partition columns and
391+
* never for a global ordering across several partitions. A node
392+
* that changes the declared key set must drop the keyed
393+
* partitioning, whether it coarsens it (a key-dropping
394+
* projection, a join-key projection) or expands it over a marked
395+
* leg (a union, where another leg may declare exactly the key
396+
* that leg holds out-of-set).
377397
*/
378398
case class KeyGroupedPartitioning(
379399
expressions: Seq[Expression],
380400
numPartitions: Int,
381401
partitionValues: Seq[InternalRow] = Seq.empty,
382402
originalPartitionValues: Seq[InternalRow] = Seq.empty,
383-
isPartiallyClustered: Boolean = false) extends HashPartitioningLike {
403+
isPartiallyClustered: Boolean = false,
404+
mayContainUnknownPartitionKeys: Boolean = false) extends HashPartitioningLike {
384405

385406
// See SPARK-55848. We must check ClusteredDistribution BEFORE delegating to
386407
// super.satisfies0(), because HashPartitioningLike.satisfies0() also matches
@@ -399,7 +420,11 @@ case class KeyGroupedPartitioning(
399420
// We'll need to find leaf attributes from the partition expressions first.
400421
val attributes = expressions.flatMap(_.collectLeaves())
401422

402-
if (SQLConf.get.v2BucketingAllowJoinKeysSubsetOfPartitionKeys) {
423+
if (mayContainUnknownPartitionKeys) {
424+
// Whole keys co-locate, subsets do not (see the `@param`): a window or aggregate
425+
// keyed on a strict subset of the partition columns must still shuffle.
426+
attributes.forall(x => requiredClustering.exists(_.semanticEquals(x)))
427+
} else if (SQLConf.get.v2BucketingAllowJoinKeysSubsetOfPartitionKeys) {
403428
// check that join keys (required clustering keys)
404429
// overlap with partition keys (KeyGroupedPartitioning attributes)
405430
requiredClustering.exists(x => attributes.exists(_.semanticEquals(x))) &&
@@ -410,7 +435,10 @@ case class KeyGroupedPartitioning(
410435
}
411436

412437
case o @ OrderedDistribution(_) if SQLConf.get.v2BucketingAllowSorting =>
413-
o.areAllClusterKeysMatched(expressions)
438+
// An out-of-set key can break the ascending sequence of the declared keys, so a marked
439+
// layout keeps a global ordering claim only for a single partition (see the `@param`).
440+
o.areAllClusterKeysMatched(expressions) &&
441+
(!mayContainUnknownPartitionKeys || numPartitions == 1)
414442

415443
case _ =>
416444
super.satisfies0(required)
@@ -420,12 +448,22 @@ case class KeyGroupedPartitioning(
420448
override def createShuffleSpec(distribution: ClusteredDistribution): ShuffleSpec = {
421449
val result = KeyGroupedShuffleSpec(this, distribution)
422450
if (SQLConf.get.v2BucketingAllowJoinKeysSubsetOfPartitionKeys) {
451+
val joinKeyPositions = result.keyPositions.map(_.nonEmpty).zipWithIndex.filter(_._1).map(_._2)
452+
// The projection coarsens the declared set, which a marked claim cannot survive (see the
453+
// `@param`). Return the unprojected spec: its extra expressions map to no clustering key,
454+
// so `areKeysCompatible` refuses it as a partner and `canCreatePartitioning` refuses it as
455+
// a shuffle template, and the child falls back to the ordinary shuffle.
456+
if (mayContainUnknownPartitionKeys && joinKeyPositions.length < expressions.length) {
457+
return result
458+
}
423459
// If allowing join keys to be subset of clustering keys, we should create a new
424460
// `KeyGroupedPartitioning` here that is grouped on the join keys instead, and use that as
425-
// the returned shuffle spec.
426-
val joinKeyPositions = result.keyPositions.map(_.nonEmpty).zipWithIndex.filter(_._1).map(_._2)
461+
// the returned shuffle spec. The construction carries the unknown-keys marker across:
462+
// only an identity projection reaches here when it is set, the refusal above turns away
463+
// the narrowing one.
427464
val projectedPartitioning = KeyGroupedPartitioning(expressions, joinKeyPositions,
428-
partitionValues, originalPartitionValues, isPartiallyClustered)
465+
partitionValues, originalPartitionValues, isPartiallyClustered,
466+
mayContainUnknownPartitionKeys = mayContainUnknownPartitionKeys)
429467
result.copy(partitioning = projectedPartitioning, joinKeyPositions = Some(joinKeyPositions))
430468
} else {
431469
result
@@ -449,7 +487,8 @@ object KeyGroupedPartitioning {
449487
projectionPositions: Seq[Int],
450488
partitionValues: Seq[InternalRow],
451489
originalPartitionValues: Seq[InternalRow],
452-
isPartiallyClustered: Boolean): KeyGroupedPartitioning = {
490+
isPartiallyClustered: Boolean,
491+
mayContainUnknownPartitionKeys: Boolean): KeyGroupedPartitioning = {
453492
val projectedExpressions = projectionPositions.map(expressions(_))
454493
val projectedPartitionValues = partitionValues.map(project(expressions, projectionPositions, _))
455494
val projectedOriginalPartitionValues =
@@ -461,7 +500,8 @@ object KeyGroupedPartitioning {
461500
.map(_.row)
462501

463502
KeyGroupedPartitioning(projectedExpressions, finalPartitionValues.length,
464-
finalPartitionValues, projectedOriginalPartitionValues, isPartiallyClustered)
503+
finalPartitionValues, projectedOriginalPartitionValues, isPartiallyClustered,
504+
mayContainUnknownPartitionKeys)
465505
}
466506

467507
def project(
@@ -901,6 +941,46 @@ case class KeyGroupedShuffleSpec(
901941
}
902942
} && expressions.zip(otherExpressions).forall {
903943
case (l, r) => isExpressionCompatible(l, r)
944+
} && {
945+
// An unknown-keyed side co-locates only its declared keys, and the out-of-set routing is
946+
// a deterministic hash, so it can pair only with a side whose keys are a subset of those
947+
// declared keys (see `KeyGroupedPartitioning.mayContainUnknownPartitionKeys`).
948+
//
949+
// The key comparison below must also happen in a single domain: `isExpressionCompatible`
950+
// admits two different-but-compatible transforms when `v2BucketingAllowCompatibleTransforms`
951+
// is on, and in those cases the two sides' `partitionValues` hold the two transforms'
952+
// outputs, so the subset test would compare unrelated values. Require the partition
953+
// expressions to be the same function per position before comparing keys.
954+
//
955+
// Two unknown-keyed sides are compatible only when they agree on the declared keys *and*
956+
// their order: the out-of-set keys hash to the same-index partition on both sides, so a
957+
// differing declared order would push the out-of-set keys into different output partitions
958+
// and lose their matches.
959+
if (partitioning.mayContainUnknownPartitionKeys ||
960+
other.partitioning.mayContainUnknownPartitionKeys) {
961+
expressions.zip(otherExpressions).forall {
962+
case (_: AttributeReference, _: AttributeReference) => true
963+
case (l: TransformExpression, r: TransformExpression) => l.isSameFunction(r)
964+
case _ => false
965+
} && {
966+
def valuesOf(kgp: KeyGroupedPartitioning): Seq[InternalRowComparableWrapper] =
967+
kgp.partitionValues.map(InternalRowComparableWrapper(_, kgp.expressions))
968+
val values = valuesOf(partitioning)
969+
val otherValues = valuesOf(other.partitioning)
970+
if (partitioning.mayContainUnknownPartitionKeys &&
971+
other.partitioning.mayContainUnknownPartitionKeys) {
972+
values == otherValues
973+
} else if (partitioning.mayContainUnknownPartitionKeys) {
974+
val declared = values.toSet
975+
otherValues.forall(declared.contains)
976+
} else {
977+
val declared = otherValues.toSet
978+
values.forall(declared.contains)
979+
}
980+
}
981+
} else {
982+
true
983+
}
904984
}
905985
}
906986

@@ -947,6 +1027,10 @@ case class KeyGroupedShuffleSpec(
9471027
override def canCreatePartitioning: Boolean =
9481028
SQLConf.get.v2BucketingShuffleEnabled &&
9491029
!SQLConf.get.v2BucketingPartiallyClusteredDistributionEnabled &&
1030+
// Every partition expression must map to a clustering key, otherwise `createPartitioning`
1031+
// cannot rewrite it. This also keeps the unprojected spec returned by `createShuffleSpec`
1032+
// for a marked narrowing projection from being chosen as the best spec.
1033+
keyPositions.forall(_.nonEmpty) &&
9501034
partitioning.expressions.forall { e =>
9511035
e.isInstanceOf[AttributeReference] || e.isInstanceOf[TransformExpression]
9521036
}
@@ -960,9 +1044,15 @@ case class KeyGroupedShuffleSpec(
9601044
te.copy(children = te.children.map(_ => clustering(positionSet.head)))
9611045
case (_, positionSet) => clustering(positionSet.head)
9621046
}
1047+
// The child re-shuffled onto this layout may hold keys outside the declared set, so every
1048+
// partitioning produced here carries the marker (see the `@param`). Only the shuffle loop
1049+
// reaches this call, and it carries no same-domain subset proof, so marking is sound; it is
1050+
// conservative where the child's keys are in fact a known subset (identity key [1] inside
1051+
// declared [1, 2]), a precision this path does not attempt.
9631052
KeyGroupedPartitioning(newExpressions,
9641053
partitioning.numPartitions,
965-
partitioning.partitionValues)
1054+
partitioning.partitionValues,
1055+
mayContainUnknownPartitionKeys = true)
9661056
}
9671057
}
9681058

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/DistributionSuite.scala

Lines changed: 21 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20,11 +20,13 @@ package org.apache.spark.sql.catalyst
2020
import org.apache.spark.SparkFunSuite
2121
/* Implicit conversions */
2222
import org.apache.spark.sql.catalyst.dsl.expressions._
23-
import org.apache.spark.sql.catalyst.expressions.{CollationAwareMurmur3Hash, Expression, Literal, Pmod}
23+
import org.apache.spark.sql.catalyst.expressions.{Ascending, AttributeReference, CollationAwareMurmur3Hash, Expression, Literal, Pmod, SortOrder}
24+
import org.apache.spark.sql.catalyst.plans.SQLHelper
2425
import org.apache.spark.sql.catalyst.plans.physical._
26+
import org.apache.spark.sql.internal.SQLConf
2527
import org.apache.spark.sql.types.IntegerType
2628

27-
class DistributionSuite extends SparkFunSuite {
29+
class DistributionSuite extends SparkFunSuite with SQLHelper {
2830

2931
protected def checkSatisfied(
3032
inputPartitioning: Partitioning,
@@ -391,4 +393,21 @@ class DistributionSuite extends SparkFunSuite {
391393
StatefulOpClusteredDistribution(Seq($"a", $"b", $"c"), 10),
392394
false)
393395
}
396+
397+
test("SPARK-59050: a marked one-partition layout keeps the global ordering claim") {
398+
// The one-partition exemption inside the ordered branch of `satisfies0`: a single partition
399+
// holds every row, so an out-of-set key cannot break the cross-partition sequence, while
400+
// two partitions can (the e2e `ORDER BY` repro measures that). Positive control: an
401+
// always-false gate would shuffle these plans for nothing.
402+
val a = AttributeReference("a", IntegerType)()
403+
val ordered = OrderedDistribution(Seq(SortOrder(a, Ascending)))
404+
val markedOne = KeyGroupedPartitioning(Seq(a), 1, Seq(InternalRow(1)))
405+
.copy(mayContainUnknownPartitionKeys = true)
406+
val markedTwo = KeyGroupedPartitioning(Seq(a), 2, Seq(InternalRow(1), InternalRow(2)))
407+
.copy(mayContainUnknownPartitionKeys = true)
408+
withSQLConf(SQLConf.V2_BUCKETING_SORTING_ENABLED.key -> "true") {
409+
checkSatisfied(markedOne, ordered, true)
410+
checkSatisfied(markedTwo, ordered, false)
411+
}
412+
}
394413
}

0 commit comments

Comments
 (0)