Skip to content
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

[SPARK-17808][PYSPARK] Upgraded version of Pyrolite to 4.13 #15386

Closed
wants to merge 4 commits into from
Closed

[SPARK-17808][PYSPARK] Upgraded version of Pyrolite to 4.13 #15386

wants to merge 4 commits into from

Conversation

BryanCutler
Copy link
Member

What changes were proposed in this pull request?

Upgraded to a newer version of Pyrolite which supports serialization of a BinaryType StructField for PySpark.SQL

How was this patch tested?

Added a unit test which fails with a raised ValueError when using the previous version of Pyrolite 4.9 and Python3

@SparkQA
Copy link

SparkQA commented Oct 7, 2016

Test build #66474 has finished for PR 15386 at commit a3468f4.

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

holdenk
holdenk reviewed Oct 7, 2016
View reviewed changes
python/pyspark/sql/tests.py
# Pyrolite version <= 4.9 could not serialize BinaryType with Python3 SPARK-17808
schema = StructType([StructField('mybytes', BinaryType())])
data = [[bytearray(b'here is my data')],
[bytearray(b'and here is some more')],]
Copy link
Contributor

Choose a reason for hiding this comment

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

linter error here with the ','

Copy link
Member Author

Choose a reason for hiding this comment

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

ooops, thought I ran local style checks, but maybe it was for scala :(

@holdenk
Copy link
Contributor

holdenk commented Oct 7, 2016

Thanks for working on this - the pylint script found a style problem (PEP8 checks failed.
./python/pyspark/sql/tests.py:1709:54: E231 missing whitespace after ',') - if you want to test the style locally first you can use ./dev/lint-python

srowen
srowen approved these changes Oct 7, 2016
View reviewed changes
Copy link
Member

@srowen srowen left a comment

Choose a reason for hiding this comment

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

I skimmed the changes from 4.9 and didn't see anything that seemed like it was worth investigating. Looks OK for master.

@BryanCutler
Copy link
Member Author

Thanks @holdenk and @srowen for looking into this and reviewing!

@SparkQA
Copy link

SparkQA commented Oct 7, 2016

Test build #66509 has finished for PR 15386 at commit 612a51e.

  • This patch fails build dependency tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@srowen
Copy link
Member

srowen commented Oct 7, 2016

Yeah I figured you'd need to run ./dev/test-dependencies.sh --replace-manifest here

@srowen
Copy link
Member

srowen commented Oct 10, 2016

@BryanCutler if you'll update accordingly I'll merge it.

@SparkQA
Copy link

SparkQA commented Oct 10, 2016

Test build #3312 has finished for PR 15386 at commit 612a51e.

  • This patch fails build dependency tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@BryanCutler
Copy link
Member Author

Thanks @srowen , I wasn't aware of that script. Hopefully this should be good to go now!

@SparkQA
Copy link

SparkQA commented Oct 10, 2016

Test build #66666 has finished for PR 15386 at commit 6259438.

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

@BryanCutler
Copy link
Member Author

Hmm, I don't think there should be a MiMa issue right? It passes locally anyway.. let me try again

@BryanCutler
Copy link
Member Author

Jenkins retest this please

@SparkQA
Copy link

SparkQA commented Oct 10, 2016

Test build #66672 has finished for PR 15386 at commit 6259438.

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

@BryanCutler
Copy link
Member Author

retest this please

@SparkQA
Copy link

SparkQA commented Oct 10, 2016

Test build #66683 has finished for PR 15386 at commit 6259438.

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

@srowen
Copy link
Member

srowen commented Oct 11, 2016

Merged to master

@asfgit asfgit closed this in 658c714 Oct 11, 2016
@srowen
Copy link
Member

srowen commented Oct 12, 2016

Merged to 2.0 too

asfgit pushed a commit that referenced this pull request Oct 12, 2016
@BryanCutler @srowen
[SPARK-17808][PYSPARK] Upgraded version of Pyrolite to 4.13
4dcbde4 
## What changes were proposed in this pull request?
Upgraded to a newer version of Pyrolite which supports serialization of a BinaryType StructField for PySpark.SQL

## How was this patch tested?
Added a unit test which fails with a raised ValueError when using the previous version of Pyrolite 4.9 and Python3

Author: Bryan Cutler <cutlerb@gmail.com>

Closes #15386 from BryanCutler/pyrolite-upgrade-SPARK-17808.

(cherry picked from commit 658c714)
Signed-off-by: Sean Owen <sowen@cloudera.com>
@BryanCutler BryanCutler deleted the pyrolite-upgrade-SPARK-17808 branch December 2, 2016 01:01
uzadude pushed a commit to uzadude/spark that referenced this pull request Jan 27, 2017
@BryanCutler @uzadude
[SPARK-17808][PYSPARK] Upgraded version of Pyrolite to 4.13
8828ab1 
## What changes were proposed in this pull request?
Upgraded to a newer version of Pyrolite which supports serialization of a BinaryType StructField for PySpark.SQL

## How was this patch tested?
Added a unit test which fails with a raised ValueError when using the previous version of Pyrolite 4.9 and Python3

Author: Bryan Cutler <cutlerb@gmail.com>

Closes apache#15386 from BryanCutler/pyrolite-upgrade-SPARK-17808.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@holdenk holdenk holdenk left review comments

@srowen srowen srowen approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@BryanCutler @SparkQA @holdenk @srowen