[SPARK-18058][SQL] Comparing column types ignoring Nullability in Union and SetOperation #15595

CodingCat · 2016-10-22T01:06:54Z

What changes were proposed in this pull request?

The PR tries to fix SPARK-18058 which refers to a bug that the column types are compared with the extra care about Nullability in Union and SetOperation.

This PR converts the columns types by setting all fields as nullable before comparison

How was this patch tested?

regular unit test cases

SparkQA · 2016-10-22T03:20:32Z

Test build #67364 has finished for PR 15595 at commit c62f84f.

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

SparkQA · 2016-10-22T03:51:33Z

Test build #67367 has finished for PR 15595 at commit 4a79782.

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

HyukjinKwon · 2016-10-22T14:59:47Z

@CodingCat I think we need a test maybe (at least the one specified in the JIRA) to confirm this PR fixes that problem and avoid the regression in the future.

hvanhovell · 2016-10-22T15:20:30Z

+1 on adding a test.

SparkQA · 2016-10-23T00:33:36Z

Test build #67397 has finished for PR 15595 at commit e7b5a9b.

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

tejasapatil · 2016-10-23T01:39:15Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/AnalysisSuite.scala

@@ -377,4 +377,14 @@ class AnalysisSuite extends AnalysisTest {
    assertExpressionType(sum(Divide(Decimal(1), 2.0)), DoubleType)
    assertExpressionType(sum(Divide(1.0, Decimal(2.0))), DoubleType)
  }
+


nit: delete extra newline

tejasapatil · 2016-10-23T01:43:21Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/AnalysisSuite.scala

@@ -377,4 +377,14 @@ class AnalysisSuite extends AnalysisTest {
    assertExpressionType(sum(Divide(Decimal(1), 2.0)), DoubleType)
    assertExpressionType(sum(Divide(1.0, Decimal(2.0))), DoubleType)
  }
