Skip to content

[SPARK-23261] [PySpark] Rename Pandas UDFs - #20428

Closed
gatorsmile wants to merge 4 commits into
apache:masterfrom
gatorsmile:renamePandasUDFs
Closed

[SPARK-23261] [PySpark] Rename Pandas UDFs#20428
gatorsmile wants to merge 4 commits into
apache:masterfrom
gatorsmile:renamePandasUDFs

Conversation

@gatorsmile

@gatorsmilegatorsmile commented Jan 29, 2018•
edited
Loading

Copy link
Copy Markdown
Member

What changes were proposed in this pull request?

Rename the public APIs and names of pandas udfs.

  • PANDAS SCALAR UDF -> SCALAR PANDAS UDF
  • PANDAS GROUP MAP UDF -> GROUPED MAP PANDAS UDF
  • PANDAS GROUP AGG UDF -> GROUPED AGG PANDAS UDF

How was this patch tested?

The existing tests

@gatorsmile

gatorsmile commented Jan 29, 2018•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

Had an offline discussion with @sameeragarwal and @cloud-fan . To be consistent with the other APIs, we would propose to make the above changes.

The major question is about the new name of Pandas UDAF. This might be not the best when we supporting partial aggregate.

Also cc @rxin@ueshin@HyukjinKwon@icexelloss@BryanCutler

@gatorsmilegatorsmile mentioned this pull request Jan 29, 2018
Closed
@icexelloss

icexelloss commented Jan 29, 2018

Copy link
Copy Markdown
Contributor

  • PANDAS SCALAR UDF -> SCALAR PANDAS UDF
    This doesn't really change the API so +1

  • PANDAS GROUP MAP UDF -> GROUPED MAP PANDAS UDF
    The API changes from PandasUDFType.GROUP_MAP to PandasUDFType.GROUPED_MAP. I think this is fine as well.

  • PANDAS GROUP AGG UDF -> PANDAS UDAF
    The API changes from PandasUDFType.GROUP_AGG to PandasUDFType.UDAF. I don't love this because it feels different from SCALAR and GROUPED_MAP. I would probably call it something like GROUPED_AGG. But since this type is not released in 2.3, we can always change later.

@HyukjinKwon

HyukjinKwon commented Jan 30, 2018•
edited
Loading

Copy link
Copy Markdown
Member

Yup, fortunately(?) we are free to rename SQL_PANDAS_GROUP_AGG_UDF within 2.4.0 currently but I believe here is a good place to decide based on what we got so far. The proposal seems fine to me for now.

@viirya

viirya commented Jan 30, 2018

Copy link
Copy Markdown
Member

First two changes looks good. The last one, maybe PANDAS GROUP AGG UDF -> GROUPED AGG PANDAS UDF?

@SparkQA

SparkQA commented Jan 30, 2018

Copy link
Copy Markdown

Test build #86784 has finished for PR 20428 at commit 8f75cc8.

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

@sameeragarwal

sameeragarwal commented Jan 30, 2018

Copy link
Copy Markdown
Member

+1 on GROUPED AGG as well

@HyukjinKwon

HyukjinKwon commented Jan 30, 2018

Copy link
Copy Markdown
Member

+1 on GROUPED AGG to me too, just to be clear.

@cloud-fan

cloud-fan commented Jan 30, 2018

Copy link
Copy Markdown
Contributor

+1 on GROUPED AGG too, we may add new UDF type when we support partial aggregate.

@SparkQA

SparkQA commented Jan 30, 2018

Copy link
Copy Markdown

Test build #86794 has finished for PR 20428 at commit 9a4aada.

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

@cloud-fan

cloud-fan commented Jan 30, 2018

Copy link
Copy Markdown
Contributor

LGTM

dongjoon-hyun
dongjoon-hyun approved these changes Jan 30, 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.

HyukjinKwon
HyukjinKwon approved these changes Jan 30, 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

viirya
viirya approved these changes Jan 30, 2018
View reviewed changes

@viiryaviirya 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

@viirya

viirya commented Jan 30, 2018

Copy link
Copy Markdown
Member

Let's also update PR description too.

@SparkQA

SparkQA commented Jan 30, 2018

Copy link
Copy Markdown

Test build #86798 has finished for PR 20428 at commit 7a71c5a.

  • This patch fails due to an unknown error code, -9.
  • This patch merges cleanly.
  • This patch adds no public classes.

@ueshin

ueshin commented Jan 30, 2018

Copy link
Copy Markdown
Member

Jenkins, retest this please.

@ueshin

ueshin commented Jan 30, 2018

Copy link
Copy Markdown
Member

LGTM.

@SparkQA

SparkQA commented Jan 30, 2018

Copy link
Copy Markdown

Test build #86804 has finished for PR 20428 at commit 7a71c5a.

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

@HyukjinKwon

HyukjinKwon commented Jan 30, 2018

Copy link
Copy Markdown
Member

Merged to master.

@cloud-fan

cloud-fan commented Jan 30, 2018

Copy link
Copy Markdown
Contributor

@gatorsmile shall we backport it to 2.3? excluding the GROUPED AGG.

@gatorsmile

gatorsmile commented Jan 30, 2018

Copy link
Copy Markdown
MemberAuthor

Yes. We need to backport it to 2.3

@gatorsmile

gatorsmile commented Jan 30, 2018

Copy link
Copy Markdown
MemberAuthor

Will submit a new PR to 2.3

gatorsmile added a commit to gatorsmile/spark that referenced this pull request Jan 30, 2018
@gatorsmile
[SPARK-23261][PYSPARK] Rename Pandas UDFs
1a70ae1
Rename the public APIs and names of pandas udfs.
- `PANDAS SCALAR UDF` -> `SCALAR PANDAS UDF`
- `PANDAS GROUP MAP UDF` -> `GROUPED MAP PANDAS UDF`
- `PANDAS GROUP AGG UDF` -> `GROUPED AGG PANDAS UDF`
The existing tests
Author: gatorsmile <gatorsmile@gmail.com>
Closesapache#20428 from gatorsmile/renamePandasUDFs.
@gatorsmilegatorsmile mentioned this pull request Jan 30, 2018
Closed
asfgit pushed a commit that referenced this pull request Jan 31, 2018
@gatorsmile
[SPARK-23261][PYSPARK][BACKPORT-2.3] Rename Pandas UDFs
7b9fe08
This PR is to backport #20428 to Spark 2.3 without adding the changes regarding `GROUPED AGG PANDAS UDF`
---
## What changes were proposed in this pull request?
Rename the public APIs and names of pandas udfs.
- `PANDAS SCALAR UDF` -> `SCALAR PANDAS UDF`
- `PANDAS GROUP MAP UDF` -> `GROUPED MAP PANDAS UDF`
## How was this patch tested?
The existing tests
Author: gatorsmile <gatorsmile@gmail.com>
Closes#20439 from gatorsmile/backport2.3.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@viiryaviiryaviirya approved these changes
@HyukjinKwonHyukjinKwonHyukjinKwon approved these changes
@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.

9 participants

@gatorsmile@icexelloss@HyukjinKwon@viirya@SparkQA@sameeragarwal@cloud-fan@ueshin@dongjoon-hyun