Describe the bug
dev/diffs/3.5.9.diff, dev/diffs/4.0.4.diff and dev/diffs/4.1.3.diff each weaken an assertion in AdaptiveQueryExecSuite to a value that contradicts the line immediately after it. In the test Reuse the default parallelism in local shuffle read, the diffs change Spark's assert(localReads.length == 2) to == 1, but leave the two statements that follow untouched:
val localReads = collect(adaptivePlan) {
case read: AQEShuffleReadExec if read.isLocalRead => read
}
// Comet shuffle changes shuffle metrics
assert(localReads.length == 1)
val localShuffleRDD0 = localReads(0).execute().asInstanceOf[ShuffledRowRDD]
val localShuffleRDD1 = localReads(1).execute().asInstanceOf[ShuffledRowRDD]
The body cannot succeed for any collection length. With one local read the assertion passes and localReads(1) throws IndexOutOfBoundsException; with two the assertion fails. Upstream Spark expects two, which is why localReads(1) is there.
The change is byte-identical in all three diffs, and in each one it is applied to the second of a pair of adjacent tests. The first, Reuse the parallelism of coalesced shuffle in local shuffle read, is tagged the same way and correctly keeps == 2, so the edit looks accidental rather than deliberate.
Why CI is green
Both tests carry IgnoreComet("Comet shuffle changes shuffle partition size"), and IgnoreComet skips only when Comet is enabled. Comet's CI runs the Spark SQL suites with ENABLE_COMET=true, so the body never executes there. It breaks only the Spark-only baseline run, which is the mode a contributor uses to confirm that a failure is Comet's rather than Spark's. Since the assertion is already unreachable under Comet, weakening it buys nothing even in the mode it was written for.
To Reproduce
Apply any of the three diffs to the matching Spark tag and run the suite with Comet disabled:
git clone -b v4.1.3 --single-branch --depth 1 https://github.com/apache/spark.git apache-spark
cd apache-spark
git apply ../datafusion-comet/dev/diffs/4.1.3.diff
ENABLE_COMET=false build/sbt "sql/testOnly org.apache.spark.sql.execution.adaptive.AdaptiveQueryExecSuite -- -z \"Reuse the default parallelism in local shuffle read\""
Expected behavior
The assertion is restored to Spark's assert(localReads.length == 2) and the IgnoreComet tag is kept, so the baseline run exercises the test and the Comet run still skips it.
Additional context
dev/diffs/4.2.0.diff does not have this: #4950 leaves the assertion at Spark's value, keeping only the IgnoreComet tag. So the fix is to bring the other three into line with 4.2 rather than the reverse.
Follow the process in Running Spark SQL Tests for each diff: apply it to a checkout of the tag, edit the Spark source there, and regenerate with git diff. The diff files must not be hand-edited.
Reported by @sunchao in review of #4950, where only the 4.2 patch was in scope.
Describe the bug
dev/diffs/3.5.9.diff,dev/diffs/4.0.4.diffanddev/diffs/4.1.3.diffeach weaken an assertion inAdaptiveQueryExecSuiteto a value that contradicts the line immediately after it. In the testReuse the default parallelism in local shuffle read, the diffs change Spark'sassert(localReads.length == 2)to== 1, but leave the two statements that follow untouched:The body cannot succeed for any collection length. With one local read the assertion passes and
localReads(1)throwsIndexOutOfBoundsException; with two the assertion fails. Upstream Spark expects two, which is whylocalReads(1)is there.The change is byte-identical in all three diffs, and in each one it is applied to the second of a pair of adjacent tests. The first,
Reuse the parallelism of coalesced shuffle in local shuffle read, is tagged the same way and correctly keeps== 2, so the edit looks accidental rather than deliberate.Why CI is green
Both tests carry
IgnoreComet("Comet shuffle changes shuffle partition size"), andIgnoreCometskips only when Comet is enabled. Comet's CI runs the Spark SQL suites withENABLE_COMET=true, so the body never executes there. It breaks only the Spark-only baseline run, which is the mode a contributor uses to confirm that a failure is Comet's rather than Spark's. Since the assertion is already unreachable under Comet, weakening it buys nothing even in the mode it was written for.To Reproduce
Apply any of the three diffs to the matching Spark tag and run the suite with Comet disabled:
Expected behavior
The assertion is restored to Spark's
assert(localReads.length == 2)and theIgnoreComettag is kept, so the baseline run exercises the test and the Comet run still skips it.Additional context
dev/diffs/4.2.0.diffdoes not have this: #4950 leaves the assertion at Spark's value, keeping only theIgnoreComettag. So the fix is to bring the other three into line with 4.2 rather than the reverse.Follow the process in Running Spark SQL Tests for each diff: apply it to a checkout of the tag, edit the Spark source there, and regenerate with
git diff. The diff files must not be hand-edited.Reported by @sunchao in review of #4950, where only the 4.2 patch was in scope.