Skip to content

Conversation

@sunchao
Copy link
Member

@sunchao sunchao commented May 2, 2021

What changes were proposed in this pull request?

In StaticInvoke, when result is nullable, don't box the return value if its type is primitive.

Why are the changes needed?

It is unnecessary to apply boxing when the method return value is of primitive type, and it would hurt performance a lot if the method is simple. The check is done in Invoke but not in StaticInvoke.

Does this PR introduce any user-facing change?

No

How was this patch tested?

Added a UT.

@github-actions github-actions bot added the SQL label May 2, 2021
@SparkQA
Copy link

SparkQA commented May 2, 2021

Kubernetes integration test unable to build dist.

exiting with code: 1
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/42662/

${ev.value} = $boxedResult;
}
"""
if (method.getReturnType.isPrimitive) {
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, why returnNullable is true when return type is primitive?

Copy link
Member

Choose a reason for hiding this comment

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

Doesn't primitive return type go the last else block (i.e. returnNullable = false)?

Copy link
Member Author

@sunchao sunchao May 2, 2021

Choose a reason for hiding this comment

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

I think the caller of StaticInvoke can set returnNullable to true even though the return type is primitive, so it's better to check here nevertheless. I think I can make the code simpler by reusing the else branch.

Copy link
Member

Choose a reason for hiding this comment

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

Yea, if the caller sets returnNullable correctly, StaticInvoke won't box primitive return value. I agree, it is possible the caller wrongly set returnNullable to true.

@SparkQA
Copy link

SparkQA commented May 2, 2021

Test build #138141 has finished for PR 32416 at commit 7c687d0.

  • This patch fails from timeout after a configured wait of 500m.
  • This patch merges cleanly.
  • This patch adds no public classes.

@viirya
Copy link
Member

viirya commented May 2, 2021

retest this please

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.

lgtm

@HyukjinKwon
Copy link
Member

retest this please

@HyukjinKwon
Copy link
Member

Merged to master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants