[SPARK-18230][MLLib]Throw a better exception, if the user or product doesn't exist

[shahidki31]

When invoking MatrixFactorizationModel.recommendProducts(Int, Int) with a non-existing user, a java.util.NoSuchElementException is thrown:

java.util.NoSuchElementException: next on empty iterator
at scala.collection.Iterator$$anon$2.next(Iterator.scala:39)
at scala.collection.Iterator$$anon$2.next(Iterator.scala:37)
at scala.collection.IndexedSeqLike$Elements.next(IndexedSeqLike.scala:63)
at scala.collection.IterableLike$class.head(IterableLike.scala:107)
at scala.collection.mutable.WrappedArray.scala$collection$IndexedSeqOptimized$$super$head(WrappedArray.scala:35)
at scala.collection.IndexedSeqOptimized$class.head(IndexedSeqOptimized.scala:126)
at scala.collection.mutable.WrappedArray.head(WrappedArray.scala:35)
at org.apache.spark.mllib.recommendation.MatrixFactorizationModel.recommendProducts(MatrixFactorizationModel.scala:169)

What changes were proposed in this pull request?

Throw a better exception, like "user-id/product-id doesn't found in the model", for a non-existent user/product

How was this patch tested?

Added UT

[jianran]

why is the IllegalArgumentException better than SparkException?

[shahidki31]

@jianran please refer the PR, #15809. In this PR, I am checking if the 'userFeatures.lookup(user)', is empty or not.

[srowen]

Also see the JIRA for a discussion. The main reason is that NoSuchElementException doesn't really make sense semantically and has no useful message attached.

[srowen]

I don't think it's necessary to assert about the exact message string. Maybe assert it contains the user ID. But just checking for the exception also seems close enough.

[shahidki31]

Thanks for the review. Done.

[srowen]

The problem is you now look up the user and product twice. Instead, the callers should probably check whether userFeatures.lookup(user) is empty before calling .head and otherwise throw the exception.

[shahidki31]

I have renamed the method to 'validateAndGetUser', where it check, whether the user exist or not and it returns the corresponding user feature. Similarly for the product also.
Please let me know if anymore changes required.

[dmateusp]

I'm new to the code base, I understand Either and Option isn't used a lot in public APIs in Spark but shouldn't it be annotated that the functions throws a certain type of exception (being more explicit with the Exception) ?

like @throws(classOf[IllegalArgumentException]) ?

[srowen]

Returning Option could be reasonable, but, I think the caller would end up giving up with an exception anyway. There isn't an obviously better thing to do.

Exceptions all behave as if unchecked in Scala and don't get expressed in method signatures in the byte code. @throws is really for Java compatibility where it's important for the Java language to express that a checked exception is thrown (it makes a compile-time difference in Java).

That's not necessary here, but scaladoc'ing exceptions like this is indeed a good idea.

[dmateusp]

Thanks for the explanation Sean :)

[srowen]

Why the pattern match? .lookup should already be known to return a Seq of the type of the RDD's values which is already known to be Array[Double].

The name of the method isn't really accurate either; it gets a user vector, not user.

val vec = userFeatures.lookup(user)
require(vec.nonEmpty, ...)
vec.head

How about one method instead of two, that takes the RDD as an arg? might cut down on the code expansion here. I'm on the fence about whether this warrants a new method since there are only two call sites, but it's reasonable.

[shahidki31]

Thank you for the comment.
I have removed both the methods. Because each method has to give distinct exception, like "user not found" in the first method and "product not found" in the second method. So, it is better to remove both the methods, instead of making one.
I have added the validation code inside the methods such as predict, recommendProduct etc.

[shahidki31]

@srowen Please let me know if anymore changes required.

[SparkQA]

Test build #4211 has finished for PR 21740 at commit 01d43c8.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[shahidki31]

Hi @srowen. The build has passed.

[srowen]

Merged to master. Your JIRA handle is "shahid" right?

[shahidki31]

Thanks @srowen. yes, my JIRA handle is "shahid".