[SPARK-4699][SQL] make caseSensitive configurable in Analyzer.scala

[jackylk]

No description provided.

[marmbrus]

Thanks for working on this! What about HiveContext?

[marmbrus]

ok to test

[SparkQA]

Test build #24051 has finished for PR 3558 at commit 578d167.

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

[chenghao-intel]

I think we probably have to always keep the HiveContext analyzer case INSENSITIVE, See https://cwiki.apache.org/confluence/display/Hive/LanguageManual+DDL, Hive will always normalizes the table/view names, columns etc. in DDL actually.

[chenghao-intel]

The change LGTM.

[jackylk]

@chenghao-intel agreed.
Besides, I think the caseSensitive passing to SimpleCatalog also needs to be changed
here: https://github.com/apache/spark/blob/master/sql/core/src/main/scala/org/apache/spark/sql/SQLContext.scala#L61

[chenghao-intel]

Great, you're right, please make the change.

[chenghao-intel]

And it will be nice if you can also add test suite for this.

[jackylk]

testcase added. Since analyzer's caseSensitive variable can't be modified after initialization, I am creating another test suite instead of using SQLQuerySuite.

[SparkQA]

Test build #24096 has finished for PR 3558 at commit f57f15c.

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

[chenghao-intel]

Yeah, that's problem, we probably has 2 ways for this:

• Make the analyzer as function, not lazy val in SqlContext
• Take the SqlContext or SqlConfas parameter in constructing Analyzer, instead of the caseSensetive

I prefer the second way, what do you think @marmbrus @jackylk ?

[scwf]

The second way +1, because maybe future we need other configs in analyzer

[jackylk]

If we go for second way, it will create cyclic dependency between spark-catalyst and spark-sql sub-projects, because SQLConf and SQLContext is in spark-sql while Analyzer is in spark-catalyst.
I think the current way the only drawback is that caseSensitive can only be set while initializing SQLContext, but can not be set after initialization. If client want to use case insensitive analyzer, he need to create a new SQLContext, which I think it is probably OK.
I tested this way locally and it is passing SQLQuerySuite, I do not know why test case failed, as I can not access Jenkins test report now. Can anyone trigger Jenkins again, thanks.
And any more suggestion for better solution?

[chenghao-intel]

That's a good point, Can we make another interface that SqlConf inherit from? Or can we move the SqlConf into the module Catalyst? Any other idea?

[marmbrus]

I like the idea of having a configuration interface in catalyst that SQLConf can inherit from. We can then pass that into the analyzer.

I do want the HiveContext to be configurable as well since it should be a super set of what you can do with SQLContext. By default it should be case insensitive to match Hive.

[SparkQA]

Test build #24671 has finished for PR 3558 at commit 91b1b96.

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

[SparkQA]

Test build #24672 has finished for PR 3558 at commit 210915f.

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

[SparkQA]

Test build #24674 has finished for PR 3558 at commit e7bca31.

• This patch fails Scala style tests.
• This patch merges cleanly.
• This patch adds the following public classes (experimental):
  • class SimpleCatalog(val conf: CatalystConf) extends Catalog

[jackylk]

modified according to @marmbrus, a CatalystConf trait is added and used in Analyzer and Catalog

[SparkQA]

Test build #24675 has finished for PR 3558 at commit fcbf0d9.

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

[SparkQA]

Test build #24673 has finished for PR 3558 at commit 578d167.

• This patch fails Spark unit tests.
• This patch does not merge cleanly.
• This patch adds no public classes.

[marmbrus]

ping

[SparkQA]

Test build #25011 has finished for PR 3558 at commit 8753610.

• This patch fails Spark unit tests.
• This patch merges cleanly.
• This patch adds the following public classes (experimental):
  • class SimpleCatalog(val conf: CatalystConf) extends Catalog

[SparkQA]

Test build #25013 has finished for PR 3558 at commit 005c56d.

• This patch fails Spark unit tests.
• This patch merges cleanly.
• This patch adds the following public classes (experimental):
  • class SimpleCatalog(val conf: CatalystConf) extends Catalog

[SparkQA]

Test build #25014 has finished for PR 3558 at commit 9bf4cc7.

• This patch fails Spark unit tests.
• This patch merges cleanly.
• This patch adds the following public classes (experimental):
  • class SimpleCatalog(val conf: CatalystConf) extends Catalog

[SparkQA]

Test build #25015 has finished for PR 3558 at commit 73c16b1.

• This patch fails Spark unit tests.
• This patch merges cleanly.
• This patch adds the following public classes (experimental):
  • class SimpleCatalog(val conf: CatalystConf) extends Catalog

[marmbrus]

I think this should probably go in after #3965. We can have some parent trait in catalyst that the SQL version can extend. That way we can just pass the configuration we use in SQL back to catalyst.

[jackylk]

I have updated the code based on SPARK-5168 (#3965)

[SparkQA]

Test build #25751 has finished for PR 3558 at commit 05b09a3.

• This patch fails Spark unit tests.
• This patch merges cleanly.
• This patch adds the following public classes (experimental):
  • class SimpleCatalog(val conf: CatalystConf) extends Catalog

[SparkQA]

Test build #25755 has finished for PR 3558 at commit dee56e9.

• This patch fails Spark unit tests.
• This patch merges cleanly.
• This patch adds the following public classes (experimental):
  • class SimpleCatalog(val conf: CatalystConf) extends Catalog

[jackylk]

Please run the test again. I tested all sql and hive test case locally, and no error is reported. I am not sure why online test fails

[SparkQA]

Test build #26660 has finished for PR 3558 at commit 39e369c.

• This patch fails Spark unit tests.
• This patch merges cleanly.
• This patch adds the following public classes (experimental):
  • class SimpleCatalog(val conf: CatalystConf) extends Catalog

[SparkQA]

Test build #27815 has finished for PR 3558 at commit 12eca9a.

• This patch fails Spark unit tests.
• This patch merges cleanly.
• This patch adds the following public classes (experimental):
  • class SimpleCatalog(val conf: CatalystConf) extends Catalog

[SparkQA]

Test build #27816 has finished for PR 3558 at commit 664d1e9.

• This patch fails Spark unit tests.
• This patch merges cleanly.
• This patch adds the following public classes (experimental):
  • class SimpleCatalog(val conf: CatalystConf) extends Catalog

[marmbrus]

I think the catalyst conf can be as simple as a set of abstract methods that need to be implemented by some concrete conf.

trait CatalystConf {
  def caseSensitiveAnalysis: Boolean
}

We can have a trivial one for testing: case class SimpleConf(caseSensitiveAnalysis: Boolean) extends CatalystConf and SQLConf can mix this trait in as you are already doing.

[marmbrus]

Also, most interfaces in catalyst are not private since this package is not part of our standard compatibility guarantees.

[marmbrus]

ping.

Thanks for working on this! However, to keep the PR queue small, I propose we close this issue until you have time to update it.

[JoshRosen]

This was fixed by #5806, so I think we can close this issue / PR.