[SPARK-24443][SQL] comparison should accept structurally-equal types #21470

cloud-fan · 2018-05-31T18:14:52Z

What changes were proposed in this pull request?

When comparing struct type values, it's a little hard to make the field names same. Simple queries like
SELECT (a, b) = (1, 2) FROM t may fail because the field names do not match.

In Postgres, when comparing struct type values, it will do safe type coercion and ignore field name difference

# create table t(i smallint, j smallint);
CREATE TABLE

# select Row(i, j) = Row(1, 1) from t;                              
 ?column? 
----------
 t
(1 row)

# select Row(i, j) < Row(1, 1) from t;
 ?column? 
----------
 f
(1 row)

# select Row(i, j) = Row(j, i) from t;
 ?column? 
----------
 t
(1 row)

This PR follows Postgres and accept structurally-equal types in comparison.

This also fixes the test failures in #21442 .

TODO:

check Hive and Presto.
think about array/map type. Postgres doesn't support type coercion for elements in array/map when comparison.

How was this patch tested?

new test cases.

cloud-fan · 2018-05-31T18:18:24Z

cc @gatorsmile @dbtsai @dongjoon-hyun @viirya

cloud-fan · 2018-05-31T18:21:26Z

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

-        sql("CREATE TABLE S(C struct<F:int>) USING parquet")
-        Seq(("c", "C"), ("C", "c"), ("c.f", "C.F"), ("C.F", "c.f")).foreach {
-          case (left, right) =>
-            checkAnswer(sql(s"SELECT * FROM t, S WHERE t.$left = S.$right"), Seq.empty)


this test case wants to test set operation, but here it's testing filter. the new tests should've covered it.

SparkQA · 2018-05-31T22:18:26Z

Test build #91355 has finished for PR 21470 at commit 3555e0f.

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

gatorsmile · 2018-05-31T23:38:29Z

sql/core/src/main/scala/org/apache/spark/sql/execution/joins/HashJoin.scala

-    require(leftKeys.map(_.dataType) == rightKeys.map(_.dataType),
-      "Join keys from two sides should have same types")
+    require(leftKeys.map(_.dataType).zip(rightKeys.map(_.dataType)).forall {
+      case (l, r) => BinaryOperator.sameType(l, r)


Do we have a test case to cover this?

I hit a test failure before changing this. This kind of check(assert, require) can only be hitten when there is a bug.

gatorsmile · 2018-05-31T23:40:36Z

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

-          }
-        }.getOrElse(b)  // If there is no applicable conversion, leave expression unchanged.
+      case b @ BinaryOperator(left, right)
+          if !BinaryOperator.sameType(left.dataType, right.dataType) =>


How about just change this line? The other changes in this file can be done later?

findTightestCommonType doesn't accept struct type with different filed names, so the other code are needed.

cloud-fan · 2018-06-01T00:44:24Z

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

+            var i = 0
+            var continue = fields1.length == fields2.length
+            while (i < len && continue) {
+              val commonType = findTightestCommonType(fields1(i).dataType, fields2(i).dataType)


if we don't want to support type coercion, we can change this line to use fields1(i).dataType == fields2(i).dataType

What about nested structs?

dongjoon-hyun · 2018-06-01T06:43:52Z

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

-        val m = intercept[AnalysisException] {
-          sql("SELECT * FROM t, S WHERE c = C")
-        }.message
-        assert(m.contains("cannot resolve '(t.`c` = S.`C`)' due to data type mismatch"))


Thank you for pinging me, @cloud-fan .

Since this removal is a real behavior change instead of new test coverage of comparator.sql, could you add a documentation for this?

viirya · 2018-06-01T07:15:21Z

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

+            var i = 0
+            var continue = fields1.length == fields2.length
+            while (i < len && continue) {
+              val commonType = findTightestCommonType(fields1(i).dataType, fields2(i).dataType)


What about nested structs?

viirya · 2018-06-01T07:17:39Z

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

+              })
+              val newRight = if (right.dataType == newRightST) right else Cast(right, newRightST)
+
+              if (b.inputType.acceptsType(newLeftST) && b.inputType.acceptsType(newRightST)) {


Is it possible b only accepts one side (e.g., only newLeftST) but doesn't accept other side?

eric-maynard · 2018-06-04T17:24:09Z

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

+            findTightestCommonType(left.dataType, right.dataType).map { commonType =>
+              if (b.inputType.acceptsType(commonType)) {
+                // If the expression accepts the tightest common type, cast to that.
+                val newLeft = if (left.dataType == commonType) left else Cast(left, commonType)


This ternary operation seems to crop up a few times in this PR. Maybe we can push it out into a method?

private def castIfNeeded(e: Expression, possibleType: DataType): Expression = { if (e.dataType == possibleType) data else Cast(e, possibleType) }

eric-maynard · 2018-06-04T17:27:52Z

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

+            val len = fields1.length
+            var i = 0
+            var continue = fields1.length == fields2.length
+            while (i < len && continue) {


This loop could be refactored functionally, e.g.

val commonTypes = (fields1 zip fields2).map(f => findTightestCommonType(f._1, f._2)) if (commonTypes.forall(_.isDefined)) { . . .

github-actions · 2020-01-11T00:06:22Z

We're closing this PR because it hasn't been updated in a while. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable.
If you'd like to revive this PR, please reopen it and ask a committer to remove the Stale tag!

comparison should accept structurally-equal types

3555e0f

cloud-fan commented May 31, 2018

View reviewed changes

gatorsmile reviewed May 31, 2018

View reviewed changes

cloud-fan commented Jun 1, 2018

View reviewed changes

dongjoon-hyun reviewed Jun 1, 2018

View reviewed changes

viirya reviewed Jun 1, 2018

View reviewed changes

eric-maynard reviewed Jun 4, 2018

View reviewed changes

dbtsai mentioned this pull request Jul 17, 2018

[SPARK-24402] [SQL] Optimize In expression when only one element in the collection or collection is empty #21797

Closed

dongjoon-hyun added the SQL label Jun 14, 2019

github-actions bot added the Stale label Jan 11, 2020

github-actions bot closed this Jan 12, 2020

[SPARK-24443][SQL] comparison should accept structurally-equal types #21470

[SPARK-24443][SQL] comparison should accept structurally-equal types #21470

Uh oh!

Conversation

cloud-fan commented May 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

cloud-fan commented May 31, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented May 31, 2018

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!

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!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jan 11, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

cloud-fan commented May 31, 2018 •

edited

Loading