Github user datumbox commented on a diff in the pull request:
https://github.com/apache/spark/pull/17059#discussion_r103557982
--- Diff: mllib/src/main/scala/org/apache/spark/ml/recommendation/ALS.scala
---
@@ -82,12 +82,20 @@ private[recommendation] trait ALSModelParams extends
Params with HasPredictionCo
* Attempts to safely cast a user/item id to an Int. Throws an exception
if the value is
* out of integer range.
*/
- protected val checkedCast = udf { (n: Double) =>
- if (n > Int.MaxValue || n < Int.MinValue) {
- throw new IllegalArgumentException(s"ALS only supports values in
Integer range for columns " +
- s"${$(userCol)} and ${$(itemCol)}. Value $n was out of Integer
range.")
- } else {
- n.toInt
+ protected val checkedCast = udf { (n: Any) =>
+ n match {
+ case v: Int => v // Avoid unnecessary casting
+ case v: Number =>
+ val intV = v.intValue()
+ // Checks if number within Int range and has no fractional part.
+ if (v.doubleValue == intV) {
--- End diff --
@imatiach-msft I already tested it, check this snippet posted
[here](https://github.com/apache/spark/pull/17059#discussion_r103554415). I can
add it in ALSSuite if necessary. The exception message is technically correct
since a number with a fractional part is not in Integer range (plus you get to
see the actual value of the number). Returning a different exception would
require having two separate if checks instead of one. Do we really want that?
Guys to be honest this PR solves a very simple thing and I did not
anticipate to be so controvercial. May I suggest we agree on the final set of
changes and merge it? Perhaps we can tackle any other concerns on new
pull-requests?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
