Skip to content

[SPARK-23928][SQL][WIP] Add shuffle collection function. #21386

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
huizhilu wants to merge 1 commit into from
Closed

[SPARK-23928][SQL][WIP] Add shuffle collection function. #21386

huizhilu wants to merge 1 commit into from

Conversation

@huizhilu
Copy link
Contributor

@huizhilu huizhilu commented May 21, 2018

What changes were proposed in this pull request?

This PR adds a new collection function: shuffle. It generates a random permutation of the given array. This implementation uses the modern version of Fisher-Yates algorithm.

How was this patch tested?

New tests are added to CollectionExpressionsSuite.scala and DataFrameFunctionsSuite.scala.

@huizhilu
Copy link
Contributor Author

huizhilu commented May 21, 2018

For the tests, I was trying to do this:

  1. assertEqualsInorgeOrder(shuffle(originSeq), originSeq)
    But spark does not have assertEqualsInorgeOrder implemented. I was thinking to check Multiset of shuffle(originSeq) and originSeq. But had trouble using Multiset for expression and seq.
  2. About the randomness, I was thinking to generate a Seq range(1, 501) and shuffle it 30 times. And it should produce at least 80% distinct permutations. Say using HashSet.add(shuffledResult). But I don't know how to implement this idea in scala and codeGen for expressions.

This is my 1st time contributing to spark and codeGen. I hope committers and contributors could help with tests, and also the shuffle function code. Thanks a lot!

mn-mikke
mn-mikke reviewed May 21, 2018
View reviewed changes
python/pyspark/sql/functions.py Outdated
Copy link
Contributor

@mn-mikke mn-mikke May 21, 2018

Choose a reason for hiding this comment

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

Generates / Returns

...catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/collectionOperations.scala Outdated
Copy link
Contributor

@mn-mikke mn-mikke May 21, 2018

Choose a reason for hiding this comment

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

Isn't ExpectsInputTypes sufficient in this case?

Copy link
Contributor Author

@huizhilu huizhilu May 22, 2018

Choose a reason for hiding this comment

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

Correct. Input is an Array. No string for input. Fixed.

...catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/collectionOperations.scala Outdated
Copy link
Contributor

@mn-mikke mn-mikke May 21, 2018

Choose a reason for hiding this comment

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

line wrapping

Copy link
Contributor Author

@huizhilu huizhilu May 21, 2018

Choose a reason for hiding this comment

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

Good catch. I must hit the enter button by mistake...

...catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/collectionOperations.scala Outdated
Copy link
Contributor

@mn-mikke mn-mikke May 21, 2018

Choose a reason for hiding this comment

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

I see strong similarities with reverse function. Would it be possible to separate common code into a new trait or class and subsequently reference it?

Copy link
Contributor Author

@huizhilu huizhilu May 21, 2018

Choose a reason for hiding this comment

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

Yes, I was thinking about it. But I did not want to change the code in reverse function without reviewer's comments. Will do it.

python/pyspark/sql/functions.py Outdated
Copy link
Contributor

@mn-mikke mn-mikke May 21, 2018

Choose a reason for hiding this comment

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

This will trigger a python test. Won't it fail if it's random?

Copy link
Contributor Author

@huizhilu huizhilu May 21, 2018

Choose a reason for hiding this comment

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

Cool. My bad. Not familiar with this. Thought they were just doc like comments... Will fix it.

@huizhilu
Copy link
Contributor Author

huizhilu commented May 21, 2018

@mn-mikke Can you give some comments on the tests code? I believe they need to be improved, using functions like Multiset? Thanks.

ueshin
ueshin reviewed May 31, 2018
View reviewed changes
...catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/collectionOperations.scala Outdated
Copy link
Member

@ueshin ueshin May 31, 2018

Choose a reason for hiding this comment

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

nit: revert unrelated change.

...catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/collectionOperations.scala Outdated
Copy link
Member

@ueshin ueshin May 31, 2018

Choose a reason for hiding this comment

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

child.nullable?
We don't need to override this in that case because it's the same in UnaryExpression.

...catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/collectionOperations.scala Outdated
Copy link
Member

@ueshin ueshin May 31, 2018

Choose a reason for hiding this comment

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

I guess this should extend Stateful trait and implement the related methods. Uuid and its test MiscExpressionsSuite should be the reference of the implementation to handle randomness.

Copy link
Contributor Author

@huizhilu huizhilu May 31, 2018

Choose a reason for hiding this comment

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

Thanks a lot, Takuya @ueshin ! This is a good tip! Working on it and will push a new commit.

@ueshin
Copy link
Member

ueshin commented May 31, 2018

Thanks for your contribution!
You can refer Uuid and its test as I mentioned in a comment. Could you update this?
I'll revisit when you push new commits.

@ueshin
Copy link
Member

ueshin commented May 31, 2018

@pkuwm Btw, if you want to implement some algorithm like "Fisher-Yates algorithm" by hand, please add a comment near the code, and a link you referred hopefully.

@AmplabJenkins
Copy link

AmplabJenkins commented Jun 9, 2018

Can one of the admins verify this patch?

@huizhilu
Copy link
Contributor Author

huizhilu commented Jun 9, 2018

I learned more of the code and am polishing my second commit. I had something else to do and also attended the spark summit this week. Sorry for being late. Will submit a new commit over the weekend.

@ueshin
Copy link
Member

ueshin commented Jul 13, 2018

@pkuwm Hi, any updates on this? If you have any questions, please let us know. Thanks!

@huizhilu huizhilu force-pushed the SPARK-23928 branch 3 times, most recently from a8a36ef to 70cd78c Compare July 17, 2018 23:34
@huizhilu
Copy link
Contributor Author

huizhilu commented Jul 17, 2018

@ueshin Really sorry for the delay. I was handling some personal stuff recently and was not able to modify this patch as I am not really familiar with this part.
I updated the commit and fixed line wraps, typos, replacing the shuffle tests in functions.py with a note.

About the similarities of Reverse and Shuffle, I was trying to implement a trait, but did not have a good idea because the code is not the same.
And not sure if Stateful would be a good fit for this function.

Can you help? If you have better idea, maybe you can continue completing this implementation. Thanks a lot!

@ueshin
Copy link
Member

ueshin commented Jul 18, 2018

Okay, I'll take this over, and ping you when I submit a PR to ask a review. Thanks!

@ueshin
Copy link
Member

ueshin commented Jul 18, 2018

@pkuwm I submitted a PR #21802 based on this. Could you take a look if you have time? Thanks!

@dongjoon-hyun
Copy link
Member

dongjoon-hyun commented Sep 13, 2018

@pkuwm . Could you close this PR since #21802 is merged?

@huizhilu
Copy link
Contributor Author

huizhilu commented Sep 13, 2018

Thanks for reminding, @dongjoon-hyun

@huizhilu huizhilu closed this Sep 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ueshin ueshin ueshin left review comments

+1 more reviewer

@mn-mikke mn-mikke mn-mikke left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@huizhilu @ueshin @AmplabJenkins @dongjoon-hyun @mn-mikke