Skip to content

[MINOR] Avoid code duplication for nullable in Higher Order function #22243

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
mgaido91 wants to merge 2 commits into from
Closed

[MINOR] Avoid code duplication for nullable in Higher Order function #22243

mgaido91 wants to merge 2 commits into from

Conversation

@mgaido91
Copy link
Contributor

@mgaido91 mgaido91 commented Aug 27, 2018 •
edited
Loading

What changes were proposed in this pull request?

Most of HigherOrderFunctions have the same nullable definition, ie. they are nullable when one of their arguments is nullable. The PR refactors it in order to avoid code duplication.

How was this patch tested?

NA

@mgaido91
Copy link
Contributor Author

mgaido91 commented Aug 27, 2018

cc @ueshin @mn-mikke

hvanhovell
hvanhovell approved these changes Aug 27, 2018
View reviewed changes
Copy link
Contributor

@hvanhovell hvanhovell left a comment

Choose a reason for hiding this comment

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

LGTM

@kiszk
Copy link
Member

kiszk commented Aug 27, 2018

LGTM

@ueshin
Copy link
Member

ueshin commented Aug 27, 2018

LGTM.

mn-mikke
mn-mikke reviewed Aug 27, 2018
View reviewed changes
...catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/higherOrderFunctions.scala Outdated
*/
trait SimpleHigherOrderFunction extends HigherOrderFunction {

override def nullable: Boolean = argument.nullable
Copy link
Contributor

@mn-mikke mn-mikke Aug 27, 2018

Choose a reason for hiding this comment

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

If we moved the definition of nullable straight to HigherOrderFunction as arguments.exists(_.nullable), we could also avoid the duplicities in ZipWith and MapZipWith. WDYT?

Copy link
Contributor Author

@mgaido91 mgaido91 Aug 27, 2018

Choose a reason for hiding this comment

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

this works too IMO, if others agree I'll update with this suggestion, thanks.

Copy link
Member

@HyukjinKwon HyukjinKwon Aug 28, 2018

Choose a reason for hiding this comment

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

Yea, let's go ahead then if the change is small, straightforward and more deduplciation

@SparkQA
Copy link

SparkQA commented Aug 27, 2018

Test build #95288 has finished for PR 22243 at commit 7bf7b29.

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

mn-mikke
mn-mikke approved these changes Aug 28, 2018
View reviewed changes
Copy link
Contributor

@mn-mikke mn-mikke left a comment

Choose a reason for hiding this comment

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

LGTM

@SparkQA
Copy link

SparkQA commented Aug 28, 2018

Test build #95342 has finished for PR 22243 at commit 393379c.

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

@mgaido91
Copy link
Contributor Author

mgaido91 commented Aug 28, 2018

retest this please

@SparkQA
Copy link

SparkQA commented Aug 28, 2018

Test build #95345 has finished for PR 22243 at commit 393379c.

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

@mgaido91
Copy link
Contributor Author

mgaido91 commented Aug 28, 2018

retest this please

@SparkQA
Copy link

SparkQA commented Aug 28, 2018

Test build #95350 has finished for PR 22243 at commit 393379c.

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

@mgaido91
Copy link
Contributor Author

mgaido91 commented Aug 28, 2018

retest this please

@SparkQA
Copy link

SparkQA commented Aug 28, 2018

Test build #95362 has finished for PR 22243 at commit 393379c.

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

@HyukjinKwon
Copy link
Member

HyukjinKwon commented Aug 29, 2018

Merged to master.

@asfgit asfgit closed this in 32c8a3d Aug 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@HyukjinKwon HyukjinKwon HyukjinKwon approved these changes

@hvanhovell hvanhovell hvanhovell approved these changes

+1 more reviewer

@mn-mikke mn-mikke mn-mikke approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@mgaido91 @kiszk @ueshin @SparkQA @HyukjinKwon @mn-mikke @hvanhovell