Skip to content

[SPARK-28321][SQL] 0-args Java UDF should not be called only once #25108

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 1 commit into from
Closed

[SPARK-28321][SQL] 0-args Java UDF should not be called only once #25108

wants to merge 1 commit into from

Conversation

@HyukjinKwon
Copy link
Member

@HyukjinKwon HyukjinKwon commented Jul 11, 2019 •
edited
Loading

What changes were proposed in this pull request?

0-args Java UDF alone calls the function even before making it as an expression.
It causes that the function always returns the same value and the function is called at driver side.
Seems like a mistake.

How was this patch tested?

Unit test was added

@HyukjinKwon
Copy link
Member Author

HyukjinKwon commented Jul 11, 2019

cc @cloud-fan

@cloud-fan
Copy link
Contributor

cloud-fan commented Jul 11, 2019

good catch! LGTM

@SparkQA
Copy link

SparkQA commented Jul 11, 2019

Test build #107507 has finished for PR 25108 at commit c725002.

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

@HyukjinKwon
Copy link
Member Author

HyukjinKwon commented Jul 11, 2019

retest this please

viirya
viirya approved these changes Jul 11, 2019
View reviewed changes
Copy link
Member

@viirya viirya left a comment

Choose a reason for hiding this comment

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

Good catch!

viirya
viirya reviewed Jul 11, 2019
View reviewed changes
HyukjinKwon
HyukjinKwon commented Jul 11, 2019
View reviewed changes
sql/core/src/main/scala/org/apache/spark/sql/UDFRegistration.scala Outdated
Copy link
Member Author

@HyukjinKwon HyukjinKwon Jul 11, 2019

Choose a reason for hiding this comment

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

This one, I manually tested but didn't add the test just in case we want to add some kind of optimization in the future. We shouldn't do such thing here but in optimizer anyway. Seems like just a mistake.

@SparkQA
Copy link

SparkQA commented Jul 11, 2019

Test build #107517 has finished for PR 25108 at commit c725002.

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

viirya
viirya reviewed Jul 11, 2019
View reviewed changes
@SparkQA
Copy link

SparkQA commented Jul 11, 2019

Test build #107529 has finished for PR 25108 at commit 9039f3d.

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

srowen
srowen reviewed Jul 11, 2019
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.

Seems OK, though I don't know enough to really review.

@SparkQA
Copy link

SparkQA commented Jul 11, 2019

Test build #107536 has finished for PR 25108 at commit 1567b6c.

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

@cloud-fan
Copy link
Contributor

cloud-fan commented Jul 12, 2019

thanks, merging to master!

@cloud-fan cloud-fan closed this in a5c88ec Jul 12, 2019
vinodkc pushed a commit to vinodkc/spark that referenced this pull request Jul 18, 2019
@HyukjinKwon @vinodkc
[SPARK-28321][SQL] 0-args Java UDF should not be called only once
6280475 
## What changes were proposed in this pull request?

0-args Java UDF alone calls the function even before making it as an expression.
It causes that the function always returns the same value and the function is called at driver side.
Seems like a mistake.

## How was this patch tested?

Unit test was added

Closes apache#25108 from HyukjinKwon/SPARK-28321.

Authored-by: HyukjinKwon <[email protected]>
Signed-off-by: Wenchen Fan <[email protected]>
@gatorsmile
Copy link
Member

gatorsmile commented Jul 21, 2019

This a nice fix, but we still need to document it in the migration guide. This changes a behavior. Also it could cause a perf regression when calling the 0-args Java UDF is expensive.

@HyukjinKwon
Copy link
Member Author

HyukjinKwon commented Jul 22, 2019

Yea, that's fine. Let me update the migration guide.

@HyukjinKwon HyukjinKwon mentioned this pull request Jul 22, 2019
Closed
yiheng pushed a commit to yiheng/spark that referenced this pull request Jul 24, 2019
@HyukjinKwon @yiheng
[SPARK-28321][DOCS][FOLLOW-UP] Update migration guide by 0-args Java …
3ed547e 
…UDF's internal behaviour change

## What changes were proposed in this pull request?

This PR proposes to add a note in the migration guide. See apache#25108 (comment)

## How was this patch tested?

N/A

Closes apache#25224 from HyukjinKwon/SPARK-28321-doc.

Lead-authored-by: HyukjinKwon <[email protected]>
Co-authored-by: Hyukjin Kwon <[email protected]>
Signed-off-by: Wenchen Fan <[email protected]>
@HyukjinKwon HyukjinKwon deleted the SPARK-28321 branch March 3, 2020 01:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@viirya viirya viirya approved these changes

@srowen srowen srowen left review comments

Assignees

No one assigned

Labels

SQL

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@HyukjinKwon @cloud-fan @SparkQA @gatorsmile @viirya @srowen @dongjoon-hyun