You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "zhengruifeng (via GitHub)" <gi...@apache.org> on 2026/08/25 10:21:42 UTC
[PR] [SPARK-58993][ML] Use OpenHashMap for CountVectorizer vocabulary lookup [spark]
zhengruifeng opened a new pull request, #58278:
URL: https://github.com/apache/spark/pull/58278
### What changes were proposed in this pull request?
Replace the immutable `Map[String, Int]` used by `CountVectorizerModel` for vocabulary lookup with `OpenHashMap[String, Int]`.
Add `Utils.toOpenHashMapWithIndex`, which accepts an `Array` so the initial size is known. `CountVectorizerModel` stores vocabulary indexes with an offset of one, reserving zero for missing terms and avoiding an `Option` allocation on each lookup.
### Why are the changes needed?
`CountVectorizerModel.transform` performs one vocabulary lookup per input token. The specialized `Int` values in `OpenHashMap` avoid boxed map values, and direct lookup with the zero sentinel avoids allocating `Option` objects.
In a local Java 17 microbenchmark with 262,144 string keys, 2 million lookups, and a 90% hit rate, immutable `Map.get` took about 69.5 ns/op while `OpenHashMap.apply` with the zero sentinel took about 37.6 ns/op, a roughly 46% improvement.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
Added coverage for `Utils.toOpenHashMapWithIndex` and ran:
- `build/sbt "core/testOnly *OpenHashMapSuite"`
- `build/sbt "mllib/testOnly *CountVectorizerSuite"`
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: OpenAI Codex (GPT-5)
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-58993][ML] Use Java HashMap for CountVectorizer vocabulary lookup [spark]
Posted by "zhengruifeng (via GitHub)" <gi...@apache.org>.
zhengruifeng closed pull request #58278: [SPARK-58993][ML] Use Java HashMap for CountVectorizer vocabulary lookup
URL: https://github.com/apache/spark/pull/58278
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-58993][ML] Use OpenHashMap for CountVectorizer vocabulary lookup [spark]
Posted by "zhengruifeng (via GitHub)" <gi...@apache.org>.
zhengruifeng commented on PR #58278:
URL: https://github.com/apache/spark/pull/58278#issuecomment-5420318267
Thanks for the detailed review. I reproduced the comparison with a CountVectorizer-like workload on Java 17: 262,144 String keys, 2 million lookups, and a 90% hit rate.
| Representation | Lookup | Estimated heap | JavaSerializer + LZ4 payload | Compressed deserialization |
| --- | ---: | ---: | ---: | ---: |
| Scala immutable Map | 69.5 ns/op | 25.6 MiB | 3.37 MB | 127.7 ms |
| Spark OpenHashMap | 38.3 ns/op | 17.5 MiB | 3.39 MB | 35.7 ms |
| java.util.HashMap | 20.7 ns/op | 22.3 MiB | 2.50 MB | 67.6 ms |
The checked-in OpenHashMapBenchmark agrees on lookup performance: java.util.HashMap is about 2.1x faster for random String-key lookup. OpenHashMap remains best for deserialized heap and deserialization speed, but java.util.HashMap gives the best result for this lookup-heavy use case while also producing the smallest default compressed broadcast payload.
I will switch this PR to java.util.HashMap. That also removes the zero-sentinel contract and the new OpenHashMap Kryo-registration issue. I will hoist Broadcast.value out of the per-token loop and remove the OpenHashMap-specific helper and test, which addresses the sizing, offset-contract, placement, and helper-test comments.
More broadly, these results suggest that we should revisit existing OpenHashMap/OpenHashSet call sites. The right replacement depends on the operation: JDK collections appear preferable for String-key insert/lookup workloads, while specialized primitive storage and changeValue-style aggregation can still favor the Spark collections. That should be evaluated separately per call site, including lookup/insert performance, heap footprint, serialization, and configured Kryo behavior.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-58993][ML] Use Java HashMap for CountVectorizer vocabulary lookup [spark]
Posted by "zhengruifeng (via GitHub)" <gi...@apache.org>.
zhengruifeng commented on PR #58278:
URL: https://github.com/apache/spark/pull/58278#issuecomment-5420948896
Hi @dongjoon-hyun, thank you for the detailed review. I addressed the comments and replied to each thread. The PR now uses a pre-sized `java.util.HashMap`, hoists the broadcast dereference outside the token loop, removes the sentinel/helper changes, and adds strict-Kryo registration coverage. I also added the lookup, heap, Java serialization, and Kryo comparison to the benchmark summary above. Could you please take another look when you have time?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-58993][ML] Use Java HashMap for CountVectorizer vocabulary lookup [spark]
Posted by "zhengruifeng (via GitHub)" <gi...@apache.org>.
zhengruifeng commented on code in PR #58278:
URL: https://github.com/apache/spark/pull/58278#discussion_r3859849365
##########
core/src/main/scala/org/apache/spark/util/collection/Utils.scala:
##########
@@ -31,6 +32,24 @@ import org.apache.spark.util.SparkCollectionUtils
*/
private[spark] object Utils extends SparkCollectionUtils {
+ /**
+ * Same function as `keys.zipWithIndex.toMap`, but uses an [[OpenHashMap]] and allows the first
+ * index to be specified.
+ */
+ def toOpenHashMapWithIndex[K: ClassTag](
+ keys: Array[K],
+ indexOffset: Int = 0): OpenHashMap[K, Int] = {
+ val map = new OpenHashMap[K, Int](keys.length)
Review Comment:
The OpenHashMap helper has been removed. The replacement Java HashMap is pre-sized with `ceil(vocabulary.length / 0.75)`, avoiding a construction-time resize for the expected number of entries.
##########
mllib/src/main/scala/org/apache/spark/ml/feature/CountVectorizer.scala:
##########
@@ -310,9 +311,9 @@ class CountVectorizerModel(
val termCounts = new OpenHashMap[Int, Int]
var tokenCount = 0L
document.foreach { term =>
- dictBr.value.get(term) match {
- case Some(index) => termCounts.changeValue(index, 1, _ + 1)
- case None => // ignore terms not in the vocabulary
+ val encodedIndex = dictBr.value(term)
Review Comment:
Fixed. The UDF now reads `dictBr.value` and `dict.size()` once at the start of each document, before the token loop. Every token lookup uses the local dictionary reference.
##########
core/src/main/scala/org/apache/spark/util/collection/Utils.scala:
##########
@@ -31,6 +32,24 @@ import org.apache.spark.util.SparkCollectionUtils
*/
private[spark] object Utils extends SparkCollectionUtils {
+ /**
+ * Same function as `keys.zipWithIndex.toMap`, but uses an [[OpenHashMap]] and allows the first
+ * index to be specified.
+ */
+ def toOpenHashMapWithIndex[K: ClassTag](
+ keys: Array[K],
+ indexOffset: Int = 0): OpenHashMap[K, Int] = {
Review Comment:
The helper and its index-offset API have been removed, so the ambiguous default and sentinel contract no longer exist.
##########
mllib/src/main/scala/org/apache/spark/ml/feature/CountVectorizer.scala:
##########
@@ -294,13 +294,14 @@ class CountVectorizerModel(
def setBinary(value: Boolean): this.type = set(binary, value)
/** Dictionary created from [[vocabulary]] and its indices, broadcast once for [[transform()]] */
- private var broadcastDict: Option[Broadcast[Map[String, Int]]] = None
+ private var broadcastDict: Option[Broadcast[OpenHashMap[String, Int]]] = None
@Since("2.0.0")
override def transform(dataset: Dataset[_]): DataFrame = {
val outputSchema = transformSchema(dataset.schema, logging = true)
if (broadcastDict.isEmpty) {
- val dict = Utils.toMapWithIndex(vocabulary)
+ // Reserve 0 for missing terms, which avoids allocating an Option for every lookup.
+ val dict = Utils.toOpenHashMapWithIndex(vocabulary, indexOffset = 1)
broadcastDict = Some(dataset.sparkSession.sparkContext.broadcast(dict))
Review Comment:
I tested both approaches. `java.util.HashMap` also needs registration with strict Kryo, so the current patch registers that single class. I also implemented the vocabulary-array plus executor-cache alternative, but it keeps both the array references and dictionary resident and adds cache/lifecycle complexity. I chose to broadcast one Java HashMap instead: its compressed payload measured 2.50 MB with JavaSerializer and 1.83 MB with Kryo, and the registration preserves `spark.kryo.registrationRequired=true`.
##########
mllib/src/main/scala/org/apache/spark/ml/feature/CountVectorizer.scala:
##########
@@ -310,9 +311,9 @@ class CountVectorizerModel(
val termCounts = new OpenHashMap[Int, Int]
var tokenCount = 0L
document.foreach { term =>
- dictBr.value.get(term) match {
- case Some(index) => termCounts.changeValue(index, 1, _ + 1)
- case None => // ignore terms not in the vocabulary
+ val encodedIndex = dictBr.value(term)
+ if (encodedIndex != 0) {
Review Comment:
Fixed by switching to `java.util.HashMap`. `get` returns a boxed Integer and null indicates a missing term, so there is no Option allocation, zero sentinel, offset encoding, or decode arithmetic.
##########
core/src/test/scala/org/apache/spark/util/collection/OpenHashMapSuite.scala:
##########
@@ -27,6 +27,14 @@ import org.apache.spark.util.SizeEstimator
class OpenHashMapSuite extends SparkFunSuite with Matchers {
+ test("build with index") {
Review Comment:
The OpenHashMap helper and its test have been removed. The patch now adds strict-Kryo coverage for Java HashMap, while CountVectorizerSuite already covers present and missing vocabulary terms; all 17 CountVectorizer tests pass.
##########
mllib/src/main/scala/org/apache/spark/ml/feature/CountVectorizer.scala:
##########
@@ -294,13 +294,14 @@ class CountVectorizerModel(
def setBinary(value: Boolean): this.type = set(binary, value)
/** Dictionary created from [[vocabulary]] and its indices, broadcast once for [[transform()]] */
- private var broadcastDict: Option[Broadcast[Map[String, Int]]] = None
+ private var broadcastDict: Option[Broadcast[OpenHashMap[String, Int]]] = None
Review Comment:
I measured this side as well for 262,144 String keys. Estimated heap was 25.6 MiB for the immutable map, 17.5 MiB for OpenHashMap, and 22.3 MiB for Java HashMap. JavaSerializer+LZ4 payloads were 3.37, 3.39, and 2.50 MB respectively; Kryo+LZ4 payloads were 2.73, 3.09, and 1.83 MB. I tried broadcasting the array and lazily caching the map, but that retains both array references and the map and adds cache lifecycle complexity. I therefore chose a single broadcast Java HashMap, which is smaller than the old map in heap and compressed wire size.
##########
core/src/main/scala/org/apache/spark/util/collection/Utils.scala:
##########
@@ -31,6 +32,24 @@ import org.apache.spark.util.SparkCollectionUtils
*/
private[spark] object Utils extends SparkCollectionUtils {
+ /**
+ * Same function as `keys.zipWithIndex.toMap`, but uses an [[OpenHashMap]] and allows the first
+ * index to be specified.
+ */
+ def toOpenHashMapWithIndex[K: ClassTag](
Review Comment:
The helper has been removed, so there is no new placement or reuse API in this patch. I summarized separately that existing OpenHashMap/OpenHashSet call sites are worth revisiting per workload; String-key insert/lookup favors JDK collections, while primitive specialization and aggregation may still favor the Spark collections.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-58993][ML] Use Java HashMap for CountVectorizer vocabulary lookup [spark]
Posted by "zhengruifeng (via GitHub)" <gi...@apache.org>.
zhengruifeng commented on code in PR #58278:
URL: https://github.com/apache/spark/pull/58278#discussion_r3859637222
##########
mllib/src/main/scala/org/apache/spark/ml/feature/CountVectorizer.scala:
##########
@@ -294,13 +294,14 @@ class CountVectorizerModel(
def setBinary(value: Boolean): this.type = set(binary, value)
/** Dictionary created from [[vocabulary]] and its indices, broadcast once for [[transform()]] */
- private var broadcastDict: Option[Broadcast[Map[String, Int]]] = None
+ private var broadcastDict: Option[Broadcast[OpenHashMap[String, Int]]] = None
@Since("2.0.0")
override def transform(dataset: Dataset[_]): DataFrame = {
val outputSchema = transformSchema(dataset.schema, logging = true)
if (broadcastDict.isEmpty) {
- val dict = Utils.toMapWithIndex(vocabulary)
+ // Reserve 0 for missing terms, which avoids allocating an Option for every lookup.
+ val dict = Utils.toOpenHashMapWithIndex(vocabulary, indexOffset = 1)
broadcastDict = Some(dataset.sparkSession.sparkContext.broadcast(dict))
Review Comment:
Applied in 103f82da6cc. CountVectorizer now broadcasts only a cloned `Array[String]` and builds the pre-sized Java HashMap lazily through an executor-local weak cache keyed by the broadcast array identity. Concurrent tasks share the map, and the weak key lets it be reclaimed with the broadcast value. This also removes the core Kryo registration change because `Array[String]` already works with `spark.kryo.registrationRequired=true`.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-58993][ML] Use Java HashMap for CountVectorizer vocabulary lookup [spark]
Posted by "zhengruifeng (via GitHub)" <gi...@apache.org>.
zhengruifeng commented on PR #58278:
URL: https://github.com/apache/spark/pull/58278#issuecomment-5422630542
**Merge Summary:**
- merged into master https://github.com/apache/spark/commit/ed8dbf4c8858ba542985a8ccbcac0a74282a7fc8
- merged into branch-4.x https://github.com/apache/spark/commit/eccb8faffcbd25cc8b4dfcac8a7b3ca215c0583b
*Posted by `merge_spark_pr.py`*
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-58993][ML] Use Java HashMap for CountVectorizer vocabulary lookup [spark]
Posted by "zhengruifeng (via GitHub)" <gi...@apache.org>.
zhengruifeng commented on code in PR #58278:
URL: https://github.com/apache/spark/pull/58278#discussion_r3859823387
##########
mllib/src/main/scala/org/apache/spark/ml/feature/CountVectorizer.scala:
##########
@@ -294,13 +294,14 @@ class CountVectorizerModel(
def setBinary(value: Boolean): this.type = set(binary, value)
/** Dictionary created from [[vocabulary]] and its indices, broadcast once for [[transform()]] */
- private var broadcastDict: Option[Broadcast[Map[String, Int]]] = None
+ private var broadcastDict: Option[Broadcast[OpenHashMap[String, Int]]] = None
@Since("2.0.0")
override def transform(dataset: Dataset[_]): DataFrame = {
val outputSchema = transformSchema(dataset.schema, logging = true)
if (broadcastDict.isEmpty) {
- val dict = Utils.toMapWithIndex(vocabulary)
+ // Reserve 0 for missing terms, which avoids allocating an Option for every lookup.
+ val dict = Utils.toOpenHashMapWithIndex(vocabulary, indexOffset = 1)
broadcastDict = Some(dataset.sparkSession.sparkContext.broadcast(dict))
Review Comment:
Follow-up: I reverted the executor-lazy vocabulary-array change in 6fc985d7c6c. The PR is back to broadcasting the Java HashMap directly with the strict-Kryo registration.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-58993][ML] Use OpenHashMap for CountVectorizer vocabulary lookup [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #58278:
URL: https://github.com/apache/spark/pull/58278#discussion_r3855707689
##########
mllib/src/main/scala/org/apache/spark/ml/feature/CountVectorizer.scala:
##########
@@ -294,13 +294,14 @@ class CountVectorizerModel(
def setBinary(value: Boolean): this.type = set(binary, value)
/** Dictionary created from [[vocabulary]] and its indices, broadcast once for [[transform()]] */
- private var broadcastDict: Option[Broadcast[Map[String, Int]]] = None
+ private var broadcastDict: Option[Broadcast[OpenHashMap[String, Int]]] = None
@Since("2.0.0")
override def transform(dataset: Dataset[_]): DataFrame = {
val outputSchema = transformSchema(dataset.schema, logging = true)
if (broadcastDict.isEmpty) {
- val dict = Utils.toMapWithIndex(vocabulary)
+ // Reserve 0 for missing terms, which avoids allocating an Option for every lookup.
+ val dict = Utils.toOpenHashMapWithIndex(vocabulary, indexOffset = 1)
broadcastDict = Some(dataset.sparkSession.sparkContext.broadcast(dict))
Review Comment:
This changes the broadcast value from `scala.collection.immutable.Map` (registered by chill's `AllScalaRegistrar`) to `OpenHashMap`, which isn't registered in `KryoSerializer` (only `BitSet` from this package is). Broadcast values are serialized with the configured `spark.serializer`, so with `spark.serializer=KryoSerializer` and `spark.kryo.registrationRequired=true`, `transform` now fails with `IllegalArgumentException: Class is not registered: org.apache.spark.util.collection.OpenHashMap$mcI$sp` where it previously worked.
Could we either register `OpenHashMap`/`OpenHashSet`/`Hasher` (including the specialized variants) in `KryoSerializer`, or keep the wire format entries-only (e.g. broadcast the `vocabulary` array and build the map lazily per executor)?
##########
mllib/src/main/scala/org/apache/spark/ml/feature/CountVectorizer.scala:
##########
@@ -310,9 +311,9 @@ class CountVectorizerModel(
val termCounts = new OpenHashMap[Int, Int]
var tokenCount = 0L
document.foreach { term =>
- dictBr.value.get(term) match {
- case Some(index) => termCounts.changeValue(index, 1, _ + 1)
- case None => // ignore terms not in the vocabulary
+ val encodedIndex = dictBr.value(term)
Review Comment:
`dictBr.value` is called once per token, and `TorrentBroadcast.getValue()` is `synchronized` with a `Reference` dereference on every call - a 1,000-token document does 1,001 synchronized calls per row (including `Vectors.sparse(dictBr.value.size, ...)` below). That per-token cost is plausibly comparable to the `Option` allocation this PR eliminates.
Hoisting `val dict = dictBr.value` (and the size) into a local at the top of the udf lambda, before the loop, keeps the win this PR is after.
##########
mllib/src/main/scala/org/apache/spark/ml/feature/CountVectorizer.scala:
##########
@@ -310,9 +311,9 @@ class CountVectorizerModel(
val termCounts = new OpenHashMap[Int, Int]
var tokenCount = 0L
document.foreach { term =>
- dictBr.value.get(term) match {
- case Some(index) => termCounts.changeValue(index, 1, _ + 1)
- case None => // ignore terms not in the vocabulary
+ val encodedIndex = dictBr.value(term)
+ if (encodedIndex != 0) {
Review Comment:
This makes `OpenHashMap.apply`'s missing-key-returns-0 behavior load-bearing across a module boundary - the exact caveat `OpenHashMap`'s class doc warns about - with the `+1` encode in `Utils` and the `!= 0` / `- 1` decode here.
An Option-free primitive on `OpenHashMap` itself (e.g. a `getOrElse(k, default)` doing a single specialized probe) would give the same performance without the sentinel arithmetic, and would also serve `StringIndexerModel.transform`, which has the same per-token `get(...).getOrElse(...)` pattern but needs a real default index - so the `+1` trick doesn't generalize to it.
##########
mllib/src/main/scala/org/apache/spark/ml/feature/CountVectorizer.scala:
##########
@@ -294,13 +294,14 @@ class CountVectorizerModel(
def setBinary(value: Boolean): this.type = set(binary, value)
/** Dictionary created from [[vocabulary]] and its indices, broadcast once for [[transform()]] */
- private var broadcastDict: Option[Broadcast[Map[String, Int]]] = None
+ private var broadcastDict: Option[Broadcast[OpenHashMap[String, Int]]] = None
Review Comment:
Worth noting: `OpenHashMap` Java-serializes its full power-of-2 capacity arrays (`_data`, `_values`, `BitSet`) - 1.4x-2.9x the entry count in slots - while the immutable `Map` serialized entries only, so the broadcast payload and per-executor footprint grow. The microbenchmark covers lookup cost but not this side.
Broadcasting the compact `vocabulary` array and building the map lazily once per executor would keep the wire format entries-only (and would also sidestep the Kryo registration issue above).
##########
core/src/test/scala/org/apache/spark/util/collection/OpenHashMapSuite.scala:
##########
@@ -27,6 +27,14 @@ import org.apache.spark.util.SizeEstimator
class OpenHashMapSuite extends SparkFunSuite with Matchers {
+ test("build with index") {
Review Comment:
This exercises `Utils.toOpenHashMapWithIndex` rather than `OpenHashMap` itself, and only the `indexOffset = 1` path - the default offset, empty array, and duplicate-key (last index wins) cases are untested, while the sibling `toMapWithIndex` covers all of these in `SparkCollectionUtilsSuite`. Could we mirror those cases here (or in a `Utils`-focused suite)?
##########
core/src/main/scala/org/apache/spark/util/collection/Utils.scala:
##########
@@ -31,6 +32,24 @@ import org.apache.spark.util.SparkCollectionUtils
*/
private[spark] object Utils extends SparkCollectionUtils {
+ /**
+ * Same function as `keys.zipWithIndex.toMap`, but uses an [[OpenHashMap]] and allows the first
+ * index to be specified.
+ */
+ def toOpenHashMapWithIndex[K: ClassTag](
+ keys: Array[K],
+ indexOffset: Int = 0): OpenHashMap[K, Int] = {
+ val map = new OpenHashMap[K, Int](keys.length)
Review Comment:
Passing `keys.length` doesn't actually reserve room for `keys.length` entries: `OpenHashSet` sets capacity to `nextPowerOf2(initialCapacity)` with a 0.7 grow threshold, so whenever `keys.length > 0.7 * nextPowerOf2(keys.length)` - every power-of-2 size, including `CountVectorizer`'s default `vocabSize = 1 << 18` - the build loop still triggers a full mid-build rehash of everything inserted so far. Sizing by the load factor, e.g. `(keys.length / 0.7).toInt + 1`, removes the rehash and ends at the same final capacity.
Also note `OpenHashSet` caps capacity at `1 << 30` (~751M entries effective), a limit the old immutable `Map` didn't have - theoretical for a vocabulary, but a behavior difference vs `toMapWithIndex`.
##########
core/src/main/scala/org/apache/spark/util/collection/Utils.scala:
##########
@@ -31,6 +32,24 @@ import org.apache.spark.util.SparkCollectionUtils
*/
private[spark] object Utils extends SparkCollectionUtils {
+ /**
+ * Same function as `keys.zipWithIndex.toMap`, but uses an [[OpenHashMap]] and allows the first
+ * index to be specified.
+ */
+ def toOpenHashMapWithIndex[K: ClassTag](
+ keys: Array[K],
+ indexOffset: Int = 0): OpenHashMap[K, Int] = {
Review Comment:
The default `indexOffset = 0` conflicts with the missing-key sentinel the only caller relies on: `apply` returns 0 for absent keys, so a future caller using the default and copying the `if (map(k) != 0)` pattern would silently treat the first key as absent. Nothing exercises the default today (the only caller and the only test both pass 1).
Since the scaladoc doesn't mention the offset or the reserve-0 contract (and the "Same function as `keys.zipWithIndex.toMap`" sentence is only accurate for offset 0), could we document that contract here - or drop the default so callers must choose? (Nit: the closing `*/` is indented one space short.)
##########
core/src/main/scala/org/apache/spark/util/collection/Utils.scala:
##########
@@ -31,6 +32,24 @@ import org.apache.spark.util.SparkCollectionUtils
*/
private[spark] object Utils extends SparkCollectionUtils {
+ /**
+ * Same function as `keys.zipWithIndex.toMap`, but uses an [[OpenHashMap]] and allows the first
+ * index to be specified.
+ */
+ def toOpenHashMapWithIndex[K: ClassTag](
Review Comment:
Placement/reuse: the map-builder helpers (`toMap`, `toJavaMap`, plus the inherited `toMapWithIndex`) are grouped at the bottom of this file, so this method probably belongs next to them rather than at the top.
Also, `StringIndexerModel.transform` and `ALS.makeBlocks` already hand-roll this exact loop (`new OpenHashMap(xs.length)` + positional `update`); migrating them to this helper would give it (and the offset-0 default) real users and keep one implementation to fix when e.g. the pre-sizing changes.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org