Skip to content

[SPARK-24131][PySpark] Add majorMinorVersion API to PySpark for determining Spark versions - #21203

Closed
viirya wants to merge 1 commit into
apache:masterfrom
viirya:SPARK-24131
Closed

[SPARK-24131][PySpark] Add majorMinorVersion API to PySpark for determining Spark versions#21203
viirya wants to merge 1 commit into
apache:masterfrom
viirya:SPARK-24131

Conversation

@viirya

@viiryaviirya commented May 1, 2018

Copy link
Copy Markdown
Member

What changes were proposed in this pull request?

We need to determine Spark major and minor versions in PySpark. We can add a majorMinorVersion API to PySpark which is similar to the Scala API in VersionUtils.majorMinorVersion.

How was this patch tested?

Added tests.

@viirya

viirya commented May 1, 2018

Copy link
Copy Markdown
MemberAuthor

cc @jkbradley@HyukjinKwon@dbtsai

@viirya

viirya commented May 1, 2018

Copy link
Copy Markdown
MemberAuthor

This is moved from #21153 based on @jkbradley's suggestion.

@viirya

viirya commented May 1, 2018

Copy link
Copy Markdown
MemberAuthor

retest this please.

dongjoon-hyun
dongjoon-hyun approved these changes May 1, 2018
View reviewed changes

@dongjoon-hyundongjoon-hyun left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

+1, LGTM except one minor comment.

Comment threadpython/pyspark/util.py
>>> version = "abc"
>>> majorMinorVersion(version) is None
True

@dongjoon-hyundongjoon-hyunMay 1, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit. Could you remove redundant empty line?

@viiryaviiryaMay 1, 2018

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Yap, thanks.

felixcheung
felixcheung reviewed May 1, 2018
View reviewed changes

@felixcheungfelixcheung left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

perhaps better to match python's version_info ?
https://docs.python.org/2/library/sys.html#sys.version_info

@viirya

viirya commented May 1, 2018•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

perhaps better to match python's version_info ?

This for now is more close to VersionUtils.majorMinorVersion. To match python's version_info, means we need to have 5 components? Or just can be accessed by name?

@dongjoon-hyun

dongjoon-hyun commented May 1, 2018

Copy link
Copy Markdown
Member

Retest this please

@SparkQA

SparkQA commented May 1, 2018

Copy link
Copy Markdown

Test build #89982 has finished for PR 21203 at commit 9558ad7.

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

HyukjinKwon
HyukjinKwon approved these changes May 2, 2018
View reviewed changes

@HyukjinKwonHyukjinKwon left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm

@HyukjinKwon

HyukjinKwon commented May 2, 2018

Copy link
Copy Markdown
Member

Merged to master.

@asfgitasfgit closed this in e15850bMay 2, 2018
@viirya

viirya commented May 2, 2018

Copy link
Copy Markdown
MemberAuthor

ah, sorry, @jkbradley asked me to do a little change like throwing exception when can't paring input. My flight just lands now. Let me submit a small follow-up.

@HyukjinKwon

HyukjinKwon commented May 2, 2018

Copy link
Copy Markdown
Member

I missed that comment too. Yea, I think it makes more sense to throw an exception - I usually use ValueError in this case though. Please go ahead for a followup.

HyukjinKwon
HyukjinKwon reviewed May 2, 2018
View reviewed changes
Comment threadpython/pyspark/util.py
return argspec


def majorMinorVersion(version):

@HyukjinKwonHyukjinKwonMay 2, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Also, I think he asked to add VersionUtils class.

@HyukjinKwon

HyukjinKwon commented May 2, 2018

Copy link
Copy Markdown
Member

Sorry @jkbradley, I rushed it. It looked making sense when I merged this but I didn't closely check the Scala side. Will be careful next time.

@jkbradley

jkbradley commented May 9, 2018

Copy link
Copy Markdown
Member

It's OK but would you mind fixing it @viirya before we use it in #21153 ? Thanks!

@viirya

viirya commented May 9, 2018

Copy link
Copy Markdown
MemberAuthor

@jkbradley A follow-up has been submitted and merged. Please see #21211. Thanks.

@viirya
viirya deleted the SPARK-24131 branch December 27, 2023 18:35
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@HyukjinKwonHyukjinKwonHyukjinKwon approved these changes
@felixcheungfelixcheungfelixcheung left review comments
@dongjoon-hyundongjoon-hyundongjoon-hyun 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.

6 participants

@viirya@dongjoon-hyun@SparkQA@HyukjinKwon@jkbradley@felixcheung