Skip to content

Commit 9b8a911

Browse files
sunchaosunchao
committed
address comments
- move version parsing to `VersionUtils` - add compatibility test - use `Option` for null
1 parent 6657c87 commit 9b8a911

File tree

4 files changed

+59
-56
lines changed

4 files changed

+59
-56
lines changed

‎core/src/main/scala/org/apache/spark/util/VersionUtils.scala‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ private[spark] object VersionUtils {
2424

2525
private val majorMinorRegex = """^(\d+)\.(\d+)(\..*)?$""".r
2626
private val shortVersionRegex = """^(\d+\.\d+\.\d+)(.*)?$""".r
27+
private val majorMinorPatchRegex = """^(\d+)(?:\.(\d+)(?:\.(\d+)(?:[.-].*)?)?)?$""".r
2728

2829
/**
2930
* Given a Spark version string, return the major version number.
@@ -63,4 +64,37 @@ private[spark] object VersionUtils {
6364
s" version string, but it could not find the major and minor version numbers.")
6465
}
6566
}
67+
68+
/**
69+
* Retrieves the major, minor and patch parts from the input `version`. Returns `None` if the
70+
* input is not of a valid format.
71+
*
72+
* Examples of valid version:
73+
* - 1
74+
* - 2.4
75+
* - 3.2.2
76+
* - 3.2.2.4
77+
* - 3.3.1-SNAPSHOT
78+
* - 3.2.2.4SNAPSHOT (we only retrieve the first 3 components)
79+
*
80+
* Examples of invalid version:
81+
* - ABC
82+
* - 1X
83+
* - 2.4XYZ
84+
* - 2.4-SNAPSHOT
85+
* - 3.4.5ABC
86+
*
87+
* @return A non-empty option containing a 3-value tuple (major, minor, patch) iff the
88+
* input is a valid version. `None` otherwise.
89+
*/
90+
def majorMinorPatchVersion(version: String): Option[(Int, Int, Int)] = {
91+
majorMinorPatchRegex.findFirstMatchIn(version) match {
92+
case Some(m) =>
93+
val major = m.group(1).toInt
94+
val minor = Option(m.group(2)).map(_.toInt).getOrElse(0)
95+
val patch = Option(m.group(3)).map(_.toInt).getOrElse(0)
96+
Some((major, minor, patch))
97+
case None => None
98+
}
99+
}
66100
}

‎core/src/test/scala/org/apache/spark/util/VersionUtilsSuite.scala‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -98,4 +98,21 @@ class VersionUtilsSuite extends SparkFunSuite {
9898
}
9999
}
100100
}
101+
102+
test("SPARK-33212: retrieve major/minor/patch version parts") {
103+
assert(VersionUtils.majorMinorPatchVersion("3.2.2").contains((3, 2, 2)))
104+
assert(VersionUtils.majorMinorPatchVersion("3.2.2.4").contains((3, 2, 2)))
105+
assert(VersionUtils.majorMinorPatchVersion("3.2.2-SNAPSHOT").contains((3, 2, 2)))
106+
assert(VersionUtils.majorMinorPatchVersion("3.2.2.4XXX").contains((3, 2, 2)))
107+
assert(VersionUtils.majorMinorPatchVersion("3.2").contains((3, 2, 0)))
108+
assert(VersionUtils.majorMinorPatchVersion("3").contains((3, 0, 0)))
109+
110+
// illegal cases
111+
assert(VersionUtils.majorMinorPatchVersion("ABC").isEmpty)
112+
assert(VersionUtils.majorMinorPatchVersion("3X").isEmpty)
113+
assert(VersionUtils.majorMinorPatchVersion("3.2-SNAPSHOT").isEmpty)
114+
assert(VersionUtils.majorMinorPatchVersion("3.2ABC").isEmpty)
115+
assert(VersionUtils.majorMinorPatchVersion("3-ABC").isEmpty)
116+
assert(VersionUtils.majorMinorPatchVersion("3.2.4XYZ").isEmpty)
117+
}
101118
}

‎sql/hive/src/main/scala/org/apache/spark/sql/hive/client/IsolatedClientLoader.scala‎

Lines changed: 3 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,6 @@ import java.io.File
2121
import java.lang.reflect.InvocationTargetException
2222
import java.net.{URL, URLClassLoader}
2323
import java.util
24-
import java.util.regex.Pattern
2524

2625
import scala.util.Try
2726

@@ -38,7 +37,7 @@ import org.apache.spark.sql.catalyst.util.quietly
3837
import org.apache.spark.sql.hive.HiveUtils
3938
import org.apache.spark.sql.internal.NonClosableMutableURLClassLoader
4039
import org.apache.spark.sql.internal.SQLConf
41-
import org.apache.spark.util.{MutableURLClassLoader, Utils}
40+
import org.apache.spark.util.{MutableURLClassLoader, Utils, VersionUtils}
4241

