holdenk commented on a change in pull request #20146: [SPARK-11215][ML] Add
multiple columns support to StringIndexer
URL: https://github.com/apache/spark/pull/20146#discussion_r243724192
##########
File path: mllib/src/main/scala/org/apache/spark/ml/feature/StringIndexer.scala
##########
@@ -172,37 +255,63 @@ object StringIndexer extends
DefaultParamsReadable[StringIndexer] {
@Since("1.6.0")
override def load(path: String): StringIndexer = super.load(path)
+
+ // Returns a function used to sort strings by frequency (ascending or
descending).
+ // In case of equal frequency, it sorts strings by alphabet (ascending).
+ private[feature] def getSortFunc(
+ ascending: Boolean): ((String, Long), (String, Long)) => Boolean = {
+ (a: (String, Long), b: (String, Long)) => {
+ if (a._2 == b._2) {
+ a._1 < b._1
+ } else {
+ if (ascending) a._2 < b._2 else a._2 > b._2
Review comment:
This is minor and a nit given it's use here, and the branch prediction will
take care of this quickly -- but we could move the check and return two
different functions (or just define two different sort functions) and avoid
having the extra branch during the sort). But in other places it would be more
important to optimize this so just a side comment while I'm reading through.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
