[SPARK-7952][SPARK-7984][SQL] equality check between boolean type and numeric type is broken. #6505

cloud-fan · 2015-05-29T19:34:27Z

The origin code has several problems:

true <=> 1 will return false as we didn't set a rule to handle it.
true = a where a is not Literal and its value is 1, will return false as we only handle literal values.

cloud-fan · 2015-05-29T19:35:13Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/HiveTypeCoercion.scala

-      // Otherwise turn them to Byte types so that there exists and ordering.
-      case p: BinaryComparison if p.left.dataType == BooleanType &&
-                                  p.right.dataType == BooleanType =>
-        p.makeCopy(Array(Cast(p.left, ByteType), Cast(p.right, ByteType)))


boolean is already comparable, we don't need to cast it to byte.

SparkQA · 2015-05-29T19:41:28Z

Test build #33766 has finished for PR 6505 at commit a3f8e9c.

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

SparkQA · 2015-05-30T05:34:05Z

Test build #33794 has finished for PR 6505 at commit 18211e7.

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

rxin · 2015-05-30T06:26:52Z

cc @yhuai

rxin · 2015-05-30T07:28:10Z

Note that this no longer merges cleanly.

liancheng · 2015-05-30T09:59:17Z

sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala

+      Seq(true, true, false, false, null, null, null, null, null).map(Row(_)))
+    checkAnswer(sql("select i <=> b from t"),
+      Seq(true, true, false, false, false, false, false, false, true).map(Row(_)))
+  }


This test case can be simplified a bit, also please drop the temp table at last:

