Skip to content

[SPARK-16931][PYTHON] PySpark APIS for bucketBy and sortBy #14517

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
wants to merge 2 commits into from
Closed

[SPARK-16931][PYTHON] PySpark APIS for bucketBy and sortBy #14517

wants to merge 2 commits into from

Conversation

@GregBowyer
Copy link

@GregBowyer GregBowyer commented Aug 6, 2016

What changes were proposed in this pull request?

API access to allow pyspark to use bucketBy and sortBy in datraframes.

MLnick
MLnick reviewed Aug 8, 2016
View reviewed changes
python/pyspark/sql/readwriter.py Outdated
self._jwrite = self._jwrite.partitionBy(_to_seq(self._spark._sc, cols))
return self

@since(2.0)
Copy link
Contributor

@MLnick MLnick Aug 8, 2016

Choose a reason for hiding this comment

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

since should be 2.1 since I don't think this will go into branch-2.0

@MLnick
Copy link
Contributor

MLnick commented Aug 8, 2016

ok to test

@SparkQA
Copy link

SparkQA commented Aug 8, 2016

Test build #63364 has finished for PR 14517 at commit 47d9ef7.

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

@GregBowyer
Copy link
Author

GregBowyer commented Aug 9, 2016

Amended commit with style changes from MLNick. Can someone call the OK to test please

@SparkQA
Copy link

SparkQA commented Aug 9, 2016

Test build #63411 has finished for PR 14517 at commit 0bc078a.

  • This patch fails PySpark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Aug 9, 2016

Test build #63414 has finished for PR 14517 at commit 96df186.

  • This patch fails PySpark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Aug 9, 2016

Test build #63422 has finished for PR 14517 at commit 31c43e6.

  • This patch fails PySpark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Aug 14, 2016

Test build #63738 has finished for PR 14517 at commit ce9f9c0.

  • This patch fails PySpark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Aug 14, 2016

Test build #63739 has finished for PR 14517 at commit 68cf597.

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

holdenk
holdenk reviewed Aug 16, 2016
View reviewed changes
python/pyspark/sql/readwriter.py Outdated
Copy link
Contributor

@holdenk holdenk Aug 16, 2016

Choose a reason for hiding this comment

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

pep8 is saying this line is too long (over 100 chars) so its failing the style tests. You can run the style tests locally with ./dev/lint-python as well for a faster turn around than Jenkins :)

Copy link
Author

@GregBowyer GregBowyer Aug 16, 2016

Choose a reason for hiding this comment

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

Thanks for the note, I was getting annoyed at not knowing where to find the tools for such things.

Copy link
Contributor

@holdenk holdenk Aug 17, 2016

Choose a reason for hiding this comment

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

Sure thing - I'm always happy to help people get up to speed with contributing to PySpark so feel free to reach out to me if you get stuck with something similar.

@SparkQA
Copy link

SparkQA commented Aug 19, 2016

Test build #64097 has finished for PR 14517 at commit 78a7b63.

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

@SparkQA
Copy link

SparkQA commented Aug 19, 2016

Test build #64106 has finished for PR 14517 at commit 8e8ce75.

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

@SparkQA
Copy link

SparkQA commented Aug 20, 2016

Test build #64113 has finished for PR 14517 at commit dfef36b.

  • This patch fails PySpark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Aug 22, 2016

Test build #64227 has finished for PR 14517 at commit 2e8c191.

  • This patch fails PySpark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Aug 22, 2016

Test build #64229 has finished for PR 14517 at commit 8eb8e71.

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

@SparkQA
Copy link

SparkQA commented Aug 22, 2016

Test build #64240 has finished for PR 14517 at commit dbb50ad.

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

@GregBowyer
Copy link
Author

GregBowyer commented Aug 26, 2016

What thoughts do people have about merging in?

davies
davies reviewed Sep 2, 2016
View reviewed changes
python/pyspark/sql/readwriter.py
except py4j.protocol.Py4JError:
spark = SparkSession(sc)

seed = int(time() * 1000)
Copy link
Contributor

@davies davies Sep 2, 2016

Choose a reason for hiding this comment

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

It's better to have determistic test, testing with parquet should be enough.

Copy link
Contributor

@davies davies Oct 3, 2016

Choose a reason for hiding this comment

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

@GregBowyer ping

Copy link
Author

@GregBowyer GregBowyer Oct 4, 2016

Choose a reason for hiding this comment

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

I have been really busy with work of late, but I will try to sort this out today

Copy link
Member

@HyukjinKwon HyukjinKwon Nov 29, 2016

Choose a reason for hiding this comment

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

@GregBowyer ping

Copy link
Member

@zero323 zero323 Jan 24, 2017

Choose a reason for hiding this comment

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

@GregBowyer Any progress on this? :)

Copy link
Member

@HyukjinKwon HyukjinKwon Feb 9, 2017

Choose a reason for hiding this comment

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

@GregBowyer ping. Let me propose to close this after a week.

Copy link
Member

@HyukjinKwon HyukjinKwon Feb 27, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

cc @zero323, would you maybe be interested in taking over this? I was thinking of taking over this if no one goes for it assuming it looks quite close to be merged.

Copy link
Member

@zero323 zero323 Feb 27, 2017

Choose a reason for hiding this comment

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

@HyukjinKwon By all means. I prepared a bunch of tests (7d911c647f21ada7fb429fd7c1c5f15934ff8847) and extended a bit code provided by @GregBowyer (72c04a3f196da5223ebb44725aa88cffa81036e4). I think we can skip low level tests (direct access to the files) which are already present in Scala test base.

Copy link
Member

@HyukjinKwon HyukjinKwon Feb 27, 2017

Choose a reason for hiding this comment

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

@zero323, Good to know. Then, please go ahead if you are ready :).

@HyukjinKwon HyukjinKwon mentioned this pull request Feb 15, 2017
Closed
@asfgit asfgit closed this in ed338f7 Feb 17, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zero323 zero323 zero323 left review comments

@HyukjinKwon HyukjinKwon HyukjinKwon left review comments

+1 more reviewer

@davies davies davies 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.

7 participants

@GregBowyer @MLnick @SparkQA @davies @holdenk @zero323 @HyukjinKwon