+
+
+  test("SPARK-18058: union operations shall not care about the nullability of columns") {


This PR also affects SetOperation. Could you please also add tests for that ?

+1 (actually, it'd be nicer if it has a end-to-end test as well).

SparkQA · 2016-10-23T15:57:41Z

Test build #67416 has finished for PR 15595 at commit e691714.

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

hvanhovell · 2016-10-23T17:43:17Z

LGTM - merging to master. Thanks!

hvanhovell · 2016-10-23T17:44:30Z

@CodingCat could you open a backport for branch-2.0?

CodingCat · 2016-10-23T17:44:50Z

sure, doing that now

hvanhovell · 2016-10-23T17:45:44Z

Thanks!

tejasapatil · 2016-10-23T17:48:47Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/AnalysisSuite.scala

+    val unionPlan = Union(firstTable, secondTable)
+    assertAnalysisSuccess(unionPlan)
+
+    val r1 = Except(firstTable, secondTable)


nit: this var could have been named better.

Actually, you could have gotten rid of the vars by doing :

assertAnalysisSuccess(Union(firstTable, secondTable)) assertAnalysisSuccess(Except(firstTable, secondTable)) assertAnalysisSuccess(Intersect(firstTable, secondTable))

nice catch, I have followed this in the backport version of the patch

tejasapatil · 2016-10-23T17:50:06Z

Just saw that this got merged. I had a tiny nit but its ok without it.

…on and SetOperation ## What changes were proposed in this pull request? The PR tries to fix [SPARK-18058](https://issues.apache.org/jira/browse/SPARK-18058) which refers to a bug that the column types are compared with the extra care about Nullability in Union and SetOperation. This PR converts the columns types by setting all fields as nullable before comparison ## How was this patch tested? regular unit test cases Author: CodingCat <zhunansjtu@gmail.com> Closes apache#15595 from CodingCat/SPARK-18058.

…lity on asNullable datatypes ## What changes were proposed in this pull request? This is absolutely minor. PR #15595 uses `dt1.asNullable == dt2.asNullable` expressions in a few places. It is however more efficient to call `dt1.sameType(dt2)`. I have replaced every instance of the first pattern with the second pattern (3/5 were introduced by #15595). ## How was this patch tested? Existing tests. Author: Herman van Hovell <hvanhovell@databricks.com> Closes #16041 from hvanhovell/SPARK-18058.

…lity on asNullable datatypes ## What changes were proposed in this pull request? This is absolutely minor. PR #15595 uses `dt1.asNullable == dt2.asNullable` expressions in a few places. It is however more efficient to call `dt1.sameType(dt2)`. I have replaced every instance of the first pattern with the second pattern (3/5 were introduced by #15595). ## How was this patch tested? Existing tests. Author: Herman van Hovell <hvanhovell@databricks.com> Closes #16041 from hvanhovell/SPARK-18058. (cherry picked from commit d449988) Signed-off-by: Reynold Xin <rxin@databricks.com>

…lity on asNullable datatypes ## What changes were proposed in this pull request? This is absolutely minor. PR apache#15595 uses `dt1.asNullable == dt2.asNullable` expressions in a few places. It is however more efficient to call `dt1.sameType(dt2)`. I have replaced every instance of the first pattern with the second pattern (3/5 were introduced by apache#15595). ## How was this patch tested? Existing tests. Author: Herman van Hovell <hvanhovell@databricks.com> Closes apache#16041 from hvanhovell/SPARK-18058.

…on and SetOperation ## What changes were proposed in this pull request? The PR tries to fix [SPARK-18058](https://issues.apache.org/jira/browse/SPARK-18058) which refers to a bug that the column types are compared with the extra care about Nullability in Union and SetOperation. This PR converts the columns types by setting all fields as nullable before comparison ## How was this patch tested? regular unit test cases Author: CodingCat <zhunansjtu@gmail.com> Closes apache#15595 from CodingCat/SPARK-18058.

…lity on asNullable datatypes ## What changes were proposed in this pull request? This is absolutely minor. PR apache#15595 uses `dt1.asNullable == dt2.asNullable` expressions in a few places. It is however more efficient to call `dt1.sameType(dt2)`. I have replaced every instance of the first pattern with the second pattern (3/5 were introduced by apache#15595). ## How was this patch tested? Existing tests. Author: Herman van Hovell <hvanhovell@databricks.com> Closes apache#16041 from hvanhovell/SPARK-18058.

CodingCat added 3 commits October 21, 2016 20:48

improve the doc for "spark.memory.offHeap.size"

0e38e28

fix

c714bb4

compare the column types ignoring the nullability

c62f84f

CodingCat changed the title ~~[SPARK-18058][SQL] Comparing column types ignoring Nullability in BinaryOperator~~ [SPARK-18058][SQL] Comparing column types ignoring Nullability in Union and SetOperation Oct 22, 2016

fix typos

4a79782

add test case

e7b5a9b

tejasapatil reviewed Oct 23, 2016

View reviewed changes

add test for setoperations

e691714

asfgit closed this in a81fba0 Oct 23, 2016

tejasapatil reviewed Oct 23, 2016

View reviewed changes

hvanhovell mentioned this pull request Nov 28, 2016

[SPARK-18058][SQL][TRIVIAL] Use dataType.sameResult(...) instead equality on asNullable datatypes #16041

Closed

CodingCat deleted the SPARK-18058 branch December 30, 2020 08:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-18058][SQL] Comparing column types ignoring Nullability in Union and SetOperation #15595

[SPARK-18058][SQL] Comparing column types ignoring Nullability in Union and SetOperation #15595

CodingCat commented Oct 22, 2016 •

edited

SparkQA commented Oct 22, 2016

SparkQA commented Oct 22, 2016

HyukjinKwon commented Oct 22, 2016

hvanhovell commented Oct 22, 2016

SparkQA commented Oct 23, 2016

tejasapatil Oct 23, 2016

tejasapatil Oct 23, 2016

HyukjinKwon Oct 23, 2016 •

edited

SparkQA commented Oct 23, 2016

hvanhovell commented Oct 23, 2016

hvanhovell commented Oct 23, 2016

CodingCat commented Oct 23, 2016

hvanhovell commented Oct 23, 2016

tejasapatil Oct 23, 2016

CodingCat Oct 23, 2016

tejasapatil commented Oct 23, 2016

[SPARK-18058][SQL] Comparing column types ignoring Nullability in Union and SetOperation #15595

[SPARK-18058][SQL] Comparing column types ignoring Nullability in Union and SetOperation #15595

Conversation

CodingCat commented Oct 22, 2016 • edited

What changes were proposed in this pull request?

How was this patch tested?

SparkQA commented Oct 22, 2016

SparkQA commented Oct 22, 2016

HyukjinKwon commented Oct 22, 2016

hvanhovell commented Oct 22, 2016

SparkQA commented Oct 23, 2016

tejasapatil Oct 23, 2016

Choose a reason for hiding this comment

tejasapatil Oct 23, 2016

Choose a reason for hiding this comment

HyukjinKwon Oct 23, 2016 • edited

Choose a reason for hiding this comment

SparkQA commented Oct 23, 2016

hvanhovell commented Oct 23, 2016

hvanhovell commented Oct 23, 2016

CodingCat commented Oct 23, 2016

hvanhovell commented Oct 23, 2016

tejasapatil Oct 23, 2016

Choose a reason for hiding this comment

CodingCat Oct 23, 2016

Choose a reason for hiding this comment

tejasapatil commented Oct 23, 2016

CodingCat commented Oct 22, 2016 •

edited

HyukjinKwon Oct 23, 2016 •

edited