[SPARK-28514][ML] Remove the redundant transformImpl method in RF & GBT

[zhengruifeng]

What changes were proposed in this pull request?

Remove the redundant and confusing transformImpl method in RF & GBT;
In GBTClassifier & RandomForestClassifier, the real transform methods inherit from ProbabilisticClassificationModel which can deal with multi output columns.
The transformImpl method, which deals with only one column - predictionCol, completely does nothing. This is quite confusing.

How was this patch tested?

existing suites

[SparkQA]

Test build #108167 has finished for PR 25256 at commit 41c0f63.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[xuanyuanking]

Seem like a partial revert of #6300, cc @BryanCutler for a further review.

[BryanCutler]

Yes, from what I can remember the point of these methods was to broadcast the model. It's been a while since I looked at this and it has gotten a little confusing over time. I'm not sure if this is still needed or can be removed cc @mengxr @WeichenXu123

[srowen]

I think I'd leave this, as it's on purpose and probably for performance reasons. I wonder if we can just always broadcast the model here? What's the downside? the model is already by default serialized in the closure, so it should serialize. There's overhead to broadcasting a tiny model I guess, but maybe that's fine.

[BryanCutler]

I wonder if we can just always broadcast the model here?

This sounds reasonable to me and would make the code easier to follow

[zhengruifeng]

@BryanCutler @srowen I am neutral on model broadcasting, I notice that there are three approachs for broadcastable/small models to performance transformation:
1, directly serialize the model in the closure (the most cases);
2, broadcast the model in the transform method every time (like Word2Vec/GBTRegressor);
3, broadcast the model if it is not broadcasted yet, the the broadcasted model can be reused among calls (like CountVectorizer);
If the model broadcasting is better, can we apply it for all algs?

As to this pr, if it can improve performance, I am OK to leave GBTRegressor & RandomForestRegressor;
However, the transformImpl methods in GBTClassifier & RandomForestClassifier are never used, so I tend to remove them.

[srowen]

Is it really not used in GBTClassifier for example? it overrides a method in Predictor and that is called in transform, still.

[zhengruifeng]

@srowen The transform method (which need override transformImpl) defined in PredictionModel is overrided by the the one (which need override predict/predictRaw/...) define in ProbabilisticClassificationModel.

[SparkQA]

Test build #108562 has finished for PR 25256 at commit 41ca50d.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[srowen]

I see. I wonder if we should be extra safe and override transformImpl in ProbabilisticClassificationModel to throw an error, and mark it final, to ensure that subclasses don't inherit it. On the other hand... is it possible a future subclass would further override transform and then have a use for the base class transformImpl?

I'm OK with the current approach or further restricting inheritance of transformImpl. Up to your taste.

[zhengruifeng]

good idea, It is reasonable to restrict the transformImpl in ProbabilisticClassificationModel

[SparkQA]

Test build #108618 has finished for PR 25256 at commit 6c61f60.

• This patch fails to build.
• This patch merges cleanly.
• This patch adds no public classes.

[srowen]

Oh, this just can't be private

[SparkQA]

Test build #108636 has finished for PR 25256 at commit f6a10d0.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[srowen]

Merged to master