comphead commented on code in PR #7404:
URL: https://github.com/apache/datafusion-comet/pull/7404#discussion_r4470289276
##########
spark/src/main/scala/org/apache/comet/serde/aggregates.scala:
##########
@@ -963,14 +963,8 @@ private[comet] object RegrSparkVersions {
private def isAfterMinor(major: Int, minor: Int, thanMajor: Int, thanMinor:
Int): Boolean =
major > thanMajor || (major == thanMajor && minor > thanMinor)
- // Spark's VersionUtils is private to its packages, so parse the leading
major.minor.patch
- // here; a missing patch reads as 0 and a suffix such as -SNAPSHOT is
ignored.
- private val versionPattern = """^(\d+)\.(\d+)(?:\.(\d+))?""".r
-
private def majorMinorPatch(sparkVersion: String): Option[(Int, Int, Int)] =
- versionPattern.findFirstMatchIn(sparkVersion).map { m =>
- (m.group(1).toInt, m.group(2).toInt,
Option(m.group(3)).map(_.toInt).getOrElse(0))
- }
+ Utils.majorMinorPatchVersion(sparkVersion)
Review Comment:
Two points on this wrapper.
1. `majorMinorPatch` is now only an alias of `Utils.majorMinorPatchVersion`.
The two `match` expressions above could call `Utils` directly.
2. The description says a string like `4.0.5_vendor` now takes "the same
fallback the other checks already took". The other callers
(`keepsSignedZeroKeys`, `ArraySetSupport.normalizesFloats`,
`SparkFixes.fixedIn`) use `.exists`, so `None` means `false`. These two fall
back to `sparkVersion >= "3.5"`, which is `true` for `3.5.1_vendor` and
`4.0.1_vendor`. Those versions predate both fixes, and the old regex classified
them correctly. I have not run it, and such strings are rare. Should the `None`
branches return `false` like the others?
##########
spark/src/test/scala/org/apache/spark/sql/comet/util/UtilsSuite.scala:
##########
@@ -729,4 +729,22 @@ class UtilsSuite extends CometTestBase {
full.close()
}
}
+
+ test("majorMinorPatchVersion parses Spark version strings like Spark's
VersionUtils") {
+ // Comet parses these itself rather than calling
org.apache.spark.util.VersionUtils, which is
+ // private[spark]; each expectation is what Spark's VersionUtils returns
for the same string.
+ Seq(
+ "3.5.8" -> Some((3, 5, 8)),
+ "3.5.10" -> Some((3, 5, 10)),
+ "4.2.0-SNAPSHOT" -> Some((4, 2, 0)),
+ "4.1.3-amzn-1" -> Some((4, 1, 3)),
+ "3.5.8.7.3.2.30000-15" -> Some((3, 5, 8)),
+ "3.5" -> Some((3, 5, 0)),
+ "3" -> Some((3, 0, 0)),
+ "3.5.8x" -> None,
+ "v3.5.9" -> None,
+ "" -> None).foreach { case (version, expected) =>
+ assert(Utils.majorMinorPatchVersion(version) == expected, version)
+ }
Review Comment:
The expected values are typed in by hand, so the test cannot catch a drift
from Spark. `UtilsSuite` is in `org.apache.spark.sql.comet.util`, so it can
still reach `private[spark]` `VersionUtils`, which has the same
`majorMinorPatchVersion` signature in all four tags. I read the sources but did
not compile this. Asserting `Utils.majorMinorPatchVersion(v) ==
VersionUtils.majorMinorPatchVersion(v)` would make the table an actual parity
check. Spark's `VersionUtilsSuite` also covers `3.2.2.4XXX`, `3.2-SNAPSHOT` and
`3-ABC`, which this table lacks.
--
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]
