[SPARK-21072][SQL] TreeNode.mapChildren should only apply to the children node. #18284

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

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/trees/TreeNode.scala

+            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

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 {


nit: nonSons

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

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)))


nit: nonChildren

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

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/trees/TreeNodeSuite.scala

+    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.

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

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/trees/TreeNodeSuite.scala

+    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

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 {


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> Closes apache#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> Closes apache#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

gczsjdy 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 87ab0ce Jun 16, 2017

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-21072][SQL] TreeNode.mapChildren should only apply to the children node. #18284

[SPARK-21072][SQL] TreeNode.mapChildren should only apply to the children node. #18284

ConeyLiu commented Jun 13, 2017

ConeyLiu commented Jun 13, 2017

cloud-fan commented Jun 14, 2017

cloud-fan commented Jun 14, 2017

SparkQA commented Jun 14, 2017

ConeyLiu commented Jun 14, 2017

SparkQA commented Jun 15, 2017

cloud-fan Jun 15, 2017

cloud-fan Jun 15, 2017

cloud-fan Jun 15, 2017

cloud-fan Jun 15, 2017

cloud-fan Jun 15, 2017

SparkQA commented Jun 15, 2017

cloud-fan left a comment

ConeyLiu commented Jun 15, 2017

SparkQA commented Jun 15, 2017

ConeyLiu commented Jun 15, 2017 •

edited

Loading

gczsjdy Jun 15, 2017

ConeyLiu Jun 15, 2017

cloud-fan commented Jun 15, 2017

viirya commented Jun 15, 2017

viirya Jun 15, 2017

ConeyLiu Jun 15, 2017

viirya commented Jun 15, 2017

SparkQA commented Jun 15, 2017

SparkQA commented Jun 15, 2017

cloud-fan commented Jun 16, 2017

ConeyLiu commented Jun 16, 2017

[SPARK-21072][SQL] TreeNode.mapChildren should only apply to the children node. #18284

[SPARK-21072][SQL] TreeNode.mapChildren should only apply to the children node. #18284

Conversation

ConeyLiu commented Jun 13, 2017

What changes were proposed in this pull request?

How was this patch tested?

ConeyLiu commented Jun 13, 2017

cloud-fan commented Jun 14, 2017

cloud-fan commented Jun 14, 2017

SparkQA commented Jun 14, 2017

ConeyLiu commented Jun 14, 2017

SparkQA commented Jun 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Jun 15, 2017

cloud-fan left a comment

Choose a reason for hiding this comment

ConeyLiu commented Jun 15, 2017

SparkQA commented Jun 15, 2017

ConeyLiu commented Jun 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cloud-fan commented Jun 15, 2017

viirya commented Jun 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

viirya commented Jun 15, 2017

SparkQA commented Jun 15, 2017

SparkQA commented Jun 15, 2017

cloud-fan commented Jun 16, 2017

ConeyLiu commented Jun 16, 2017

ConeyLiu commented Jun 15, 2017 •

edited

Loading