[GitHub] spark pull request #14295: [SPARK-16648][SQL] Overrides TreeNode.withNewChil...

[liancheng]

Github user liancheng commented on a diff in the pull request:

https://github.com/apache/spark/pull/14295#discussion_r72013449
  
--- Diff: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/First.scala
 ---
@@ -45,6 +45,17 @@ case class First(child: Expression, ignoreNullsExpr: 
Expression) extends Declara
 
   override def children: Seq[Expression] = child :: Nil
--- End diff --

Yea, this looks like a cleaner fix. Let me try whether it breaks anything 
else. Thanks!


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request #14295: [SPARK-16648][SQL] Overrides TreeNode.withNewChil...

[cloud-fan]

Github user cloud-fan commented on a diff in the pull request:

https://github.com/apache/spark/pull/14295#discussion_r71990123
  
--- Diff: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/First.scala
 ---
@@ -45,6 +45,17 @@ case class First(child: Expression, ignoreNullsExpr: 
Expression) extends Declara
 
   override def children: Seq[Expression] = child :: Nil
--- End diff --

should we make `ignoreNullsExpr` also a child?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request #14295: [SPARK-16648][SQL] Overrides TreeNode.withNewChil...

[liancheng]

Github user liancheng commented on a diff in the pull request:

https://github.com/apache/spark/pull/14295#discussion_r71920301
  
--- Diff: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/Last.scala
 ---
@@ -42,6 +42,17 @@ case class Last(child: Expression, ignoreNullsExpr: 
Expression) extends Declarat
 
   override def children: Seq[Expression] = child :: Nil
 
+  // SPARK-16648: Default `TreeNode.withNewChildren` implementation 
doesn't work for `Last` when
--- End diff --

Well, it's probably unnecessary, since expressions are not part of the 
public API and won't appear in the generated ScalaDoc anyway...


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request #14295: [SPARK-16648][SQL] Overrides TreeNode.withNewChil...

[jaceklaskowski]

Github user jaceklaskowski commented on a diff in the pull request:

https://github.com/apache/spark/pull/14295#discussion_r71908714
  
--- Diff: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/Last.scala
 ---
@@ -42,6 +42,17 @@ case class Last(child: Expression, ignoreNullsExpr: 
Expression) extends Declarat
 
   override def children: Seq[Expression] = child :: Nil
 
+  // SPARK-16648: Default `TreeNode.withNewChildren` implementation 
doesn't work for `Last` when
--- End diff --

Make it so?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request #14295: [SPARK-16648][SQL] Overrides TreeNode.withNewChil...

[liancheng]

Github user liancheng commented on a diff in the pull request:

https://github.com/apache/spark/pull/14295#discussion_r71896054
  
--- Diff: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/Last.scala
 ---
@@ -42,6 +42,17 @@ case class Last(child: Expression, ignoreNullsExpr: 
Expression) extends Declarat
 
   override def children: Seq[Expression] = child :: Nil
 
+  // SPARK-16648: Default `TreeNode.withNewChildren` implementation 
doesn't work for `Last` when
--- End diff --

This is not a ScalaDoc comment, so `[[...]]` is not necessary.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request #14295: [SPARK-16648][SQL] Overrides TreeNode.withNewChil...

[jaceklaskowski]

Github user jaceklaskowski commented on a diff in the pull request:

https://github.com/apache/spark/pull/14295#discussion_r71871902
  
--- Diff: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/Last.scala
 ---
@@ -42,6 +42,17 @@ case class Last(child: Expression, ignoreNullsExpr: 
Expression) extends Declarat
 
   override def children: Seq[Expression] = child :: Nil
 
+  // SPARK-16648: Default `TreeNode.withNewChildren` implementation 
doesn't work for `Last` when
+  // both constructor arguments are the same, e.g.:
+  //
+  //   LAST_VALUE(FALSE) // The 2nd argument defaults to FALSE
+  //   LAST_VALUE(FALSE, FALSE)
+  //   LAST_VALUE(TRUE, TRUE)
+  override def withNewChildren(newChildren: Seq[Expression]): Expression = 
{
+val Seq(newChild) = newChildren
--- End diff --

👍 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request #14295: [SPARK-16648][SQL] Overrides TreeNode.withNewChil...

[jaceklaskowski]

Github user jaceklaskowski commented on a diff in the pull request:

https://github.com/apache/spark/pull/14295#discussion_r71871696
  
--- Diff: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/Last.scala
 ---
@@ -42,6 +42,17 @@ case class Last(child: Expression, ignoreNullsExpr: 
Expression) extends Declarat
 
   override def children: Seq[Expression] = child :: Nil
 
+  // SPARK-16648: Default `TreeNode.withNewChildren` implementation 
doesn't work for `Last` when
--- End diff --

Use `[[` to reference code symbols.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request #14295: [SPARK-16648][SQL] Overrides TreeNode.withNewChil...

[liancheng]

GitHub user liancheng opened a pull request:

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

[SPARK-16648][SQL] Overrides TreeNode.withNewChildren in Last

## What changes were proposed in this pull request?

Default `TreeNode.withNewChildren` implementation doesn't work for `Last` 
when both constructor arguments are the same, e.g.:

```sql
LAST_VALUE(FALSE) // The 2nd argument defaults to FALSE
LAST_VALUE(FALSE, FALSE)
LAST_VALUE(TRUE, TRUE)
```

This is because although `Last` is a unary expression, both of its 
constructor arguments are `Expression`s. When they have the same value, 
`TreeNode.withNewChildren` treats both of them as child nodes by mistake.

## How was this patch tested?

New test case added in `WindowQuerySuite`.

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

$ git pull https://github.com/liancheng/spark spark-16648-last-value

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

https://github.com/apache/spark/pull/14295.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 #14295


commit efbed9110b771a4f506292f73d4f0e6cb77e52d0
Author: Cheng Lian 
Date:   2016-07-21T04:48:16Z

Overrides TreeNode.withNewChildren in Last




---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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