[SPARK-17462][MLLIB]use VersionUtils to parse Spark version strings #15055
Conversation
Several places in MLlib use custom regexes or other approaches. Those should be fixed to use the VersionUtils Signed-off-by: VinceShieh <[email protected]>
|
Test build #65241 has finished for PR 15055 at commit
|
|
@srowen @jkbradley do you have time to take a look at this one? |
| import org.apache.spark.sql._ | ||
| import org.apache.spark.sql.functions._ | ||
| import org.apache.spark.sql.types.{StructField, StructType} | ||
| import org.apache.spark.util.VersionUtils._ |
There was a problem hiding this comment.
In general I think we prefer importing classes rather than importing members, except where the latter is clearly clearer. I'm not sure it is here. It's minor either way.
Are there any other occurrences across the code? i want to try to solve these issues in one go.
There was a problem hiding this comment.
@srowen Sure, these are what I can find in ml/mllib.
There was a problem hiding this comment.
I meant, are there others in the whole code base, but, I don't actually see any more. Can you address the import?
There was a problem hiding this comment.
hmm, personally I think it's fine to keep it this way, though it's only 'majorVersion' needed here, so we can import org.apache.spark.util.VersionUtils.majorVersion instead. Sounds good to you? @srowen
There was a problem hiding this comment.
You can leave it. In general I think that when you find foo() in code one looks for foo in the same compilation unit, and it's a little distracting to find it was actually imported by itself. IDEs figure it out, sure, but this mechanism is really for things like importing a method used a lot or something. Anyway, we don't have a consistent approach in the code, so OK.
There was a problem hiding this comment.
got your point. I agree. I will change the importing to
import org.apache.spark.util.VersionUtils.majorVersion instead for clarity.
Signed-off-by: VinceShieh <[email protected]>
|
Test build #3430 has finished for PR 15055 at commit
|
|
Test build #68768 has finished for PR 15055 at commit
|
|
Merged to master/2.1 |
## What changes were proposed in this pull request? Several places in MLlib use custom regexes or other approaches to parse Spark versions. Those should be fixed to use the VersionUtils. This PR replaces custom regexes with VersionUtils to get Spark version numbers. ## How was this patch tested? Existing tests. Signed-off-by: VinceShieh vincent.xieintel.com Author: VinceShieh <[email protected]> Closes #15055 from VinceShieh/SPARK-17462. (cherry picked from commit de77c67) Signed-off-by: Sean Owen <[email protected]>
## What changes were proposed in this pull request? Several places in MLlib use custom regexes or other approaches to parse Spark versions. Those should be fixed to use the VersionUtils. This PR replaces custom regexes with VersionUtils to get Spark version numbers. ## How was this patch tested? Existing tests. Signed-off-by: VinceShieh vincent.xieintel.com Author: VinceShieh <[email protected]> Closes apache#15055 from VinceShieh/SPARK-17462.
3 participants
What changes were proposed in this pull request?
Several places in MLlib use custom regexes or other approaches to parse Spark versions.
Those should be fixed to use the VersionUtils. This PR replaces custom regexes with
VersionUtils to get Spark version numbers.
How was this patch tested?
Existing tests.
Signed-off-by: VinceShieh [email protected]