[GitHub] spark pull request #22243: [MINOR] Avoid code duplication for nullable in Hi...

2018-08-28 Thread asfgit 
Github user asfgit closed the pull request at:

https://github.com/apache/spark/pull/22243


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #22243: [MINOR] Avoid code duplication for nullable in Hi...

2018-08-27 Thread HyukjinKwon 
Github user HyukjinKwon commented on a diff in the pull request:

https://github.com/apache/spark/pull/22243#discussion_r213178417
  
--- Diff: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/higherOrderFunctions.scala
 ---
@@ -155,6 +155,8 @@ trait HigherOrderFunction extends Expression with 
ExpectsInputTypes {
  */
 trait SimpleHigherOrderFunction extends HigherOrderFunction  {
 
+  override def nullable: Boolean = argument.nullable
--- End diff --

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


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #22243: [MINOR] Avoid code duplication for nullable in Hi...

2018-08-27 Thread mgaido91 
Github user mgaido91 commented on a diff in the pull request:

https://github.com/apache/spark/pull/22243#discussion_r213029487
  
--- Diff: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/higherOrderFunctions.scala
 ---
@@ -155,6 +155,8 @@ trait HigherOrderFunction extends Expression with 
ExpectsInputTypes {
  */
 trait SimpleHigherOrderFunction extends HigherOrderFunction  {
 
+  override def nullable: Boolean = argument.nullable
--- End diff --

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


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #22243: [MINOR] Avoid code duplication for nullable in Hi...

2018-08-27 Thread mn-mikke 
Github user mn-mikke commented on a diff in the pull request:

https://github.com/apache/spark/pull/22243#discussion_r213022884
  
--- Diff: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/higherOrderFunctions.scala
 ---
@@ -155,6 +155,8 @@ trait HigherOrderFunction extends Expression with 
ExpectsInputTypes {
  */
 trait SimpleHigherOrderFunction extends HigherOrderFunction  {
 
+  override def nullable: Boolean = argument.nullable
--- End diff --

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?


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #22243: [MINOR] Avoid code duplication for nullable in Hi...

2018-08-27 Thread mgaido91 
GitHub user mgaido91 opened a pull request:

https://github.com/apache/spark/pull/22243

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

## What changes were proposed in this pull request?

All `SimpleHigherOrderFunction `s have the same `nullable` definition. The 
PR refactors it in order to avoid code duplication.

## How was this patch tested?

NA


You can merge this pull request into a Git repository by running:

$ git pull https://github.com/mgaido91/spark MINOR_nullable_hof

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/spark/pull/22243.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #22243


commit 7bf7b2916f0ff16b8dae21f0fa58d65e3b5a96d2
Author: Marco Gaido 
Date:   2018-08-27T13:29:27Z

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




---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org