You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by ConeyLiu <gi...@git.apache.org> on 2017/06/13 05:15:14 UTC
[GitHub] spark pull request #18284: [SPARK-21072] `TreeNode.mapChildren` should only ...
GitHub user ConeyLiu opened a pull request:
https://github.com/apache/spark/pull/18284
[SPARK-21072] `TreeNode.mapChildren` should only apply to the children node.
## What changes were proposed in this pull request?
Just as the function name and comments of `TreeNode.mapChildren` mentioned, the function should be apply to all currently node children. So, the follow code should judge whether it is the children node.
https://github.com/apache/spark/blob/master/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/trees/TreeNode.scala#L342
## How was this patch tested?
Existing tests.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/ConeyLiu/spark treenode
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/18284.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 #18284
----
commit f0d5dc9338585086b233c5eb68144ec0e6f5389d
Author: Xianyang Liu <xi...@intel.com>
Date: 2017-06-13T04:55:57Z
mapChildren should only apply to Children
----
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18284: [SPARK-21072][SQL] `TreeNode.mapChildren` should only ap...
Posted by ConeyLiu <gi...@git.apache.org>.
Github user ConeyLiu commented on the issue:
https://github.com/apache/spark/pull/18284
I can't find the test failed message, seems like unrelated errors. Can you trigger jenkins test again, because I'm not in the whitelist. Thanks a lot.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18284: [SPARK-21072][SQL] `TreeNode.mapChildren` should only ap...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/18284
Merged build finished. Test PASSed.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #18284: [SPARK-21072][SQL] `TreeNode.mapChildren` should ...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/18284#discussion_r122120741
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/trees/TreeNode.scala ---
@@ -340,11 +340,21 @@ abstract class TreeNode[BaseType <: TreeNode[BaseType]] extends Product {
arg
}
case tuple@(arg1: TreeNode[_], arg2: TreeNode[_]) =>
- val newChild1 = f(arg1.asInstanceOf[BaseType])
- val newChild2 = f(arg2.asInstanceOf[BaseType])
+ val newChild1 = if (containsChild(arg1)) {
+ f(arg1.asInstanceOf[BaseType])
+ } else {
+ arg1
--- End diff --
we can call `arg1.asInstanceOf[BaseType]` here, to avoid [this change](https://github.com/apache/spark/pull/18284/files#diff-eac5b02bb450a235fef5e902a2671254R357)
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18284: [SPARK-21072][SQL] TreeNode.mapChildren should only appl...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/18284
Merged build finished. Test PASSed.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #18284: [SPARK-21072][SQL] `TreeNode.mapChildren` should ...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/18284#discussion_r122120954
--- Diff: sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/trees/TreeNodeSuite.scala ---
@@ -146,6 +154,23 @@ class TreeNodeSuite extends SparkFunSuite {
assert(actual === Dummy(None))
}
+ test("mapChildren should only works on children") {
+ val children = Seq((Literal(1), Literal(2)))
+ val notChildren = Seq((Literal(3), Literal(4)))
+ val before = SeqTupleExpression(children, notChildren)
+ val toZero: PartialFunction[Expression, Expression] = { case Literal(_, _) => Literal(0) }
+ val expect = SeqTupleExpression(Seq((Literal(0), Literal(0))), notChildren)
+
+ var actual = before transformDown toZero
+ assert(actual === expect)
+
+ actual = before transformUp toZero
+ assert(actual === expect)
+
+ actual = before transform toZero
--- End diff --
or we can testing `mapChildren` directly
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18284: [SPARK-21072][SQL] `TreeNode.mapChildren` should only ap...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/18284
Merged build finished. Test FAILed.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18284: [SPARK-21072][SQL] `TreeNode.mapChildren` should only ap...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/18284
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/78075/
Test PASSed.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18284: [SPARK-21072] `TreeNode.mapChildren` should only apply t...
Posted by ConeyLiu <gi...@git.apache.org>.
Github user ConeyLiu commented on the issue:
https://github.com/apache/spark/pull/18284
@cloud-fan Would you mind take a look? Thanks a lot.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18284: [SPARK-21072][SQL] `TreeNode.mapChildren` should only ap...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/18284
Merged build finished. Test PASSed.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #18284: [SPARK-21072][SQL] `TreeNode.mapChildren` should ...
Posted by ConeyLiu <gi...@git.apache.org>.
Github user ConeyLiu commented on a diff in the pull request:
https://github.com/apache/spark/pull/18284#discussion_r122139485
--- Diff: sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/trees/TreeNodeSuite.scala ---
@@ -146,6 +154,17 @@ class TreeNodeSuite extends SparkFunSuite {
assert(actual === Dummy(None))
}
+ test("mapChildren should only works on children") {
+ val children = Seq((Literal(1), Literal(2)))
+ val nonChildren = Seq((Literal(3), Literal(4)))
+ val before = SeqTupleExpression(children, nonChildren)
+ val toZero: PartialFunction[Expression, Expression] = { case Literal(_, _) => Literal(0) }
+ val expect = SeqTupleExpression(Seq((Literal(0), Literal(0))), nonChildren)
+
+ val actual = before mapChildren toZero
+ assert(actual === expect)
--- End diff --
I'm not sure if I understand it wrongly. `===` compared to `==` could provide more information about the error. You can see the follow examples:
```
scala> assert(1 == 2)
java.lang.AssertionError: assertion failed
at scala.Predef$.assert(Predef.scala:156)
... 32 elided
```
```
scala> assert(1 === 2)
<console>:12: error: value === is not a member of Int
assert(1 === 2)
```
And also you can check it in https://stackoverflow.com/questions/10489548/what-is-the-triple-equals-operator-in-scala-koans
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #18284: [SPARK-21072][SQL] `TreeNode.mapChildren` should ...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/18284#discussion_r122120790
--- Diff: sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/trees/TreeNodeSuite.scala ---
@@ -61,6 +61,14 @@ case class ExpressionInMap(map: Map[String, Expression]) extends Expression with
override lazy val resolved = true
}
+case class SeqTupleExpression(sons: Seq[(Expression, Expression)],
--- End diff --
`children` and `nonChildren` may be better names
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18284: [SPARK-21072][SQL] `TreeNode.mapChildren` should only ap...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/18284
Can one of the admins verify this patch?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #18284: [SPARK-21072][SQL] `TreeNode.mapChildren` should ...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/18284#discussion_r122120916
--- Diff: sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/trees/TreeNodeSuite.scala ---
@@ -146,6 +154,23 @@ class TreeNodeSuite extends SparkFunSuite {
assert(actual === Dummy(None))
}
+ test("mapChildren should only works on children") {
+ val children = Seq((Literal(1), Literal(2)))
+ val notChildren = Seq((Literal(3), Literal(4)))
+ val before = SeqTupleExpression(children, notChildren)
+ val toZero: PartialFunction[Expression, Expression] = { case Literal(_, _) => Literal(0) }
+ val expect = SeqTupleExpression(Seq((Literal(0), Literal(0))), notChildren)
+
+ var actual = before transformDown toZero
+ assert(actual === expect)
+
+ actual = before transformUp toZero
+ assert(actual === expect)
+
+ actual = before transform toZero
--- End diff --
I think testing `transform` is good enough
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18284: [SPARK-21072][SQL] `TreeNode.mapChildren` should only ap...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on the issue:
https://github.com/apache/spark/pull/18284
ok to test
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #18284: [SPARK-21072][SQL] TreeNode.mapChildren should on...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/18284
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18284: [SPARK-21072][SQL] `TreeNode.mapChildren` should only ap...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/18284
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/78043/
Test FAILed.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18284: [SPARK-21072][SQL] `TreeNode.mapChildren` should only ap...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/18284
**[Test build #78043 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/78043/testReport)** for PR 18284 at commit [`f0d5dc9`](https://github.com/apache/spark/commit/f0d5dc9338585086b233c5eb68144ec0e6f5389d).
* This patch **fails Scala style tests**.
* This patch merges cleanly.
* This patch adds no public classes.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18284: [SPARK-21072][SQL] `TreeNode.mapChildren` should only ap...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/18284
**[Test build #78092 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/78092/testReport)** for PR 18284 at commit [`9a9d8af`](https://github.com/apache/spark/commit/9a9d8afee0e4ef59bfd4faca66e106851d51822e).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18284: [SPARK-21072][SQL] `TreeNode.mapChildren` should only ap...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on the issue:
https://github.com/apache/spark/pull/18284
good catch! can you add a test in `TreeNodeSuite`?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18284: [SPARK-21072][SQL] `TreeNode.mapChildren` should only ap...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on the issue:
https://github.com/apache/spark/pull/18284
LGTM
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18284: [SPARK-21072][SQL] `TreeNode.mapChildren` should only ap...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on the issue:
https://github.com/apache/spark/pull/18284
retest this please
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18284: [SPARK-21072][SQL] `TreeNode.mapChildren` should only ap...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/18284
**[Test build #78082 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/78082/testReport)** for PR 18284 at commit [`9a9d8af`](https://github.com/apache/spark/commit/9a9d8afee0e4ef59bfd4faca66e106851d51822e).
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #18284: [SPARK-21072][SQL] `TreeNode.mapChildren` should ...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/18284#discussion_r122120866
--- Diff: sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/trees/TreeNodeSuite.scala ---
@@ -146,6 +154,23 @@ class TreeNodeSuite extends SparkFunSuite {
assert(actual === Dummy(None))
}
+ test("mapChildren should only works on children") {
+ val children = Seq((Literal(1), Literal(2)))
+ val notChildren = Seq((Literal(3), Literal(4)))
--- End diff --
nit: `nonChildren`
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18284: [SPARK-21072][SQL] `TreeNode.mapChildren` should only ap...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/18284
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/78092/
Test PASSed.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18284: [SPARK-21072][SQL] `TreeNode.mapChildren` should only ap...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/18284
**[Test build #78081 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/78081/testReport)** for PR 18284 at commit [`92d4a80`](https://github.com/apache/spark/commit/92d4a80929783cca9284b9b74c37fbfc8717dc96).
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18284: [SPARK-21072][SQL] TreeNode.mapChildren should only appl...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/18284
**[Test build #78108 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/78108/testReport)** for PR 18284 at commit [`27fe790`](https://github.com/apache/spark/commit/27fe7905d7935cf54c2c681a5504226d693d9455).
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18284: [SPARK-21072][SQL] `TreeNode.mapChildren` should only ap...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/18284
Merged build finished. Test FAILed.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #18284: [SPARK-21072][SQL] `TreeNode.mapChildren` should ...
Posted by gczsjdy <gi...@git.apache.org>.
Github user gczsjdy commented on a diff in the pull request:
https://github.com/apache/spark/pull/18284#discussion_r122123445
--- Diff: sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/trees/TreeNodeSuite.scala ---
@@ -146,6 +154,17 @@ class TreeNodeSuite extends SparkFunSuite {
assert(actual === Dummy(None))
}
+ test("mapChildren should only works on children") {
+ val children = Seq((Literal(1), Literal(2)))
+ val nonChildren = Seq((Literal(3), Literal(4)))
+ val before = SeqTupleExpression(children, nonChildren)
+ val toZero: PartialFunction[Expression, Expression] = { case Literal(_, _) => Literal(0) }
+ val expect = SeqTupleExpression(Seq((Literal(0), Literal(0))), nonChildren)
+
+ val actual = before mapChildren toZero
+ assert(actual === expect)
--- End diff --
Maybe better to use `.equals`? Although it will call Object's `.equals` which is `==`.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18284: [SPARK-21072][SQL] `TreeNode.mapChildren` should only ap...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/18284
**[Test build #78092 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/78092/testReport)** for PR 18284 at commit [`9a9d8af`](https://github.com/apache/spark/commit/9a9d8afee0e4ef59bfd4faca66e106851d51822e).
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #18284: [SPARK-21072][SQL] TreeNode.mapChildren should on...
Posted by ConeyLiu <gi...@git.apache.org>.
Github user ConeyLiu commented on a diff in the pull request:
https://github.com/apache/spark/pull/18284#discussion_r122205853
--- Diff: sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/trees/TreeNodeSuite.scala ---
@@ -61,6 +61,14 @@ case class ExpressionInMap(map: Map[String, Expression]) extends Expression with
override lazy val resolved = true
}
+case class SeqTupleExpression(sons: Seq[(Expression, Expression)],
+ nonSons: Seq[(Expression, Expression)]) extends Expression with Unevaluable {
--- End diff --
done, 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #18284: [SPARK-21072][SQL] `TreeNode.mapChildren` should ...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/18284#discussion_r122151719
--- Diff: sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/trees/TreeNodeSuite.scala ---
@@ -61,6 +61,14 @@ case class ExpressionInMap(map: Map[String, Expression]) extends Expression with
override lazy val resolved = true
}
+case class SeqTupleExpression(sons: Seq[(Expression, Expression)],
+ nonSons: Seq[(Expression, Expression)]) extends Expression with Unevaluable {
--- End diff --
`Unevaluable` already extends `Expression`. I think you don't need to extend `Expression` again?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18284: [SPARK-21072][SQL] `TreeNode.mapChildren` should only ap...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/18284
**[Test build #78075 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/78075/testReport)** for PR 18284 at commit [`c3622bb`](https://github.com/apache/spark/commit/c3622bbe3293c81dd3dc433e01af4107d5208a6d).
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18284: [SPARK-21072][SQL] `TreeNode.mapChildren` should only ap...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on the issue:
https://github.com/apache/spark/pull/18284
Nit: The PR title doesn't need to add ``.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18284: [SPARK-21072][SQL] `TreeNode.mapChildren` should only ap...
Posted by ConeyLiu <gi...@git.apache.org>.
Github user ConeyLiu commented on the issue:
https://github.com/apache/spark/pull/18284
Thanks for reviewing, I'll add it.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18284: [SPARK-21072][SQL] TreeNode.mapChildren should only appl...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/18284
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/78108/
Test PASSed.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #18284: [SPARK-21072][SQL] `TreeNode.mapChildren` should ...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/18284#discussion_r122120851
--- Diff: sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/trees/TreeNodeSuite.scala ---
@@ -61,6 +61,14 @@ case class ExpressionInMap(map: Map[String, Expression]) extends Expression with
override lazy val resolved = true
}
+case class SeqTupleExpression(sons: Seq[(Expression, Expression)],
+ notsons: Seq[(Expression, Expression)]) extends Expression with Unevaluable {
--- End diff --
nit: `nonSons`
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18284: [SPARK-21072][SQL] TreeNode.mapChildren should only appl...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/18284
**[Test build #78108 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/78108/testReport)** for PR 18284 at commit [`27fe790`](https://github.com/apache/spark/commit/27fe7905d7935cf54c2c681a5504226d693d9455).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds the following public classes _(experimental)_:
* `case class ExpressionInMap(map: Map[String, Expression]) extends Unevaluable `
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18284: [SPARK-21072][SQL] `TreeNode.mapChildren` should only ap...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/18284
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/78082/
Test FAILed.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18284: [SPARK-21072][SQL] `TreeNode.mapChildren` should only ap...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/18284
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/78081/
Test FAILed.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18284: [SPARK-21072][SQL] `TreeNode.mapChildren` should only ap...
Posted by ConeyLiu <gi...@git.apache.org>.
Github user ConeyLiu commented on the issue:
https://github.com/apache/spark/pull/18284
@cloud-fan thanks for reviewing, code has updated.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18284: [SPARK-21072][SQL] `TreeNode.mapChildren` should only ap...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/18284
**[Test build #78043 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/78043/testReport)** for PR 18284 at commit [`f0d5dc9`](https://github.com/apache/spark/commit/f0d5dc9338585086b233c5eb68144ec0e6f5389d).
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18284: [SPARK-21072][SQL] TreeNode.mapChildren should only appl...
Posted by ConeyLiu <gi...@git.apache.org>.
Github user ConeyLiu commented on the issue:
https://github.com/apache/spark/pull/18284
thanks everyone for reviewing.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18284: [SPARK-21072][SQL] `TreeNode.mapChildren` should only ap...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/18284
**[Test build #78075 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/78075/testReport)** for PR 18284 at commit [`c3622bb`](https://github.com/apache/spark/commit/c3622bbe3293c81dd3dc433e01af4107d5208a6d).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18284: [SPARK-21072][SQL] `TreeNode.mapChildren` should only ap...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/18284
Merged build finished. Test FAILed.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18284: [SPARK-21072][SQL] TreeNode.mapChildren should only appl...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on the issue:
https://github.com/apache/spark/pull/18284
thanks, merging to master/2.2/2.1!
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org