[Spark-20145] Fix range case insensitive bug in SQL #17487

samelamin · 2017-03-30T22:09:20Z

What changes were proposed in this pull request?

Range in SQL should be case insensitive

How was this patch tested?

unit test

…ase insensitive

gatorsmile · 2017-03-30T22:26:27Z

...lyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveTableValuedFunctions.scala

  override def apply(plan: LogicalPlan): LogicalPlan = plan resolveOperators {
    case u: UnresolvedTableValuedFunction if u.functionArgs.forall(_.resolved) =>
-      builtinFunctions.get(u.functionName) match {
+      builtinFunctions.get(u.functionName.toLowerCase) match {


Does the function name is case sensitive in Hive?

What do you mean? There are no table valued functions in Hive.

@samelamin you have to make this dependent on the case sensitivity setting of the analyzer. This means you will have to turn this object into case class that takes a SQLConf as its argument.

ok cool thanks @hvanhovell ill amend it over the weekend. Do you know how I can run a specific set of tests on SBT?

im getting garbage collection errors when I try to run using sbt test-only form http://spark.apache.org/developer-tools.html

For example,

build/sbt "catalyst/test-only org.apache.spark.sql.catalyst.optimizer.ColumnPruningSuite" build/sbt -Phive "hive/test-only org.apache.spark.sql.hive.execution.SQLQuerySuite"

@hvanhovell instead of creating a new case class, is there a way I can reuse the UnresolvedTableValuedFunction case class and just add in the SQLConf class?

Yeah, @gatorsmile is right. We don't do case sensitive resolution for functions. So you can ignore my comment, and undo the last change you have made.

So would you like me to revert the change to the case class?

Yes, sorry about that!

Ok I can revert but how will we test it since the apply method isn't being called by the tests. They new up an unresolvedTableValuedFunction? Unless we hardcore it in unresolvedTableValuedFunction to always be case insensitive then I don't know how else to do it?

hvanhovell · 2017-03-31T07:47:20Z

ok to test

SparkQA · 2017-03-31T09:30:47Z

Test build #75426 has finished for PR 17487 at commit ee15dc0.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

samelamin · 2017-04-02T00:15:06Z

based on comments from @hvanhovell I am depending on the case sensitivity setting of the analyser. That said I had to make the functionName a var to change the value to lower case which feels like a code smell to me. I am happy with suggestions to how I can improve on it

SparkQA · 2017-04-02T01:42:02Z

Test build #75453 has finished for PR 17487 at commit 407bdbf.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class UnresolvedTableValuedFunction(conf: SQLConf,

hvanhovell · 2017-04-02T12:39:23Z

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

 * }}}
 */