4342
/** Factory for `IsolatedClientLoader` with specific versions of hive. */
4443
private[hive] object IsolatedClientLoader extends Logging {
@@ -90,7 +89,7 @@ private[hive] object IsolatedClientLoader extends Logging {
9089
}
9190

9291
def hiveVersion(version: String): HiveVersion = {
93-
getVersionParts(version).flatMap {
92+
VersionUtils.majorMinorPatchVersion(version).flatMap {
9493
case (12, _, _) | (0, 12, _) => Some(hive.v12)
9594
case (13, _, _) | (0, 13, _) => Some(hive.v13)
9695
case (14, _, _) | (0, 14, _) => Some(hive.v14)
@@ -111,43 +110,12 @@ private[hive] object IsolatedClientLoader extends Logging {
111110
}
112111

113112
def supportHadoopShadedClient(hadoopVersion: String): Boolean = {
114-
getVersionParts(hadoopVersion).exists {
113+
VersionUtils.majorMinorPatchVersion(hadoopVersion).exists {
115114
case (3, 2, v) if v >= 2 => true
116115
case _ => false
117116
}
118117
}
119118

120-
/**
121-
* Retrieves the major, minor and patch parts from the input `version`. Returns `None` if the
122-
* input is not of a valid format.
123-
*
124-
* Examples of valid version:
125-
* - 1
126-
* - 2.4
127-
* - 3.2.2
128-
* - 3.2.2.4
129-
* - 3.3.1-SNAPSHOT
130-
* - 3.2.2.4SNAPSHOT (we only retrieve the first 3 components)
131-
*
132-
* Examples of invalid version:
133-
* - ABC
134-
* - 1X
135-
* - 2.4XYZ
136-
* - 2.4-SNAPSHOT
137-
* - 3.4.5ABC
138-
*/
139-
def getVersionParts(version: String): Option[(Int, Int, Int)] = {
140-
val matcher = VERSION_PATTERN.matcher(version)
141-
if (matcher.matches() && matcher.groupCount() == 3) {
142-
val major = matcher.group(1).toInt
143-
val minor = if (matcher.group(2) == null) 0 else matcher.group(2).toInt
144-
val patch = if (matcher.group(3) == null) 0 else matcher.group(3).toInt
145-
Some((major, minor, patch))
146-
} else {
147-
None
148-
}
149-
}
150-
151119
private def downloadVersion(
152120
version: HiveVersion,
153121
hadoopVersion: String,
@@ -182,10 +150,6 @@ private[hive] object IsolatedClientLoader extends Logging {
182150
tempDir.listFiles().map(_.toURI.toURL)
183151
}
184152

185-
// A regex pattern to match Maven version numbers
186-
private lazy val VERSION_PATTERN =
187-
Pattern.compile("(\\d+)(?:\\.(\\d+)(?:\\.(\\d+)(?:[.-].*)?)?)?")
188-
189153
// A map from a given pair of HiveVersion and Hadoop version to jar files.
190154
// It is only used by forVersion.
191155
private val resolvedVersions =

‎sql/hive/src/test/scala/org/apache/spark/sql/hive/client/HadoopVersionInfoSuite.scala‎

Lines changed: 5 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ import java.io.File
2121
import java.net.URLClassLoader
2222

2323
import org.apache.hadoop.conf.Configuration
24+
import org.apache.hadoop.util.VersionInfo
2425

2526
import org.apache.spark.{SparkConf, SparkFunSuite}
2627
import org.apache.spark.sql.hive.{HiveExternalCatalog, HiveUtils}
@@ -69,23 +70,6 @@ class HadoopVersionInfoSuite extends SparkFunSuite {
6970
}
7071
}
7172

72-
test("SPARK-33212: test getVersionParts()") {
73-
assert(IsolatedClientLoader.getVersionParts("3.2.2").contains((3, 2, 2)))
74-
assert(IsolatedClientLoader.getVersionParts("3.2.2.4").contains((3, 2, 2)))
75-
assert(IsolatedClientLoader.getVersionParts("3.2.2-SNAPSHOT").contains((3, 2, 2)))
76-
assert(IsolatedClientLoader.getVersionParts("3.2.2.4XXX").contains((3, 2, 2)))
77-
assert(IsolatedClientLoader.getVersionParts("3.2").contains((3, 2, 0)))
78-
assert(IsolatedClientLoader.getVersionParts("3").contains((3, 0, 0)))
79-
80-
// illegal cases
81-
assert(IsolatedClientLoader.getVersionParts("ABC").isEmpty)
82-
assert(IsolatedClientLoader.getVersionParts("3X").isEmpty)
83-
assert(IsolatedClientLoader.getVersionParts("3.2-SNAPSHOT").isEmpty)
84-
assert(IsolatedClientLoader.getVersionParts("3.2ABC").isEmpty)
85-
assert(IsolatedClientLoader.getVersionParts("3-ABC").isEmpty)
86-
assert(IsolatedClientLoader.getVersionParts("3.2.4XYZ").isEmpty)
87-
}
88-
8973
test("SPARK-32212: test supportHadoopShadedClient()") {
9074
assert(IsolatedClientLoader.supportHadoopShadedClient("3.2.2"))
9175
assert(IsolatedClientLoader.supportHadoopShadedClient("3.2.3"))
@@ -99,4 +83,8 @@ class HadoopVersionInfoSuite extends SparkFunSuite {
9983
assert(!IsolatedClientLoader.supportHadoopShadedClient("3.2.1"))
10084
assert(!IsolatedClientLoader.supportHadoopShadedClient("4"))
10185
}
86+
87+
test("SPARK-32212: built-in Hadoop version should support shaded client") {
88+
assert(IsolatedClientLoader.supportHadoopShadedClient(VersionInfo.getVersion))
89+
}
10290
}

0 commit comments

Comments
 (0)