Skip to content

Commit 919f080

Browse files
committed
[SPARK-59434][SQL] Non-deterministic predicates should not be pushed down to CTE definitions
### What changes were proposed in this pull request? `PushdownPredicatesAndPruneColumnsForCTEDef` collects the predicates at each CTE reference and pushes their OR-merged combination into the shared CTE definition. Each reference keeps its own predicates, so this only works for deterministic predicates -- a non-deterministic one is then evaluated a second time in the definition. This PR filters the collected predicates to deterministic ones before push-down. When that leaves a reference with nothing pushable, the combined predicate becomes `TRUE` and the definition gets no push-down at all, including for its other references; the scaladoc is updated to say so. ### Why are the changes needed? A non-deterministic predicate evaluated a second time in the CTE definition can drop rows, which is a wrong-results bug: ```sql create or replace temp view t as select * from values (0), (1), (2) as t(c1); with v as (select c1, rand(1) r from t) select c1 from v where rand(2) < 0.5 union all select c1 from v where rand(3) < 0.5; ``` The definition is non-deterministic and referenced twice, so it survives `InlineCTE`. `(rand(2) < 0.5) OR (rand(3) < 0.5)` is pushed into the definition while both references keep their own filter, so the optimized plan holds four `rand` filters instead of two and rows are filtered twice. The rule has had this behavior since it was added in SPARK-37670 (3.2.2). Its scaladoc claimed determinism was taken care of by `ScanOperation`, but that was never true -- the filter-collection guard admits the first filter whatever it is: ```scala filters.isEmpty || (filters.forall(_.deterministic) && condition.deterministic) ``` `PhysicalOperation`, which SPARK-39764 (3.4.0) later swapped in, behaves the same way here -- it returns a single filter even when it is non-deterministic, since its `filters.length > 1` assert only binds when more than one filter was collected. The scaladoc is corrected along with the fix. ### Does this PR introduce _any_ user-facing change? Yes, it fixes wrong results for the query shape above. Affects 3.2.2 and later. ### How was this patch tested? Four new tests in `CTEInlineSuite`, all failing without the fix: - non-deterministic predicates stay at the references, two `rand` filters instead of four; - deterministic conjuncts are still pushed while the non-deterministic one stays up; - a reference with only non-deterministic predicates blocks push-down for its deterministic sibling, which must not be pushed alone; - a row-count assertion using `monotonically_increasing_id()`, 10 rows instead of 8. ### Was this patch authored or co-authored using generative AI tooling? Generated-by: Claude Opus 5 Closes #58735 from pan3793/cte-nondeterministic-pushdown. Authored-by: Cheng Pan <pan3793@gmail.com> Signed-off-by: Cheng Pan <chengpan@apache.org>
1 parent ddfcc17 commit 919f080

2 files changed

Lines changed: 116 additions & 6 deletions

File tree

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/PushdownPredicatesAndPruneColumnsForCTEDef.scala

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -57,10 +57,12 @@ object PushdownPredicatesAndPruneColumnsForCTEDef extends Rule[LogicalPlan] with
5757

