xkrogen commented on a change in pull request #31203:
URL: https://github.com/apache/spark/pull/31203#discussion_r563867053
##########
File path: core/src/test/scala/org/apache/spark/util/VersionUtilsSuite.scala
##########
@@ -98,4 +98,21 @@ class VersionUtilsSuite extends SparkFunSuite {
}
}
}
+
+ test("SPARK-33212: retrieve major/minor/patch version parts") {
+ assert(VersionUtils.majorMinorPatchVersion("3.2.2").contains((3, 2, 2)))
+ assert(VersionUtils.majorMinorPatchVersion("3.2.2.4").contains((3, 2, 2)))
+ assert(VersionUtils.majorMinorPatchVersion("3.2.2-SNAPSHOT").contains((3,
2, 2)))
+ assert(VersionUtils.majorMinorPatchVersion("3.2.2.4XXX").contains((3, 2,
2)))
+ assert(VersionUtils.majorMinorPatchVersion("3.2").contains((3, 2, 0)))
+ assert(VersionUtils.majorMinorPatchVersion("3").contains((3, 0, 0)))
+
+ // illegal cases
+ assert(VersionUtils.majorMinorPatchVersion("ABC").isEmpty)
+ assert(VersionUtils.majorMinorPatchVersion("3X").isEmpty)
+ assert(VersionUtils.majorMinorPatchVersion("3.2-SNAPSHOT").isEmpty)
+ assert(VersionUtils.majorMinorPatchVersion("3.2ABC").isEmpty)
+ assert(VersionUtils.majorMinorPatchVersion("3-ABC").isEmpty)
+ assert(VersionUtils.majorMinorPatchVersion("3.2.4XYZ").isEmpty)
Review comment:
You can consider simplifying this a little like:
```
Seq("ABC", "3X", ...).foreach { versionStr =>
assert(VersionUtils.majorMinorPatchVersion(versionStr).isEmpty, s"version
$versionStr")
}
```
##########
File path:
sql/hive/src/test/scala/org/apache/spark/sql/hive/client/HadoopVersionInfoSuite.scala
##########
@@ -68,4 +69,22 @@ class HadoopVersionInfoSuite extends SparkFunSuite {
Utils.deleteRecursively(ivyPath)
}
}
+
+ test("SPARK-32212: test supportHadoopShadedClient()") {
+ assert(IsolatedClientLoader.supportHadoopShadedClient("3.2.2"))
+ assert(IsolatedClientLoader.supportHadoopShadedClient("3.2.3"))
+ assert(IsolatedClientLoader.supportHadoopShadedClient("3.2.2.1"))
+ assert(IsolatedClientLoader.supportHadoopShadedClient("3.2.2-XYZ"))
+ assert(IsolatedClientLoader.supportHadoopShadedClient("3.2.2.4-SNAPSHOT"))
Review comment:
Maybe consider shortening like:
```
Seq("3.2.2", "3.2.3", ...).foreach { versionStr =>
assert(IsolatedClientLoader.supportHadoopShadedClient(versionStr),
s"version $versionStr")
}
```
(and same for the negative cases)
##########
File path: core/src/main/scala/org/apache/spark/util/VersionUtils.scala
##########
@@ -63,4 +64,37 @@ private[spark] object VersionUtils {
s" version string, but it could not find the major and minor version
numbers.")
}
}
+
+ /**
+ * Retrieves the major, minor and patch parts from the input `version`.
Returns `None` if the
+ * input is not of a valid format.
Review comment:
I think we should mention that minor/patch versions are filled in as 0
if they're not found. This is different from the behavior of other methods in
this class (e.g. majorMinor will give an error if minor is not present)
##########
File path:
sql/hive/src/main/scala/org/apache/spark/sql/hive/client/IsolatedClientLoader.scala
##########
@@ -88,31 +88,40 @@ private[hive] object IsolatedClientLoader extends Logging {
barrierPrefixes = barrierPrefixes)
}
- def hiveVersion(version: String): HiveVersion = version match {
- case "12" | "0.12" | "0.12.0" => hive.v12
- case "13" | "0.13" | "0.13.0" | "0.13.1" => hive.v13
- case "14" | "0.14" | "0.14.0" => hive.v14
- case "1.0" | "1.0.0" | "1.0.1" => hive.v1_0
- case "1.1" | "1.1.0" | "1.1.1" => hive.v1_1
- case "1.2" | "1.2.0" | "1.2.1" | "1.2.2" => hive.v1_2
- case "2.0" | "2.0.0" | "2.0.1" => hive.v2_0
- case "2.1" | "2.1.0" | "2.1.1" => hive.v2_1
- case "2.2" | "2.2.0" => hive.v2_2
- case "2.3" | "2.3.0" | "2.3.1" | "2.3.2" | "2.3.3" | "2.3.4" | "2.3.5" |
"2.3.6" | "2.3.7" |
- "2.3.8" => hive.v2_3
- case "3.0" | "3.0.0" => hive.v3_0
- case "3.1" | "3.1.0" | "3.1.1" | "3.1.2" => hive.v3_1
- case version =>
+ def hiveVersion(version: String): HiveVersion = {
+ VersionUtils.majorMinorPatchVersion(version).flatMap {
+ case (12, _, _) | (0, 12, _) => Some(hive.v12)
+ case (13, _, _) | (0, 13, _) => Some(hive.v13)
+ case (14, _, _) | (0, 14, _) => Some(hive.v14)
+ case (1, 0, _) => Some(hive.v1_0)
+ case (1, 1, _) => Some(hive.v1_1)
+ case (1, 2, _) => Some(hive.v1_2)
+ case (2, 0, _) => Some(hive.v2_0)
+ case (2, 1, _) => Some(hive.v2_1)
+ case (2, 2, _) => Some(hive.v2_2)
+ case (2, 3, _) => Some(hive.v2_3)
+ case (3, 0, _) => Some(hive.v3_0)
+ case (3, 1, _) => Some(hive.v3_1)
+ case _ => None
+ }.getOrElse {
throw new UnsupportedOperationException(s"Unsupported Hive Metastore
version ($version). " +
s"Please set ${HiveUtils.HIVE_METASTORE_VERSION.key} with a valid
version.")
+ }
+ }
+
+ def supportHadoopShadedClient(hadoopVersion: String): Boolean = {
Review comment:
Should this be `supportsHadoopShadedClient` instead of
`supportHadoopShadedClient` ?
##########
File path: core/src/main/scala/org/apache/spark/util/VersionUtils.scala
##########
@@ -63,4 +64,37 @@ private[spark] object VersionUtils {
s" version string, but it could not find the major and minor version
numbers.")
}
}
+
+ /**
+ * Retrieves the major, minor and patch parts from the input `version`.
Returns `None` if the
+ * input is not of a valid format.
+ *
+ * Examples of valid version:
+ * - 1
+ * - 2.4
+ * - 3.2.2
+ * - 3.2.2.4
+ * - 3.3.1-SNAPSHOT
+ * - 3.2.2.4SNAPSHOT (we only retrieve the first 3 components)
+ *
+ * Examples of invalid version:
+ * - ABC
+ * - 1X
+ * - 2.4XYZ
+ * - 2.4-SNAPSHOT
+ * - 3.4.5ABC
+ *
+ * @return A non-empty option containing a 3-value tuple (major, minor,
patch) iff the
+ * input is a valid version. `None` otherwise.
+ */
+ def majorMinorPatchVersion(version: String): Option[(Int, Int, Int)] = {
+ majorMinorPatchRegex.findFirstMatchIn(version) match {
+ case Some(m) =>
+ val major = m.group(1).toInt
+ val minor = Option(m.group(2)).map(_.toInt).getOrElse(0)
+ val patch = Option(m.group(3)).map(_.toInt).getOrElse(0)
+ Some((major, minor, patch))
+ case None => None
Review comment:
Maybe a little cleaner to use `map`, the `None => None` is a little
awkward:
```
majorMinorPatchRegex.findFirstMatchIn(version).map { m =>
val major = m.group(1).toInt
val minor = Option(m.group(2)).map(_.toInt).getOrElse(0)
val patch = Option(m.group(3)).map(_.toInt).getOrElse(0)
(major, minor, patch)
}
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
