andygrove opened a new issue, #6122:
URL: https://github.com/apache/datafusion-comet/issues/6122
## 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:
```scala
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:
```shell
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: apache/datafusion-comet#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](https://github.com/apache/datafusion-comet/blob/main/docs/source/contributor-guide/spark-sql-tests.md)
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 apache/datafusion-comet#4950, where only
the 4.2 patch was in scope.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
