dongjoon-hyun commented on PR #57845:
URL: https://github.com/apache/spark/pull/57845#issuecomment-5220134345
Nice optimization — pre-selecting the hash function and UDF avoids the
per-term `hashFuncVersion` match and `$(numFeatures)` param lookup, and
`OpenHashMap[Int, Int]` benefits from value specialization. I verified the
semantics are unchanged: binary mode's `update(index, 1)` matches the old
`changeValue` result, `Vectors.sparse(n, Seq)` sorts by index in both paths,
and the eta-expanded object methods keep the closure free of the `HashingTF`
receiver.
A few suggestions:
1. The binary flag and hash version are independent axes, so the 2x2 match
could be simplified to a hash-function match plus a binary branch, mirroring
the existing `indexOf` structure:
```scala
val hashFunc: Any => Int = hashFuncVersion match {
case HashingTF.SPARK_2_MURMUR3_HASH => OldHashingTF.murmur3Hash
case HashingTF.SPARK_3_MURMUR3_HASH => FeatureHasher.murmur3Hash
case _ => throw new IllegalArgumentException("Illegal hash function
version setting.")
}
val hashUDF = if ($(binary)) binaryHashUDF(hashFunc) else
countHashUDF(hashFunc)
```
2. Test coverage: with the new test, three of the four `(binary,
hashFuncVersion)` combinations are exercised through `transform`; only `(binary
= true, SPARK_2_MURMUR3_HASH)` remains uncovered. Adding a `setBinary(true)`
comparison in the new test would close that path. Note the 2.4.4 model's
`binary` is the default `false`, so
`mLlibHashingTF.setBinary(loadedHashingTF.getBinary)` is effectively a no-op
today.
3. Nit: the sparse-vector construction is duplicated between the two UDF
helpers, and `new OpenHashMap[Int, Int]` drops the `()` used elsewhere in this
file.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