-case class UnresolvedTableValuedFunction(functionName: String, functionArgs: Seq[Expression])
+case class UnresolvedTableValuedFunction(conf: SQLConf,


@samelamin this is a moot point now, but try to avoid mutable state as much as you can. In this case it would have been better to pass the SQLConf class as a parameter to the ResolveTableValuedFunction, and deal with case-sensitivity there.

samelamin · 2017-04-02T12:44:04Z

Ok I was planning on doing that but unfortunately all the tests don't go through the resolveTableValuedFuntion but new up an UnTableValued. I totally agree with you with regards to state. What we can do is create a method that calculates the function name inside the unresolvedTableValuedFunction which then has the dependency on the conf. How does that sound?

SparkQA · 2017-04-02T14:17:54Z

Test build #75460 has finished for PR 17487 at commit ccbfcb3.

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

samelamin · 2017-04-02T16:37:32Z

Maybe @gatorsmile or @srowen have an idea. How can I test the case insensitive unresolvedTableValuedFunction without using a var for the functionName. I'm open to all ideas folks! introducing a var just for the tests feels inheritely incorrect.

samelamin · 2017-04-02T17:30:56Z

FYI I can squash these commits to clean up the history once we agree on how best to approach the testing

SparkQA · 2017-04-02T19:29:10Z

Test build #75462 has finished for PR 17487 at commit 8bce1da.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class UnresolvedTableValuedFunction(var functionName: String, functionArgs: Seq[Expression])

SparkQA · 2017-04-02T19:35:57Z

Test build #75463 has finished for PR 17487 at commit 6efb254.

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

gatorsmile · 2017-04-03T00:24:39Z

@samelamin IMHO, you do not need a unit test case. We can just add end-to-end test cases. See the code changes I made in my private branch. (BTW, it would be better if you add concise comments above the code change)

To generate the new result file after you add the new queries into the .sql files, you need to run the following command.

SPARK_GENERATE_GOLDEN_FILES=1 build/sbt "sql/test-only *SQLQueryTestSuite"

selamin · 2017-04-03T13:17:08Z

@gatorsmile thanks so much for that, i spent over 2 hours getting that sql file correct manually! what was frustrating was after all that i had to revert my changes 😢

Atleast I know this now so it wasnt a complete waste! Anyways I updated the code to remove the vars

How does it look now folks
cc @hvanhovell

SparkQA · 2017-04-03T15:01:20Z

Test build #75480 has finished for PR 17487 at commit a01aba6.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class UnresolvedTableValuedFunction(functionName: String, functionArgs: Seq[Expression])

SparkQA · 2017-04-03T15:35:58Z

Test build #75481 has finished for PR 17487 at commit 406eba8.

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

gatorsmile · 2017-04-03T15:45:19Z

LGTM cc @hvanhovell @cloud-fan

cloud-fan · 2017-04-03T18:24:33Z

LGTM

samelamin · 2017-04-03T20:17:45Z

perfect! Can it be merged then?

rxin · 2017-04-04T00:15:47Z

Merging in master.

samelamin added 2 commits March 30, 2017 22:59

add failing test

3d2a3ba

SPARK-20145: lower case value passed into map to make the statement c…

ee15dc0

…ase insensitive

samelamin changed the title ~~[Spark-20145] Make range case insensitive~~ [Spark-20145] Fix range case insensitive bug in SQL Mar 30, 2017

gatorsmile reviewed Mar 30, 2017

View reviewed changes

samelamin changed the title ~~[Spark-20145] Fix range case insensitive bug in SQL~~ [Spark-20145] [WIP] Fix range case insensitive bug in SQL Mar 31, 2017

use sql conf to get case sensitivity

407bdbf

samelamin changed the title ~~[Spark-20145] [WIP] Fix range case insensitive bug in SQL~~ [Spark-20145] Fix range case insensitive bug in SQL Apr 2, 2017

pass failing test

ccbfcb3

hvanhovell reviewed Apr 2, 2017

View reviewed changes

samelamin added 5 commits April 2, 2017 18:07

pass failing test (reverted from commit ccbfcb3)

e1541b4

use sql conf to get case sensitivity (reverted from commit 407bdbf)

de7b8a6

revert based on comments from pr

8bce1da

code cleanup

ad5780a

remove empty spaces

6efb254

remove vars and use actual SQL

a01aba6

remove unused test

406eba8

asfgit closed this in 58c9e6e Apr 4, 2017

[Spark-20145] Fix range case insensitive bug in SQL #17487

[Spark-20145] Fix range case insensitive bug in SQL #17487

Uh oh!

Conversation

samelamin commented Mar 30, 2017

What changes were proposed in this pull request?

How was this patch tested?

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!

samelamin Mar 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

hvanhovell Apr 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

hvanhovell commented Mar 31, 2017

Uh oh!

SparkQA commented Mar 31, 2017

Uh oh!

samelamin commented Apr 2, 2017

Uh oh!

SparkQA commented Apr 2, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samelamin commented Apr 2, 2017

Uh oh!

SparkQA commented Apr 2, 2017

Uh oh!

samelamin commented Apr 2, 2017

Uh oh!

samelamin commented Apr 2, 2017

Uh oh!

SparkQA commented Apr 2, 2017

Uh oh!

SparkQA commented Apr 2, 2017

Uh oh!

gatorsmile commented Apr 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

selamin commented Apr 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SparkQA commented Apr 3, 2017

Uh oh!

SparkQA commented Apr 3, 2017

Uh oh!

gatorsmile commented Apr 3, 2017

Uh oh!

cloud-fan commented Apr 3, 2017

Uh oh!

samelamin commented Apr 3, 2017

Uh oh!

rxin commented Apr 4, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

samelamin Mar 31, 2017 •

edited

Loading

hvanhovell Apr 2, 2017 •

edited

Loading

gatorsmile commented Apr 3, 2017 •

edited

Loading

selamin commented Apr 3, 2017 •

edited

Loading