You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "ngtanvan (via GitHub)" <gi...@apache.org> on 2026/08/30 11:16:53 UTC
[PR] [SPARK-48685][ML] Fix MinHashLSH to handle empty sparse vectors without crashing [spark]
ngtanvan opened a new pull request, #58417:
URL: https://github.com/apache/spark/pull/58417
### What changes were proposed in this pull request?
Fix `MinHashLSHModel.hashFunction` to return `Array.empty[Vector]` when encountering empty sparse vectors (0 non-zero entries) instead of throwing an unhandled `IllegalArgumentException`.
### Why are the changes needed?
When pipelines combining `CountVectorizer` and `MinHashLSH` process empty sparse vectors (e.g. text containing terms outside vocabulary), `MinHashLSH` threw `java.lang.IllegalArgumentException: requirement failed: Must have at least 1 non zero entry.`, crashing the entire Spark job.
Returning an empty vector array allows `posexplode` in `approxSimilarityJoin` to cleanly skip empty vectors without crashing.
### Does this PR introduce _any_ user-facing change?
No breaking change. It fixes an unexpected crash when transforming or joining empty vectors with `MinHashLSH`.
### How was this patch tested?
Updated unit test `MinHashLSHSuite` ("hashFunction: empty vector") to verify returning empty vector array, and verified all 15 tests in `MinHashLSHSuite` pass cleanly.
--
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-48685][ML] Fix MinHashLSH to handle empty sparse vectors without crashing [spark]
Posted by "tdcmeehan (via GitHub)" <gi...@apache.org>.
tdcmeehan commented on PR #58417:
URL: https://github.com/apache/spark/pull/58417#issuecomment-5499040448
Could we follow other Spark ML transformers and add `handleInvalid` with `error` (default), `skip`, and `keep`? keep could produce the proposed empty hash array. This preserves existing behavior while letting users opt into tolerant handling, consistent with `VectorSizeHint` and `VectorAssembler`.
--
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-48685][ML] Fix MinHashLSH to handle empty sparse vectors without crashing [spark]
Posted by "ngtanvan (via GitHub)" <gi...@apache.org>.
ngtanvan commented on PR #58417:
URL: https://github.com/apache/spark/pull/58417#issuecomment-5629813372
Hi @tdcmeehan , I have implemented handleInvalid for both Scala and PySpark as you suggested, and fixed the line length issues. All CI checks are now successfully passing. Could you please take another look when you have a chance? Thank you!
--
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-48685][ML] Fix MinHashLSH to handle empty sparse vectors without crashing [spark]
Posted by "ngtanvan (via GitHub)" <gi...@apache.org>.
ngtanvan commented on PR #58417:
URL: https://github.com/apache/spark/pull/58417#issuecomment-5638663129
Hi @tdcmeehan , @HyukjinKwon , and @srowen . Following the suggestions, I have implemented the `handleInvalid` parameter in both Scala and PySpark, and fixed the line-length issues. All CI checks are now green! Could you please take a quick look when you have a chance? I am working on this for an upcoming university deadline, so a review or merge would be highly appreciated. Thanks again for your guidance!
--
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-48685][ML] Fix MinHashLSH to handle empty sparse vectors without crashing [spark]
Posted by "ngtanvan (via GitHub)" <gi...@apache.org>.
ngtanvan commented on PR #58417:
URL: https://github.com/apache/spark/pull/58417#issuecomment-5469304986
Hi @HyukjinKwon , @srowen
Could you please help take a look or trigger CI for this PR when you have a moment?
This PR fixes [SPARK-48685] where `MinHashLSHModel.hashFunction` throws an unhandled `IllegalArgumentException` when encountering empty sparse vectors. All 15 unit tests in `MinHashLSHSuite` have been updated and pass cleanly.
Thank you very much for your time and assistance!
--
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-48685][ML] Fix MinHashLSH to handle empty sparse vectors without crashing [spark]
Posted by "tdcmeehan (via GitHub)" <gi...@apache.org>.
tdcmeehan commented on code in PR #58417:
URL: https://github.com/apache/spark/pull/58417#discussion_r3906087693
##########
mllib/src/main/scala/org/apache/spark/ml/feature/MinHashLSH.scala:
##########
@@ -222,16 +222,20 @@ object MinHashLSHModel extends MLReadable[MinHashLSHModel] {
private def hashFunction(
elems: Vector,
randCoefficients: Array[(Int, Int)]): Array[Vector] = {
- require(elems.nonZeroIterator.nonEmpty, "Must have at least 1 non zero entry.")
- val hashValues = randCoefficients.map { case (a, b) =>
- elems.nonZeroIterator.map { case (i, _) =>
- ((1L + i) * a + b) % MinHashLSH.HASH_PRIME
- }.min.toDouble
+ if (elems.nonZeroIterator.isEmpty) {
+ Array.empty[Vector]
+ } else {
+ val hashValues = randCoefficients.map { case (a, b) =>
+ elems.nonZeroIterator.map { case (i, _) =>
+ ((1L + i) * a + b) % MinHashLSH.HASH_PRIME
+ }.min.toDouble
+ }
+ // TODO: Output vectors of dimension numHashFunctions in SPARK-18450
+ hashValues.map(Vectors.dense(_))
}
- // TODO: Output vectors of dimension numHashFunctions in SPARK-18450
- hashValues.map(Vectors.dense(_))
}
+
Review Comment:
Please remove the unnecessary whitespace only change.
--
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-48685][ML] Fix MinHashLSH to handle empty sparse vectors without crashing [spark]
Posted by "ngtanvan (via GitHub)" <gi...@apache.org>.
ngtanvan commented on PR #58417:
URL: https://github.com/apache/spark/pull/58417#issuecomment-5499705613
Thank you @tdcmeehan for the great suggestion! Adding `handleInvalid` (`error`, `skip`, `keep`) makes a lot of sense to maintain backward compatibility while providing flexibility consistent with other Spark ML transformers.
I will work on updating the PR to implement `HasHandleInvalid`. Thanks again for the review!
--
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-48685][ML] Fix MinHashLSH to handle empty sparse vectors without crashing [spark]
Posted by "ngtanvan (via GitHub)" <gi...@apache.org>.
ngtanvan closed pull request #58417: [SPARK-48685][ML] Fix MinHashLSH to handle empty sparse vectors without crashing
URL: https://github.com/apache/spark/pull/58417
--
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