Skip to content

[SPARK-11447][SQL] change NullType to StringType during binaryComparison between NullType and StringType #9720

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
kevinyu98 wants to merge 3 commits into from
Closed

[SPARK-11447][SQL] change NullType to StringType during binaryComparison between NullType and StringType #9720

kevinyu98 wants to merge 3 commits into from

Conversation

@kevinyu98
Copy link
Contributor

@kevinyu98 kevinyu98 commented Nov 15, 2015

During executing PromoteStrings rule, if one side of binaryComparison is StringType and the other side is not StringType, the current code will promote(cast) the StringType to DoubleType, and if the StringType doesn't contain the numbers, it will get null value. So if it is doing <=> (NULL-safe equal) with Null, it will not filter anything, caused the problem reported by this jira.

I proposal to the changes through this PR, can you review my code changes ?

This problem only happen for <=>, other operators works fine.

scala> val filteredDF = df.filter(df("column") > (new Column(Literal(null))))
filteredDF: org.apache.spark.sql.DataFrame = [column: string]

scala> filteredDF.show
+------+
|column|
+------+
+------+

scala> val filteredDF = df.filter(df("column") === (new Column(Literal(null))))
filteredDF: org.apache.spark.sql.DataFrame = [column: string]

scala> filteredDF.show
+------+
|column|
+------+
+------+

scala> df.registerTempTable("DF")

scala> sqlContext.sql("select * from DF where 'column' = NULL")
res27: org.apache.spark.sql.DataFrame = [column: string]

scala> res27.show
+------+
|column|
+------+
+------+

@cloud-fan
Copy link
Contributor

cloud-fan commented Nov 16, 2015

ok to test

cloud-fan
cloud-fan reviewed Nov 16, 2015
View reviewed changes
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/HiveTypeCoercion.scala
Copy link
Contributor

@cloud-fan cloud-fan Nov 16, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this rule looks weird to me, how about casting to tightest common type of left and right? cc @marmbrus @yhuai @nongli

Copy link
Contributor

@yhuai yhuai Nov 16, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems @liancheng added this change.

Copy link
Contributor

@liancheng liancheng Nov 16, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, actually I only simplified the original rule with conciser pattern matching in PR #6537 (here). Tracked down the history and it turned out that this rule had already been there ever since the very first commit of Spark SQL by @marmbrus :) (here).

Copy link
Contributor

@marmbrus marmbrus Nov 16, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The goal here was to mimic hive's type coercion rules. I think if you create a compatibility test like SELECT "0001" = 1 this rule is required (if its not then we could consider changing this).

Copy link
Contributor

@cloud-fan cloud-fan Nov 17, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hive do support SELECT "0001" = 1, however, I think this rule is too simple, how about using findTightestCommonTypeToString?

Copy link
Contributor

@cloud-fan cloud-fan Nov 17, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so, this rule will fire first and change the type to DoubleType.
btw I think it's a bad smell to have conflict rules, we should improve it and make sure it only handles cases that missed by ImplicitTypeCasts.

Copy link
Contributor Author

@kevinyu98 kevinyu98 Nov 17, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cloud-fan : do you want me to open a new jira to look into this? The new jira/pr will focus on the rules in PromoteStrings and ImplicitTypeCasts, as you suggested to reduce the redundant rules in PromoteStrings.

Copy link
Contributor

@yhuai yhuai Nov 17, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kevinyu98 I do not think that is really a problem for now. I think we do not need a jira for that right now.

Copy link
Contributor

@cloud-fan cloud-fan Nov 17, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kevinyu98 please hold off until you find something is broken by this and we have to fix it.

Copy link
Contributor Author

@kevinyu98 kevinyu98 Nov 17, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yhuai @cloud-fan : sure, I will not do that. I will try to run more testing to see if anything is broken.

@SparkQA
Copy link

SparkQA commented Nov 16, 2015

Test build #45967 has finished for PR 9720 at commit bb705ca.

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

@SparkQA
Copy link

SparkQA commented Nov 16, 2015

Test build #45974 has finished for PR 9720 at commit 5a5be06.

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

@cloud-fan
Copy link
Contributor

cloud-fan commented Nov 17, 2015

LGTM, thanks for working on this @kevinyu98

@kevinyu98
Copy link
Contributor Author

kevinyu98 commented Nov 17, 2015

@cloud-fan and @marmbrus @yhuai @nongli @liancheng : thanks for reviewing the fix.

@yhuai
Copy link
Contributor

yhuai commented Nov 17, 2015

Merging to master and branch 1.6.

asfgit pushed a commit that referenced this pull request Nov 17, 2015
@kevinyu98 @yhuai
[SPARK-11447][SQL] change NullType to StringType during binaryCompari…
113410c 
…son between NullType and StringType

During executing PromoteStrings rule, if one side of binaryComparison is StringType and the other side is not StringType, the current code will promote(cast) the StringType to DoubleType, and if the StringType doesn't contain the numbers, it will get null value. So if it is doing <=> (NULL-safe equal) with Null, it will not filter anything, caused the problem reported by this jira.

I proposal to the changes through this PR, can you review my code changes ?

This problem only happen for <=>, other operators works fine.

scala> val filteredDF = df.filter(df("column") > (new Column(Literal(null))))
filteredDF: org.apache.spark.sql.DataFrame = [column: string]

scala> filteredDF.show
+------+
|column|
+------+
+------+

scala> val filteredDF = df.filter(df("column") === (new Column(Literal(null))))
filteredDF: org.apache.spark.sql.DataFrame = [column: string]

scala> filteredDF.show
+------+
|column|
+------+
+------+

scala> df.registerTempTable("DF")

scala> sqlContext.sql("select * from DF where 'column' = NULL")
res27: org.apache.spark.sql.DataFrame = [column: string]

scala> res27.show
+------+
|column|
+------+
+------+

Author: Kevin Yu <qyu@us.ibm.com>

Closes #9720 from kevinyu98/working_on_spark-11447.

(cherry picked from commit e01865a)
Signed-off-by: Yin Huai <yhuai@databricks.com>
@asfgit asfgit closed this in e01865a Nov 17, 2015
clehene pushed a commit to clehene/spark that referenced this pull request Dec 2, 2015
@kevinyu98
[SPARK-11447][SQL] change NullType to StringType during binaryCompari…
9dfdbc3 
…son between NullType and StringType

During executing PromoteStrings rule, if one side of binaryComparison is StringType and the other side is not StringType, the current code will promote(cast) the StringType to DoubleType, and if the StringType doesn't contain the numbers, it will get null value. So if it is doing <=> (NULL-safe equal) with Null, it will not filter anything, caused the problem reported by this jira.

I proposal to the changes through this PR, can you review my code changes ?

This problem only happen for <=>, other operators works fine.

scala> val filteredDF = df.filter(df("column") > (new Column(Literal(null))))
filteredDF: org.apache.spark.sql.DataFrame = [column: string]

scala> filteredDF.show
+------+
|column|
+------+
+------+

scala> val filteredDF = df.filter(df("column") === (new Column(Literal(null))))
filteredDF: org.apache.spark.sql.DataFrame = [column: string]

scala> filteredDF.show
+------+
|column|
+------+
+------+

scala> df.registerTempTable("DF")

scala> sqlContext.sql("select * from DF where 'column' = NULL")
res27: org.apache.spark.sql.DataFrame = [column: string]

scala> res27.show
+------+
|column|
+------+
+------+

Author: Kevin Yu <qyu@us.ibm.com>

Closes apache#9720 from kevinyu98/working_on_spark-11447.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@kevinyu98 @cloud-fan @SparkQA @yhuai @marmbrus @liancheng