[jvm-packages] XGBoost Spark integration refactor

[yanboliang]

Combine a bunch of PRs into one, to merge dev branch to master.

[codecov-io]

Codecov Report

Merging #3387 into master will decrease coverage by 0.5%.
The diff coverage is 63.22%.

@@             Coverage Diff              @@
##             master    #3387      +/-   ##
============================================
- Coverage     44.99%   44.49%   -0.51%     
+ Complexity      228      188      -40     
============================================
  Files           166      163       -3     
  Lines         12787    12769      -18     
  Branches        466      443      -23     
============================================
- Hits           5754     5681      -73     
- Misses         6841     6887      +46     
- Partials        192      201       +9

Impacted Files	Coverage Δ	Complexity Δ
...c/main/scala/ml/dmlc/xgboost4j/scala/Booster.scala	35.71% <0%> (-1.33%)	8 <0> (ø)
...t4j/scala/example/spark/SparkModelTuningTool.scala	0% <0%> (ø)	0 <0> (ø)	⬇️
...ost4j/scala/example/spark/SparkWithDataFrame.scala	0% <0%> (ø)	0 <0> (ø)	⬇️
...dmlc/xgboost4j/scala/spark/CheckpointManager.scala	72.91% <100%> (-4.17%)	13 <0> (ø)
...c/xgboost4j/scala/spark/params/BoosterParams.scala	59.3% <51.78%> (-6.04%)	0 <0> (ø)
.../dmlc/xgboost4j/scala/spark/XGBoostRegressor.scala	61.72% <61.72%> (ø)	16 <16> (?)
...dmlc/xgboost4j/scala/spark/XGBoostClassifier.scala	62.5% <62.5%> (ø)	18 <18> (?)
...c/xgboost4j/scala/spark/params/GeneralParams.scala	71.64% <68.96%> (-20.96%)	0 <0> (ø)
...oost4j/scala/spark/params/LearningTaskParams.scala	80% <76.47%> (-13.11%)	0 <0> (ø)
.../scala/ml/dmlc/xgboost4j/scala/spark/XGBoost.scala	73.59% <88.88%> (-1.72%)	0 <0> (ø)
... and 7 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 578a0c7...c1ff626. Read the comment docs.

[CodingCat]

There are several PRs merged to master before this one, would you check if this PR contains those fixes

[yanboliang]

@CodingCat Yes, I have rebased master.

[CodingCat]

@yanboliang sure, thanks, looks like some newly added test were failed....@RAMitchell would you mind sharing some insights on this?

[a-johnston]

Maybe related to #3406. Since this is a sequence made from an iterable the apply happening twice in (groups(i) != groups(i - 1)) could be linear time causing this method to be quadratic (thus blowing up for larger datasets)?

[CodingCat]

@yanboliang can you look at this, I think @a-johnston 's point is valid

and additionally, loading the whole group as Seq still makes it vulnerable to OOM

[a-johnston]

Anecdotally, @ngoyal2707 and I were checking this out earlier and it worked for smaller datasets (tens of thousands) but did not for larger (tens of millions) without ever hitting OOM although we could train data without groups on the same framework. Instead it just hung forever processing, although we never connected a debugger to check what impl is actually used. I think it might even be cleaner to just do something like

var lastGroup = -1
for (group <- groups) {
    if (lastGroup != group) {
        lastGroup = group
<etc>

[yanboliang]

@a-johnston You have a good point and I think your suggestion should be more optimized, Scala apply may have some performance issue. But the current code only cause time from O(N) to O(2N) at worst, it's still linear not quadratic(O(N^2)).
I will do some experiments in the following days for further check, if you found more clues, please feel free to share them. Thanks.

[a-johnston]

@yanboliang looking a bit closer, I'm pretty sure that groups here is a scala.collection.immutable.Stream$Cons which would lead to linear time apply and then quadratic overall (including the while loop). Linear is necessary with this approach since it's streaming over the entire column. While I generally prefer this approach compared to the old groupData, this is definitely a degradation.

Also if you're too busy to dig more into this, I can open a PR for this later today.

[yanboliang]

@a-johnston I see, Seq.apply is linear time, so overall will be quadratic. You suggested code could solve the performance issue as well. What about change the input format from Seq to Iterator to make it less vulnerable to OOM? Please feel free to open a PR for this.
BTW, the new approach is align with #2749 , if that PR get merged, we can switch the underlying implementation to leverage it. Thanks.