[SPARK-11050][MLLIB] PySpark SparseVector can return wrong index in e…

[bhargav]

…rror message

For negative indices in the SparseVector, we update the index value. If we have an incorrect index
at this point, the error message has the incorrect updated index instead of the original one. This
change contains the fix for the same.

[JoshRosen]

I've been seeing a lot of bugfix patches for the PySpark SparseVector recently. This suggests to me that we need to write significantly more tests for this component.

[bhargav]

@jkbradley for this.

If there is a task for adding more tests, I can take that up as well.

[JoshRosen]

It'd be really fun to try using Hypothesis to write some property-based tests for this: https://hypothesis.readthedocs.org/en/master/

[jkbradley]

@JoshRosen I think it's that PySpark has less coverage than Scala, and our linear algebra code could use some more coverage too. And PySpark SparseVector is in the intersection of these problems. (Though 4 of those bug fix patches may have been patch + 3 backports.)

Hypothesis looks cool...future work? : )

@bhargav There is not a specific task for adding more tests, but since you're interested, it'd be awesome if you could check through some of the PySpark Vector and Matrix APIs and unit tests and see if you can find missing coverage. Note also that the tests are split between doc tests (in each .py file) and unit tests (in tests.py files). If you find missing items, can you please make one or more JIRAs? If you're unsure about any, I'd recommend making a single JIRA and listing there; we can then create subtasks for each major issue. Thanks!

[jkbradley]

It might be better to move this check to line 768, before index gets modified. That way, you don't have to even create a copy of index (though you'll need to adjust the check's index range).

[SparkQA]

Test build #1882 has finished for PR 9069 at commit 316acac.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds the following public classes (experimental):
  • class ChildProcAppHandle implements SparkAppHandle
  • abstract class LauncherConnection implements Closeable, Runnable
  • final class LauncherProtocol
  • static class Message implements Serializable
  • static class Hello extends Message
  • static class SetAppId extends Message
  • static class SetState extends Message
  • static class Stop extends Message
  • class LauncherServer implements Closeable
  • class NamedThreadFactory implements ThreadFactory
  • class OutputRedirector
  • public final class UnsafeRow extends MutableRow implements Externalizable, KryoSerializable
  • /** Run a function within Hive state (SessionState, HiveConf, Hive client and class loader) */

[bhargav]

Updated the PR. Though I now see that the branch name is different from the JIRA issue number.

[bhargav]

@jkbradley Gentle ping. :)

[jkbradley]

@bhargav Thanks for the update! LGTM pending tests

[jkbradley]

Btw, it doesn't matter what you call your branch name; that's not a problem.

[SparkQA]

Test build #1916 has finished for PR 9069 at commit 19bc764.

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

[jkbradley]

Merging with master