apache / apache/datafusion-comet
Move CometCollationSuite into the spark-4.1+ test shim to avoid per-version duplication
- Dominant language
- Scala
- Stars
- 1.3k
- Forks
- 373
- Avg merge
- 2d 4h
- Merged PRs (30d)
- 198
Description
## Describe the bug / cleanup
In #4693, datetime expression collation tests were added to `spark/src/test/spark-4.1/org/apache/spark/sql/CometCollationSuite.scala`. Although these tests behave identically on Spark 4.1 and 4.2+, they currently reside in the `spark-4.1` test shim, which applies only to the 4.1 profile. As noted in the review (https://github.com/apache/datafusion-comet/pull/4693#discussion_r3481806406), the recently introduced `spark-4.1+` shim is the appropriate location for code shared across Spark 4.1 and all later 4.x releases. Keeping the suite in `spark-4.1` would necessitate duplication once the `spark-4.2` test profile is configured.
## Additional context
Two things are needed, and neither is a trivial file copy:
1. The test build does not pick up the `4.1+` shim yet. `add-test-source` in `spark/pom.xml` only adds `${shims.majorVerSrc}` and `${shims.minorVerSrc}`; `${shims.minorPlusVerSrc}` is wired into `add-source` (main sources) but not into the test sources.
2. `CometCollationSuite` already exists in the `spark-4.1` shim, so the suite has to be *moved* rather than copied — having the same class in both `spark-4.1` and `spark-4.1+` would put two `org.apache.spark.sql.CometCollationSuite` classes on the 4.1 classpath and fail to compile.
The `spark-4.0` suite should be left as-is: it carries 4.0-specific join tests and does not pick up the `4.1+` shim.
## Proposed change
- Add `src/test/${shims.minorPlusVerSrc}` to the`add-test-source` execution in `spark/pom.xml`.
- `git mv` the suite from `spark/src/test/spark-4.1/...` to `spark/src/test/spark-4.1+/...` (content unchanged).
Verified with:
- `./mvnw test -Pspark-4.1 -Dtest=none -Dsuites=org.apache.spark.sql.CometCollationSuite`
- `./mvnw compile test-compile -Pspark-4.2 -DskipTests`
Contributor guide
Assessment
This issue has not been assessed yet.