Skip to content

Commit ce977d2

Browse files
committed
[SPARK-56395][SQL][FOLLOWUP] Generalize nullability test to cover the nondeterministic ranking path
Generalize the LEFT OUTER nullability regression test in RewriteNearestByJoinSuite from an Aggregate-only assertion to a whole-plan integrity walk, and add a nondeterministic-ranking case alongside the deterministic one across both Inner and LeftOuter. The widened ranking reference lands in the Aggregate on the deterministic path but in a `__ranking__` Project (above the Join) on the nondeterministic path. The old assertion only inspected Aggregate expressions, so a regression isolated to the nondeterministic branch would not have been caught. No framework check guards this class of bug either -- LogicalPlanIntegrity compares types `asNullable` and schemas via `equalsIgnoreNullability` -- so this test is the only guard. Verified the new coverage has teeth: reverting just the nondeterministic alias to the un-widened expression fails the test specifically on LeftOuter/nondeterministic while the deterministic case still passes. Co-authored-by: Isaac
1 parent dd79e07 commit ce977d2

1 file changed

Lines changed: 33 additions & 18 deletions

File tree

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/RewriteNearestByJoinSuite.scala

Lines changed: 33 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -156,27 +156,35 @@ class RewriteNearestByJoinSuite extends PlanTest {
156156
}
157157

158158
test("SPARK-56395: LEFT OUTER rewrite keeps right-side nullability consistent with its child") {
159-
// A LEFT OUTER NEAREST BY widens the synthetic join's right-side columns to nullable. The
160-
// Aggregate built on top of that join references those columns (in the `_matches` struct and
161-
// the ranking), so each reference must carry the widened nullability -- otherwise the
162-
// rewritten plan declares a column non-nullable while its child produces it as nullable, an
163-
// internal inconsistency. INNER does not widen the right side, so it stays a no-op.
159+
// A LEFT OUTER NEAREST BY widens the synthetic join's right-side columns to nullable. Every
160+
// operator built on top of that join that references those columns (the `_matches` struct,
161+
// the ranking) must carry the widened nullability -- otherwise the rewritten plan declares a
162+
// column non-nullable while its child produces it as nullable, an internal inconsistency that
163+
// no framework check catches (`LogicalPlanIntegrity` compares types `asNullable` and schemas
164+
// `equalsIgnoreNullability`), so this assertion is the only guard. INNER does not widen the
165+
// right side, so it stays a no-op.
164166
//
165167
// The right-side columns are declared non-nullable here: that is what makes LEFT OUTER's
166168
// widening observable (with nullable columns the widening is a no-op and the bug is hidden).
169+
// The ranking is exercised both deterministic (the reference lands directly in the Aggregate)
170+
// and nondeterministic (the rule pre-materializes it into a `__ranking__` Project above the
171+
// Join), so the widening is checked wherever the reference ends up.
167172
val left = LocalRelation($"a".int, $"b".int)
168173
val right = LocalRelation(
169174
AttributeReference("x", IntegerType, nullable = false)(),
170175
AttributeReference("y", IntegerType, nullable = false)())
171-
Seq(Inner -> false, LeftOuter -> true).foreach { case (joinType, rightNullable) =>
176+
val rankings = Seq(
177+
"deterministic" -> (left.output(0) + right.output(0)),
178+
"nondeterministic" -> (Rand(Literal(0L)) + right.output(0)))
179+
for ((joinType, rightNullable) <- Seq(Inner -> false, LeftOuter -> true);
180+
(label, ranking) <- rankings) {
172181
val query = NearestByJoin(
173182
left, right, joinType, approx = true, numResults = 1,
174-
rankingExpression = left.output(0) + right.output(0),
183+
rankingExpression = ranking,
175184
direction = NearestBySimilarity)
176185

177186
val rewritten = RewriteNearestByJoin(query.analyze)
178187
val join = rewritten.collect { case j: Join => j }.head
179-
val aggregate = rewritten.collect { case a: Aggregate => a }.head
180188

181189
// Sanity-check the fixture: the synthetic join widens its right-side output to nullable
182190
// iff it is LEFT OUTER. (`join.right` is the right relation as it appears in the rewritten
@@ -186,16 +194,23 @@ class RewriteNearestByJoinSuite extends PlanTest {
186194
assert(joinRightOutput.nonEmpty)
187195
assert(joinRightOutput.forall(_.nullable == rightNullable))
188196

189-
// Every attribute the Aggregate references that its child (the join) produces must agree
190-
// on nullability with the child -- this is exactly what the fix corrects for LEFT OUTER.
191-
val childNullability = aggregate.child.output.map(a => a.exprId -> a.nullable).toMap
192-
aggregate.aggregateExpressions.foreach(_.foreach {
193-
case ref: AttributeReference if childNullability.contains(ref.exprId) =>
194-
assert(ref.nullable == childNullability(ref.exprId),
195-
s"$joinType: ${ref.name}#${ref.exprId.id} declared nullable=${ref.nullable} " +
196-
s"but its child produces nullable=${childNullability(ref.exprId)}")
197-
case _ =>
198-
})
197+
// Whole-plan integrity: at every operator, an attribute reference whose ExprId is produced
198+
// by one of that operator's children must agree with the child on nullability -- this is
199+
// exactly what the fix corrects for LEFT OUTER. Walking the whole plan (rather than just
200+
// the Aggregate) also covers the `__ranking__` Project that the nondeterministic path
201+
// inserts above the Join, where the widened ranking reference lands.
202+
rewritten.foreach { node =>
203+
val childNullability =
204+
node.children.flatMap(_.output).map(a => a.exprId -> a.nullable).toMap
205+
node.expressions.foreach(_.foreach {
206+
case ref: AttributeReference if childNullability.contains(ref.exprId) =>
207+
assert(ref.nullable == childNullability(ref.exprId),
208+
s"$joinType/$label: ${ref.name}#${ref.exprId.id} declared " +
209+
s"nullable=${ref.nullable} but its child produces " +
210+
s"nullable=${childNullability(ref.exprId)}")
211+
case _ =>
212+
})
213+
}
199214
}
200215
}
201216

0 commit comments

Comments
 (0)