[SPARK-21072][SQL] TreeNode.mapChildren should only apply to the children node. by ConeyLiu · Pull Request #18284 · apache/spark

ConeyLiu · 2017-06-13T05:15:13Z

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.

ConeyLiu · 2017-06-13T05:15:58Z

@cloud-fan Would you mind take a look? Thanks a lot.

cloud-fan · 2017-06-14T08:36:40Z

ok to test

cloud-fan · 2017-06-14T08:38:46Z

good catch! can you add a test in TreeNodeSuite?

SparkQA · 2017-06-14T08:39:26Z

Test build #78043 has finished for PR 18284 at commit f0d5dc9.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

ConeyLiu · 2017-06-14T12:34:24Z

Thanks for reviewing, I'll add it.

SparkQA · 2017-06-15T03:20:09Z

Test build #78075 has finished for PR 18284 at commit c3622bb.

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

cloud-fan · 2017-06-15T05:11:19Z

+ val newChild1 = if (containsChild(arg1)) {
+ f(arg1.asInstanceOf[BaseType])
+ } else {
+ arg1


we can call arg1.asInstanceOf[BaseType] here, to avoid this change

cloud-fan · 2017-06-15T05:12:44Z

 }

+case class SeqTupleExpression(sons: Seq[(Expression, Expression)],
+ notsons: Seq[(Expression, Expression)]) extends Expression with Unevaluable {


nit: nonSons

cloud-fan · 2017-06-15T05:12:54Z


+ test("mapChildren should only works on children") {
+ val children = Seq((Literal(1), Literal(2)))
+ val notChildren = Seq((Literal(3), Literal(4)))


nit: nonChildren

cloud-fan · 2017-06-15T05:13:23Z

+ actual = before transformUp toZero
+ assert(actual === expect)
+
+ actual = before transform toZero


I think testing transform is good enough

or we can testing mapChildren directly

SparkQA · 2017-06-15T05:22:39Z

Test build #78081 has started for PR 18284 at commit 92d4a80.

cloud-fan

LGTM, pending tests

ConeyLiu · 2017-06-15T05:24:03Z

@cloud-fan thanks for reviewing, code has updated.

SparkQA · 2017-06-15T05:27:37Z

Test build #78082 has started for PR 18284 at commit 9a9d8af.

ConeyLiu · 2017-06-15T07:19:20Z

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.

gcz2022 · 2017-06-15T05:43:16Z

+ val expect = SeqTupleExpression(Seq((Literal(0), Literal(0))), nonChildren)
+
+ val actual = before mapChildren toZero
+ assert(actual === expect)


Maybe better to use .equals? Although it will call Object's .equals which is ==.

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

cloud-fan · 2017-06-15T08:19:54Z

retest this please

viirya · 2017-06-15T08:48:40Z

Nit: The PR title doesn't need to add ``.

viirya · 2017-06-15T08:50:27Z

 }

+case class SeqTupleExpression(sons: Seq[(Expression, Expression)],
+ nonSons: Seq[(Expression, Expression)]) extends Expression with Unevaluable {


Unevaluable already extends Expression. I think you don't need to extend Expression again?

done, thanks

viirya · 2017-06-15T08:52:07Z

LGTM

SparkQA · 2017-06-15T10:40:07Z

Test build #78092 has finished for PR 18284 at commit 9a9d8af.

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

SparkQA · 2017-06-15T16:06:40Z

Test build #78108 has finished for PR 18284 at commit 27fe790.

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

…dren 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. Author: Xianyang Liu <xianyang.liu@intel.com> Closes#18284 from ConeyLiu/treenode. (cherry picked from commit 87ab0ce) Signed-off-by: Wenchen Fan <wenchen@databricks.com>

cloud-fan · 2017-06-16T04:11:19Z

thanks, merging to master/2.2/2.1!

ConeyLiu · 2017-06-16T05:29:46Z

thanks everyone for reviewing.

…dren 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. Author: Xianyang Liu <xianyang.liu@intel.com> Closesapache#18284 from ConeyLiu/treenode.

…dren node. 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 Existing tests. Author: Xianyang Liu <xianyang.liu@intel.com> Closesapache#18284 from ConeyLiu/treenode. (cherry picked from commit 87ab0ce) Signed-off-by: Wenchen Fan <wenchen@databricks.com>

mapChildren should only apply to Children
f0d5dc9

ConeyLiu changed the title ~~[SPARK-21072] TreeNode.mapChildren should only apply to the children node.~~[SPARK-21072][SQL] TreeNode.mapChildren should only apply to the children node.Jun 13, 2017

fix code style and add test case
c3622bb

cloud-fan reviewed Jun 15, 2017
View reviewed changes

ConeyLiu added 2 commits June 15, 2017 13:20

address comments
92d4a80

small fix
9a9d8af

cloud-fan approved these changes Jun 15, 2017
View reviewed changes

gcz2022 reviewed Jun 15, 2017
View reviewed changes

viirya reviewed Jun 15, 2017
View reviewed changes

ConeyLiu changed the title ~~[SPARK-21072][SQL] TreeNode.mapChildren should only apply to the children node.~~[SPARK-21072][SQL] TreeNode.mapChildren should only apply to the children node.Jun 15, 2017

address comments
27fe790

asfgit closed this in 87ab0ceJun 16, 2017

ConeyLiu deleted the treenode branch June 16, 2017 05:29

Uh oh!

Conversation

ConeyLiu commented Jun 13, 2017

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

ConeyLiu commented Jun 13, 2017

Uh oh!

cloud-fan commented Jun 14, 2017

Uh oh!

cloud-fan commented Jun 14, 2017

Uh oh!

SparkQA commented Jun 14, 2017

Uh oh!

ConeyLiu commented Jun 14, 2017

Uh oh!

SparkQA commented Jun 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jun 15, 2017

Uh oh!

cloud-fan left a comment

Choose a reason for hiding this comment

Uh oh!

ConeyLiu commented Jun 15, 2017

Uh oh!

SparkQA commented Jun 15, 2017

Uh oh!

ConeyLiu commented Jun 15, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cloud-fan commented Jun 15, 2017

Uh oh!

viirya commented Jun 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

viirya commented Jun 15, 2017

Uh oh!

SparkQA commented Jun 15, 2017

Uh oh!

SparkQA commented Jun 15, 2017

Uh oh!

cloud-fan commented Jun 16, 2017

Uh oh!

ConeyLiu commented Jun 16, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ConeyLiu commented Jun 15, 2017•
edited
Loading