sarutak commented on code in PR #60049:
URL: https://github.com/apache/spark/pull/60049#discussion_r4451392868
##########
sql/hive/src/test/scala/org/apache/spark/sql/hive/HiveExternalCatalogVersionsSuite.scala:
##########
@@ -261,9 +262,13 @@ object PROCESS_TABLES extends QueryTest {
val isPythonVersionAvailable = TestUtils.isPythonVersionAvailable
val releaseMirror = sys.env.getOrElse("SPARK_RELEASE_MIRROR",
"https://dist.apache.org/repos/dist/release";)
- // Tests the latest version of every release line if Java version is at most
17.
+ // Spark 3.3, 4.0, and 4.2 are the first release lines that support Java 17,
21, and 25,
+ // respectively. No release line supports the other Java versions, e.g.,
Java 26.
+ val minimumTestingVersion: Option[String] =
+ Map(17 -> "3.3.0", 21 -> "4.0.0", 25 ->
"4.2.0").get(Runtime.version().feature())
Review Comment:
nit: When a release line starts supporting a new Java version and nobody
updates this map, the suite keeps testing nothing on that Java version. It logs
`Skip tests because old Spark versions don't support Java N.`, which is then
false, and still reports `backward compatibility` as passed, like the Java 26
row in the description (11 milliseconds). Java 21 was in that state from the
4.0.0 release until this PR, and Java 25 from the 4.2.0 release. Could we add a
short note here such as "Update this map when `docs/index.md` adds a Java
version"? Optionally, `assume(PROCESS_TABLES.testingVersions.nonEmpty)` in the
test would report every run that tests no old version (an unsupported Java
version, a stale map, or a failed fetch of the release listing) as canceled
instead of passed, but that is a behavior change beyond this map, so feel free
to leave it out of this PR.
##########
sql/hive/src/test/scala/org/apache/spark/sql/hive/HiveExternalCatalogVersionsSuite.scala:
##########
@@ -198,10 +198,11 @@ class HiveExternalCatalogVersionsSuite extends
SparkSubmitTestUtils {
if (PROCESS_TABLES.testingVersions.isEmpty) {
if (PROCESS_TABLES.isPythonVersionAvailable) {
- if (Utils.isJavaVersionAtMost17) {
+ if (PROCESS_TABLES.minimumTestingVersion.isDefined) {
logError("Fail to get the latest Spark versions to test.")
Review Comment:
nit: The new `>= minimumTestingVersion` filter adds another way to reach
this branch after a successful fetch: no released version in
`[minimumTestingVersion, SPARK_VERSION)`, e.g., right after adding a map entry
for a Java version whose first supporting release line is not released yet.
Then "Fail to get the latest Spark versions to test." is misleading. It only
affects the log, so feel free to ignore.
##########
sql/hive/src/test/scala/org/apache/spark/sql/hive/HiveExternalCatalogVersionsSuite.scala:
##########
@@ -273,6 +278,7 @@ object PROCESS_TABLES extends QueryTest {
.filter(sparkVersionPattern.unanchored.matches(_))
.map(sparkVersionPattern.findFirstMatchIn(_).get.group(1))
.filter(_ < org.apache.spark.SPARK_VERSION)
+ .filter(_ >= minimumTestingVersion.get)
Review Comment:
nit: This comparison, like the existing `_ < org.apache.spark.SPARK_VERSION`
above, is a string comparison. It is correct only while every version component
is a single digit, both in what `sparkVersionPattern` captures and in
`SPARK_VERSION` and the map values. For example, if the regex were widened to
multi-digit components, this line would drop `4.10.0` on Java 25 because
`"4.10.0" >= "4.2.0"` is false. A one-line comment, or comparing
`VersionUtils.majorMinorPatchVersion` results, would make that explicit. Also,
matching on the option (e.g., `minimumTestingVersion match { case Some(min) if
isPythonVersionAvailable => ... case _ => Seq.empty }`) would avoid the `.get`
that relies on the `isDefined` guard 10 lines above, and a `PROCESS_TABLES`
member for `Runtime.version().feature()` would let line 205 reuse it.
--
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]
