[SPARK-7269] [SQL] Incorrect analysis for aggregation(use semanticEquals) #6173

cloud-fan · 2015-05-15T05:58:19Z

A modified version of #6110, use semanticEquals to make it more efficient.

cloud-fan · 2015-05-15T05:59:46Z

cc @chenghao-intel @marmbrus

AmplabJenkins · 2015-05-15T06:02:11Z

Merged build triggered.

AmplabJenkins · 2015-05-15T06:02:19Z

Merged build started.

SparkQA · 2015-05-15T06:04:15Z

Test build #32779 has started for PR 6173 at commit d7ff8f4.

chenghao-intel · 2015-05-15T06:14:02Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Expression.scala

+  def semanticEquals(other: Expression): Boolean = this.getClass == other.getClass &&
+    this.productIterator.zip(other.asInstanceOf[Product].productIterator).forall {
+      case (e1: Expression, e2: Expression) => e1 semanticEquals e2
+      case (i1, i2) => i1 == i2


What about case class Coalesce(children: Seq[Expression])?

The difficulty here is we probably never knows semanticEquals in a general way, that's why I said we need to re-implemented for lots of expressions if we added this.

AmplabJenkins · 2015-05-15T06:42:10Z

Merged build triggered.

AmplabJenkins · 2015-05-15T06:42:20Z

Merged build started.

SparkQA · 2015-05-15T06:44:17Z

Test build #32785 has started for PR 6173 at commit cc02045.

cloud-fan · 2015-05-15T06:46:16Z

Hi @chenghao-intel , as far as I know, we can only instantiate leaf expressions which are all case classes. So probably we can design the semanticEquals as how case class implement equals but use semanticEquals for expression elements.

SparkQA · 2015-05-15T08:01:14Z

Test build #32779 has finished for PR 6173 at commit d7ff8f4.

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

AmplabJenkins · 2015-05-15T08:01:18Z

Merged build finished. Test PASSed.

AmplabJenkins · 2015-05-15T08:01:18Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/32779/
Test PASSed.

SparkQA · 2015-05-15T08:41:57Z

Test build #32785 has finished for PR 6173 at commit cc02045.

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

AmplabJenkins · 2015-05-15T08:42:01Z

Merged build finished. Test PASSed.

AmplabJenkins · 2015-05-15T08:42:02Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/32785/
Test PASSed.

marmbrus · 2015-05-15T18:53:01Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Expression.scala

@@ -76,6 +76,15 @@ abstract class Expression extends TreeNode[Expression] {
      case u: UnresolvedAttribute => PrettyAttribute(u.name)
    }.toString
  }
+
+  def semanticEquals(other: Expression): Boolean = this.getClass == other.getClass && {


scala doc please.

AmplabJenkins · 2015-05-16T04:22:10Z

Merged build triggered.

AmplabJenkins · 2015-05-16T04:22:15Z

Merged build started.

SparkQA · 2015-05-16T04:24:13Z

Test build #32882 has started for PR 6173 at commit e4a3cc7.

SparkQA · 2015-05-16T06:18:32Z

Test build #32882 has finished for PR 6173 at commit e4a3cc7.

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

AmplabJenkins · 2015-05-16T06:18:37Z

Merged build finished. Test PASSed.

AmplabJenkins · 2015-05-16T06:18:37Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/32882/
Test PASSed.

chenghao-intel · 2015-05-17T11:21:41Z

Thank you @cloud-fan for doing this, and I think this can work as a workaround for cases like Seq[Expression].find, however, there are still cases like Map[Expression, Expression] e.g. in patterns.scala as I did in #5798.
Adding new method for Expression, probably impact all of Expression system (increasing the overhead for maintenance for future?), instead of doing this, I still insist to provide specific collection like ExpressionMap or ExpressionSet.

marmbrus · 2015-05-18T19:07:35Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Expression.scala

@@ -76,6 +76,19 @@ abstract class Expression extends TreeNode[Expression] {
      case u: UnresolvedAttribute => PrettyAttribute(u.name)
    }.toString
  }
+
+  /**
+   * Returns true if 2 expressions are equal in semantic, which is similar to equals method


Returns true when two expressions will always compute the same result, even if they differ cosmetically (i.e. capitalization of names in attributes may be different).

marmbrus · 2015-05-18T19:11:59Z

@chenghao-intel, a single method seems easier to maintain then full Map/Set implementations. I'll add that its not clear to me the use in patterns is actually invalid. You can compare expression with equality when you are only trying to find the exact same expression.

I'm merging this to master and 1.4. I'll improve the wording of the scala doc while merging.

…als) A modified version of #6110, use `semanticEquals` to make it more efficient. Author: Wenchen Fan <cloud0fan@outlook.com> Closes #6173 from cloud-fan/7269 and squashes the following commits: e4a3cc7 [Wenchen Fan] address comments cc02045 [Wenchen Fan] consider elements length equal d7ff8f4 [Wenchen Fan] fix 7269 (cherry picked from commit 103c863) Signed-off-by: Michael Armbrust <michael@databricks.com>

chenghao-intel · 2015-05-19T00:37:20Z

I am not sure why we use the Map[Expression, NamedExpression] in patterns.scala, seems Seq[(Expression, NamedExpression)] is more reasonable, which is definitely confusing for people why we couldn't using Set/Map for expressions or should use them in a very ugly way.

…als) A modified version of apache#6110, use `semanticEquals` to make it more efficient. Author: Wenchen Fan <cloud0fan@outlook.com> Closes apache#6173 from cloud-fan/7269 and squashes the following commits: e4a3cc7 [Wenchen Fan] address comments cc02045 [Wenchen Fan] consider elements length equal d7ff8f4 [Wenchen Fan] fix 7269

It's a follow up of #6173, for expressions like `Coalesce` that have a `Seq[Expression]`, when we do semantic equal check for it, we need to do semantic equal check for all of its children. Also we can just use `Seq[(Expression, NamedExpression)]` instead of `Map[Expression, NamedExpression]` as we only search it with `find`. chenghao-intel, I agree that we probably never knows `semanticEquals` in a general way, but I think we have done that in `TreeNode`, so we can use similar logic. Then we can handle something like `Coalesce(children: Seq[Expression])` correctly. Author: Wenchen Fan <cloud0fan@outlook.com> Closes #6261 from cloud-fan/tmp and squashes the following commits: 4daef88 [Wenchen Fan] address comments dd8fbd9 [Wenchen Fan] correct semanticEquals

…als) A modified version of apache#6110, use `semanticEquals` to make it more efficient. Author: Wenchen Fan <cloud0fan@outlook.com> Closes apache#6173 from cloud-fan/7269 and squashes the following commits: e4a3cc7 [Wenchen Fan] address comments cc02045 [Wenchen Fan] consider elements length equal d7ff8f4 [Wenchen Fan] fix 7269

It's a follow up of apache#6173, for expressions like `Coalesce` that have a `Seq[Expression]`, when we do semantic equal check for it, we need to do semantic equal check for all of its children. Also we can just use `Seq[(Expression, NamedExpression)]` instead of `Map[Expression, NamedExpression]` as we only search it with `find`. chenghao-intel, I agree that we probably never knows `semanticEquals` in a general way, but I think we have done that in `TreeNode`, so we can use similar logic. Then we can handle something like `Coalesce(children: Seq[Expression])` correctly. Author: Wenchen Fan <cloud0fan@outlook.com> Closes apache#6261 from cloud-fan/tmp and squashes the following commits: 4daef88 [Wenchen Fan] address comments dd8fbd9 [Wenchen Fan] correct semanticEquals

fix 7269

d7ff8f4

chenghao-intel reviewed May 15, 2015
View reviewed changes

consider elements length equal

cc02045

marmbrus reviewed May 15, 2015
View reviewed changes

address comments

e4a3cc7

marmbrus reviewed May 18, 2015
View reviewed changes

asfgit closed this in 103c863 May 18, 2015

cloud-fan deleted the 7269 branch May 19, 2015 07:47

cloud-fan mentioned this pull request Jun 10, 2015

[SQL][minor] correct semanticEquals logic #6261

Closed

[SPARK-7269] [SQL] Incorrect analysis for aggregation(use semanticEquals) #6173

[SPARK-7269] [SQL] Incorrect analysis for aggregation(use semanticEquals) #6173

Uh oh!

Conversation

cloud-fan commented May 15, 2015

Uh oh!

cloud-fan commented May 15, 2015

Uh oh!

AmplabJenkins commented May 15, 2015

Uh oh!

AmplabJenkins commented May 15, 2015

Uh oh!

SparkQA commented May 15, 2015

Uh oh!

chenghao-intel May 15, 2015

Choose a reason for hiding this comment

Uh oh!

chenghao-intel May 15, 2015

Choose a reason for hiding this comment

Uh oh!

AmplabJenkins commented May 15, 2015

Uh oh!

AmplabJenkins commented May 15, 2015

Uh oh!

SparkQA commented May 15, 2015

Uh oh!

cloud-fan commented May 15, 2015

Uh oh!

SparkQA commented May 15, 2015

Uh oh!

AmplabJenkins commented May 15, 2015

Uh oh!

AmplabJenkins commented May 15, 2015

Uh oh!

SparkQA commented May 15, 2015

Uh oh!

AmplabJenkins commented May 15, 2015

Uh oh!

AmplabJenkins commented May 15, 2015

Uh oh!

marmbrus May 15, 2015

Choose a reason for hiding this comment

Uh oh!

AmplabJenkins commented May 16, 2015

Uh oh!

AmplabJenkins commented May 16, 2015

Uh oh!

SparkQA commented May 16, 2015

Uh oh!

SparkQA commented May 16, 2015

Uh oh!

AmplabJenkins commented May 16, 2015

Uh oh!

AmplabJenkins commented May 16, 2015

Uh oh!

chenghao-intel commented May 17, 2015

Uh oh!

marmbrus May 18, 2015

Choose a reason for hiding this comment

Uh oh!

marmbrus commented May 18, 2015

Uh oh!

chenghao-intel commented May 19, 2015

Uh oh!

Uh oh!