val data = Seq( (1, true), (0, false), (2, true), (2, false), (null, true), (null, false), (0, null), (1, null), (null, null) ).map { case (i, b) => (i.asInstanceOf[Integer], b.asInstanceOf[java.lang.Boolean]) } data.toDF("i", "b").registerTempTable("t") try { // checkAnswer calls } finally { sqlCtx.dropTempTable("t") }

SparkQA · 2015-05-30T10:55:12Z

Test build #33806 has finished for PR 6505 at commit fc0d741.

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

liancheng · 2015-05-30T12:23:00Z

sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala

@@ -1331,4 +1331,26 @@ class SQLQuerySuite extends QueryTest with BeforeAndAfterAll {

    checkAnswer(sql("SELECT a.`c.b`, `b.$q`[0].`a@!.q`, `q.w`.`w.i&`[0] FROM t"), Row(1, 1, 1))
  }
+
+  test("SPARK-7952: fix the equality check between boolean type and numeric typegd") {


liancheng · 2015-05-30T12:45:35Z

LGTM except for a few minor issues. Thanks for fixing this!

cloud-fan · 2015-05-30T14:34:52Z

Hi @liancheng , do we need to drop temp tables at last? If so, we need to change a lot of test cases in SQLQuerySuite.

liancheng · 2015-05-30T14:46:49Z

@cloud-fan Unlike persisted tables, existing temp tables can be overriden without triggering any error. But in general it's a better practice to clean up the scene. That's why we made SQLTestUtils and provide a bunch of withXxx methods to help automate these boilerplate code. For example, you may make SQLQuerySuite extend SQLTestUtils and use withTempTable("t") { ... } to wrap your test code in the newly added test case.

liancheng · 2015-05-30T14:50:22Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/HiveTypeCoercion.scala

+      CaseWhen(Seq(
+        Or(IsNull(booleanExpr), IsNull(numericExpr)), Literal.create(null, BooleanType),
+        buildCaseKeyWhen(booleanExpr, numericExpr)
+      ))


Seems that we can replace this CaseKeyWhen with an If?

SparkQA · 2015-05-30T16:23:52Z

Test build #33809 has finished for PR 6505 at commit 9ba2130.

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

SparkQA · 2015-05-30T16:47:34Z

Test build #33810 has finished for PR 6505 at commit 625973c.

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

SparkQA · 2015-05-30T17:03:02Z

Test build #33811 has finished for PR 6505 at commit ebc8c61.

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

rxin · 2015-05-30T20:56:15Z

sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala

+  test("SPARK-7952: fix the equality check between boolean and numeric types") {
+    withTempTable("t") {
+      Seq(
+        (1, true),


this test case is too confusing as it is.

Maybe you can have tuple4, where the 3rd element is the comparison value for =, and 4th element is the value for <=>. You can also just define the type in the Seq itself, i.e.

Seq[(java.lang.Integer, java.lang.Boolean, java.lang.Boolean)]( (0, false, true, true) ... )

yhuai · 2015-05-30T21:21:49Z

Can you give some examples to explain what is the problem?

SparkQA · 2015-05-31T18:58:01Z

Test build #33857 has finished for PR 6505 at commit b6401ba.

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

SparkQA · 2015-05-31T21:31:54Z

Test build #33863 has finished for PR 6505 at commit 77f0f39.

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

cloud-fan · 2015-06-01T02:49:04Z

Is it ready to go? cc @rxin @liancheng @yhuai

yhuai · 2015-06-01T02:57:31Z

Can you update the description to explain what is the problem with more information (e.g. a case that we do not handle correctly)?

cloud-fan · 2015-06-01T03:49:15Z

@yhuai , updated.

liancheng · 2015-06-01T04:01:01Z

LGTM, thanks for working on this!

rxin · 2015-06-01T04:01:45Z

Merged in master.

rxin · 2015-06-01T04:02:00Z

@marmbrus if you have time would be great to take a look at this too.

marmbrus · 2015-06-01T04:07:28Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/HiveTypeCoercion.scala

-    val trueValues = Seq(1, 1L, 1.toByte, 1.toShort, new java.math.BigDecimal(1)).map(Literal(_))
-    val falseValues = Seq(0, 0L, 0.toByte, 0.toShort, new java.math.BigDecimal(0)).map(Literal(_))
+  object BooleanEqualization extends Rule[LogicalPlan] {
+    private val trueValues = Seq(1.toByte, 1.toShort, 1, 1L, new java.math.BigDecimal(1))


Shouldn't this be a SQL internal Decimal type?

We should probably also add a test for this case since it seem broken ATM.

Good catch. This breaks for Decimal. We should probably compare Literal itself, instead of comparing the wrapped value for literals.

@cloud-fan can you fix this and add a test case? thanks.

This PR fixes a bug introduced in #6505. Decimal literal's value is not `java.math.BigDecimal`, but Spark SQL internal type: `Decimal`. Author: Wenchen Fan <cloud0fan@outlook.com> Closes #6574 from cloud-fan/fix and squashes the following commits: b0e3549 [Wenchen Fan] rename to BooleanEquality 1987b37 [Wenchen Fan] use Decimal instead of java.math.BigDecimal f93c420 [Wenchen Fan] compare literal

… numeric type is broken. The origin code has several problems: * `true <=> 1` will return false as we didn't set a rule to handle it. * `true = a` where `a` is not `Literal` and its value is 1, will return false as we only handle literal values. Author: Wenchen Fan <cloud0fan@outlook.com> Closes apache#6505 from cloud-fan/tmp1 and squashes the following commits: 77f0f39 [Wenchen Fan] minor fix b6401ba [Wenchen Fan] add type coercion for CaseKeyWhen and address comments ebc8c61 [Wenchen Fan] use SQLTestUtils and If 625973c [Wenchen Fan] improve 9ba2130 [Wenchen Fan] address comments fc0d741 [Wenchen Fan] fix style 2846a04 [Wenchen Fan] fix 7952

This PR fixes a bug introduced in apache#6505. Decimal literal's value is not `java.math.BigDecimal`, but Spark SQL internal type: `Decimal`. Author: Wenchen Fan <cloud0fan@outlook.com> Closes apache#6574 from cloud-fan/fix and squashes the following commits: b0e3549 [Wenchen Fan] rename to BooleanEquality 1987b37 [Wenchen Fan] use Decimal instead of java.math.BigDecimal f93c420 [Wenchen Fan] compare literal

cloud-fan reviewed May 29, 2015
View reviewed changes

cloud-fan force-pushed the tmp1 branch from a3f8e9c to 18211e7 Compare May 30, 2015 03:36

cloud-fan mentioned this pull request May 30, 2015

[SPARK-7562][SPARK-6444][SQL] Improve error reporting for expression data type mismatch #6405

Closed

cloud-fan added 2 commits May 30, 2015 16:51

fix 7952

2846a04

fix style

fc0d741

cloud-fan force-pushed the tmp1 branch from 18211e7 to fc0d741 Compare May 30, 2015 08:52

liancheng reviewed May 30, 2015
View reviewed changes

address comments

9ba2130

improve

625973c

liancheng reviewed May 30, 2015
View reviewed changes

use SQLTestUtils and If

ebc8c61

rxin reviewed May 30, 2015
View reviewed changes

liancheng mentioned this pull request May 31, 2015

[SQL] Simplifies binary node pattern matching #6537

Closed

add type coercion for CaseKeyWhen and address comments

b6401ba

cloud-fan changed the title ~~[SPARK-7952][SQL] equality check between boolean type and numeric type is broken.~~ [SPARK-7952][SPARK-7984][SQL] equality check between boolean type and numeric type is broken. May 31, 2015

minor fix

77f0f39

cloud-fan force-pushed the tmp1 branch from 90a82ef to 77f0f39 Compare May 31, 2015 19:33

asfgit closed this in a0e46a0 Jun 1, 2015

marmbrus reviewed Jun 1, 2015
View reviewed changes

cloud-fan mentioned this pull request Jun 2, 2015

[SPARK-7952][SQL] use internal Decimal instead of java.math.BigDecimal #6574

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-7952][SPARK-7984][SQL] equality check between boolean type and numeric type is broken. #6505

[SPARK-7952][SPARK-7984][SQL] equality check between boolean type and numeric type is broken. #6505

cloud-fan commented May 29, 2015

cloud-fan May 29, 2015

SparkQA commented May 29, 2015

SparkQA commented May 30, 2015

rxin commented May 30, 2015

rxin commented May 30, 2015

liancheng May 30, 2015

SparkQA commented May 30, 2015

liancheng May 30, 2015

liancheng commented May 30, 2015

cloud-fan commented May 30, 2015

liancheng commented May 30, 2015

liancheng May 30, 2015

SparkQA commented May 30, 2015

SparkQA commented May 30, 2015

SparkQA commented May 30, 2015

rxin May 30, 2015

yhuai commented May 30, 2015

SparkQA commented May 31, 2015

SparkQA commented May 31, 2015

cloud-fan commented Jun 1, 2015

yhuai commented Jun 1, 2015

cloud-fan commented Jun 1, 2015

liancheng commented Jun 1, 2015

rxin commented Jun 1, 2015

rxin commented Jun 1, 2015

marmbrus Jun 1, 2015

marmbrus Jun 1, 2015

rxin Jun 1, 2015

rxin Jun 1, 2015

[SPARK-7952][SPARK-7984][SQL] equality check between boolean type and numeric type is broken. #6505

[SPARK-7952][SPARK-7984][SQL] equality check between boolean type and numeric type is broken. #6505

Conversation

cloud-fan commented May 29, 2015

Choose a reason for hiding this comment

SparkQA commented May 29, 2015

SparkQA commented May 30, 2015

rxin commented May 30, 2015

rxin commented May 30, 2015

Choose a reason for hiding this comment

SparkQA commented May 30, 2015

Choose a reason for hiding this comment

liancheng commented May 30, 2015

cloud-fan commented May 30, 2015

liancheng commented May 30, 2015

Choose a reason for hiding this comment

SparkQA commented May 30, 2015

SparkQA commented May 30, 2015

SparkQA commented May 30, 2015

Choose a reason for hiding this comment

yhuai commented May 30, 2015

SparkQA commented May 31, 2015

SparkQA commented May 31, 2015

cloud-fan commented Jun 1, 2015

yhuai commented Jun 1, 2015

cloud-fan commented Jun 1, 2015

liancheng commented Jun 1, 2015

rxin commented Jun 1, 2015

rxin commented Jun 1, 2015

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