5858
/**
5959
* Gather all the predicates and referenced attributes on different points of CTE references
60-
* using pattern `ScanOperation` (which takes care of determinism) and combine those predicates
61-
* and attributes that belong to the same CTE definition.
62-
* For the same CTE definition, if any of its references does not have predicates, the combined
63-
* predicate will be a TRUE literal, which means there will be no predicate push-down.
60+
* using pattern `PhysicalOperation` and combine those predicates and attributes that belong
61+
* to the same CTE definition. `PhysicalOperation` still returns a single filter even when it
62+
* is non-deterministic, so such predicates are excluded below.
63+
* For the same CTE definition, if any of its references does not have pushable predicates, the
64+
* combined predicate will be a TRUE literal, which means there will be no predicate push-down
65+
* for that definition at all, including for its other references.
6466
*/
6567
private def gatherPredicatesAndAttributes(plan: LogicalPlan, cteMap: CTEMap): Unit = {
6668
plan match {
@@ -77,8 +79,11 @@ object PushdownPredicatesAndPruneColumnsForCTEDef extends Rule[LogicalPlan] with
7779
val newPredicates = if (isTruePredicate(preds)) {
7880
preds
7981
} else {
80-
// Make sure we only push down predicates that do not contain forward CTE references.
81-
val filteredPredicates = restoreCTEDefAttrs(predicates.filter(_.find {
82+
// Only push down deterministic predicates that do not contain forward CTE references.
83+
// A reference keeps its own predicates, so a non-deterministic one pushed into the
84+
// shared definition as well would be evaluated a second time.
85+
val deterministicPredicates = predicates.filter(_.deterministic)
86+
val filteredPredicates = restoreCTEDefAttrs(deterministicPredicates.filter(_.find {
8287
case s: SubqueryExpression => s.plan.find {
8388
case r: CTERelationRef =>
8489
// If the ref's ID does not exist in the map or if ref's corresponding precedence

sql/core/src/test/scala/org/apache/spark/sql/CTEInlineSuite.scala

Lines changed: 105 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -451,6 +451,111 @@ abstract class CTEInlineSuiteBase
451451
}
452452
}
453453

454+
test("SPARK-59434: non-deterministic predicates are not pushed into a CTE def") {
455+
withTempView("t") {
456+
Seq(0, 1, 2).toDF("c1").createOrReplaceTempView("t")
457+
// The CTE def is non-deterministic and referenced twice, so it is not inlined and the
458+
// references' predicates get OR-merged into the shared def. A reference keeps its own
459+
// predicate, so a non-deterministic one must not be pushed down as well.
460+
val df = sql(
461+
"""with v as (select c1, rand(1) r from t)
462+
|select c1 from v where rand(2) < 0.5
463+
|union all
464+
|select c1 from v where rand(3) < 0.5
465+
|""".stripMargin)
466+
val cteRepartitions = df.queryExecution.optimizedPlan.collect {
467+
case r: RepartitionOperation => r
468+
}
469+
assert(cteRepartitions.nonEmpty,
470+
"Non-deterministic With-CTE with multiple references should not be inlined.")
471+
assert(
472+
cteRepartitions.forall(_.collectFirst {
473+
case f: Filter if f.condition.exists(_.isInstanceOf[Rand]) => f
474+
}.isEmpty),
475+
"Non-deterministic predicate should not be pushed down to the CTE def 'v'.")
476+
val randFilters = df.queryExecution.optimizedPlan.collect {
477+
case f: Filter if f.condition.exists(_.isInstanceOf[Rand]) => f
478+
}
479+
assert(randFilters.length == 2,
480+
"Each reference's non-deterministic predicate should be evaluated once.")
481+
}
482+
}
483+
484+
test("SPARK-59434: deterministic conjuncts are still pushed into a CTE def") {
485+
withTempView("t") {
486+
Seq((0, 1), (1, 2), (2, 3)).toDF("c1", "c2").createOrReplaceTempView("t")
487+
val df = sql(
488+
"""with v as (select c1, c2, rand(1) r from t)
489+
|select c1 from v where c1 > 0 and rand(2) < 0.5
490+
|union all
491+
|select c1 from v where c1 < 2
492+
|""".stripMargin)
493+
val cteRepartitions = df.queryExecution.optimizedPlan.collect {
494+
case r: RepartitionOperation => r
495+
}
496+
assert(cteRepartitions.nonEmpty, "CTE should not be inlined after optimization.")
497+
// The non-deterministic conjunct stays at the reference, the deterministic ones are
498+
// still OR-merged and pushed into the definition.
499+
val distinctCteRepartitions = cteRepartitions.map(_.canonicalized).distinct
500+
assert(distinctCteRepartitions.length == 1)
501+
assert(
502+
distinctCteRepartitions.head.collectFirst {
503+
case f: Filter if f.condition.semanticEquals(
504+
Or(GreaterThan(f.output(0), Literal(0)), LessThan(f.output(0), Literal(2)))) => f
505+
}.isDefined,
506+
"Predicate 'c1 > 0 OR c1 < 2' should be pushed down to the CTE def 'v'.")
507+
assert(
508+
distinctCteRepartitions.head.collectFirst {
509+
case f: Filter if f.condition.exists(_.isInstanceOf[Rand]) => f
510+
}.isEmpty,
511+
"Non-deterministic predicate should not be pushed down to the CTE def 'v'.")
512+
}
513+
}
514+
515+
test("SPARK-59434: a non-deterministic reference blocks push-down for its siblings") {
516+
withTempView("t") {
517+
Seq((0, 1), (1, 2), (2, 3)).toDF("c1", "c2").createOrReplaceTempView("t")
518+
val df = sql(
519+
"""with v as (select c1, c2, rand(1) r from t)
520+
|select c1 from v where rand(2) < 0.5
521+
|union all
522+
|select c1 from v where c1 > 0
523+
|""".stripMargin)
524+
val cteRepartitions = df.queryExecution.optimizedPlan.collect {
525+
case r: RepartitionOperation => r
526+
}
527+
assert(cteRepartitions.nonEmpty, "CTE should not be inlined after optimization.")
528+
// The first reference has no pushable predicate, so the combined predicate is TRUE and
529+
// the definition gets no filter. The sibling's 'c1 > 0' must not be pushed on its own,
530+
// which would drop rows the first reference needs.
531+
assert(
532+
cteRepartitions.forall(_.collectFirst { case f: Filter => f }.isEmpty),
533+
"CTE def 'v' should get no pushed-down filter.")
534+
}
535+
}
536+
537+
test("SPARK-59434: a non-deterministic predicate is evaluated once per CTE reference") {
538+
withSQLConf(SQLConf.SHUFFLE_PARTITIONS.key -> "1") {
539+
withTempView("t") {
540+
spark.range(0, 6, 1, 1).selectExpr("cast(id as int) c1").createOrReplaceTempView("t")
541+
// `monotonically_increasing_id` counts the rows it sees within a partition, so each
542+
// reference drops exactly its own first row. A second evaluation in the shared def
543+
// would drop a row there as well, leaving fewer. `rand(1)` only keeps the def from
544+
// being inlined; column pruning removes it, so it never reaches a filter.
545+
val df = sql(
546+
"""with v as (select c1, rand(1) r from t)
547+
|select c1 from v where monotonically_increasing_id() > 0
548+
|union all
549+
|select c1 from v where monotonically_increasing_id() > 0
550+
|""".stripMargin)
551+
assert(
552+
df.queryExecution.optimizedPlan.exists(_.isInstanceOf[RepartitionOperation]),
553+
"Non-deterministic With-CTE with multiple references should not be inlined.")
554+
assert(df.count() === 10, "Each reference should drop only its own first row.")
555+
}
556+
}
557+
}
558+
454559
test("Views with CTEs - 1 temp view") {
455560
withTempView("t", "t2") {
456561
Seq((0, 1), (1, 2)).toDF("c1", "c2").createOrReplaceTempView("t")

0 commit comments

Comments
 (0)