Skip to content

Conversation

@srowen
Copy link
Member

@srowen srowen commented Jul 30, 2015

Check that SparseVector size is at least as big as the number of indices/values provided. And add tests for constructor checks.

CC @MechCoder @jkbradley -- I am not sure if a change needs to also happen in the Python API? I didn't see it had any similar checks to begin with, but I don't know it well.

…ces/values provided. And add tests for constructor checks.
@mengxr
Copy link
Contributor

mengxr commented Jul 30, 2015

LGTM. I think it is useful to add the same check to Python. @MechCoder could you add it after #7746 ? Thanks!

@SparkQA
Copy link

SparkQA commented Jul 30, 2015

Test build #39046 has finished for PR 7794 at commit 6ffe34a.

  • This patch fails Scala style tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

space after ,

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh duh, fix coming. Every time I think I can't possibly need to run scalastyle as well as the test ...

@SparkQA
Copy link

SparkQA commented Jul 30, 2015

Test build #39061 has finished for PR 7794 at commit e8dc31e.

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

@asfgit asfgit closed this in ed3cb1d Jul 30, 2015
@jkbradley
Copy link
Member

@MechCoder Actually, it looks like the Python SparseVector constructor is really inefficient, requiring either (a) a full pass over the indices or (b) sorting the indices, depending on what is passed to the constructor. Would you mind making a JIRA for making the Python constructor equivalent to the Scala one? Thank you!

@srowen srowen deleted the SPARK-9277 branch July 31, 2015 07:46
@MechCoder
Copy link
Contributor

Thanks you for the ping! I'll have a deeper look now.

@MechCoder
Copy link
Contributor

please check #7854

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants