Skip to content

Conversation

@wzhfy
Copy link
Contributor

@wzhfy wzhfy commented Oct 29, 2015

  1. def dialectClassName in HiveContext is unnecessary.
    In HiveContext, if conf.dialect == "hiveql", getSQLDialect() will return new HiveQLDialect(this);
    else it will use super.getSQLDialect(). Then in super.getSQLDialect(), it calls dialectClassName, which is overriden in HiveContext and still return super.dialectClassName.
    So we'll never reach the code "classOf[HiveQLDialect].getCanonicalName" of def dialectClassName in HiveContext.
  2. When we start bin/spark-sql, the default context is HiveContext, and the corresponding dialect is hiveql.
    However, if we type "set spark.sql.dialect;", the result is "sql", which is inconsistent with the actual dialect and is misleading. For example, we can use sql like "create table" which is only allowed in hiveql, but this dialect conf shows it's "sql".
    Although this problem will not cause any execution error, it's misleading to spark sql users. Therefore I think we should fix it.
    In this pr, while procesing “set spark.sql.dialect” in SetCommand, I use "conf.dialect" instead of "getConf()" for the case of key == SQLConf.DIALECT.key, so that it will return the right dialect conf.

@wzhfy
Copy link
Contributor Author

wzhfy commented Oct 29, 2015

I also delete def dialectClassName. It's useless because getSQLDialect() is enough for selecting dialect.

@davies
Copy link
Contributor

davies commented Oct 29, 2015

@wzhfy Currently, we can use sql as the dialog for HiveContext, but can't do this after the change. Is that the thing we want?

@wzhfy
Copy link
Contributor Author

wzhfy commented Oct 30, 2015

@davies My intention is to change the displayed dialect when spark-sql starts, not to set "hiveql" for HiveContext forever.

Here's my description of the problem in JIRA:
When we start bin/spark-sql, the default context is HiveContext, and the corresponding dialect is hiveql.
However, if we type "set spark.sql.dialect;", the result is "sql", which is inconsistent with the actual dialect and is misleading. For example, we can use sql like "create table" which is only allowed in hiveql, but this dialect conf shows it's "sql".
Although this problem will not cause any execution error, it's misleading to spark sql users. Therefore I think we should fix it.

After the change, we can still use "sql" as the dialect for HiveContext through "set spark.sql.dialect=sql". This is the same way as before to switch between the two dialects, right?
Now the conf.dialect in HiveContext will become sql. Because in SQLConf, def dialect = getConf(), and now the dialect in "settings" becomes "sql".

@wzhfy wzhfy changed the title [SPARK-11398][SQL] misleading dialect conf at the start of spark-sql [SPARK-11398][SQL] unnecessary def dialectClassName in HiveContext, and misleading dialect conf at the start of spark-sql Nov 3, 2015
@wzhfy
Copy link
Contributor Author

wzhfy commented Nov 3, 2015

@davies @liancheng I've updated the description of this problem, hoping to explain it better now.
Can you review this pr and authorize testing? thx.

@SparkQA
Copy link

SparkQA commented Nov 3, 2015

Test build #1972 has finished for PR 9349 at commit 6dec533.

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

@davies
Copy link
Contributor

davies commented Nov 3, 2015

@wzhfy The first part change is good (remove dialectClassName). But other one may introduce regression, when you have spark.sql.dialect sql in conf/spark-default.conf, you will still got hiveql in HiveContext. I think we should fix the issue while procesing set spark.sql.dialect, which should call conf.dialect, not conf.getConf.

@wzhfy
Copy link
Contributor Author

wzhfy commented Nov 4, 2015

@davies Thanks for the advice. The commit is updated, please check if that's what we want.

Btw, I think the cause of this problem is the inconsistency between the default dialect conf (sql) and the default sqlcontext (HiveContext), maybe we should fix that in the future.

@davies
Copy link
Contributor

davies commented Nov 4, 2015

@wzhfy The changes LGTM, could you update the description to reflect the changes?

@SparkQA
Copy link

SparkQA commented Nov 4, 2015

Test build #1975 has finished for PR 9349 at commit d15e14a.

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

@wzhfy
Copy link
Contributor Author

wzhfy commented Nov 5, 2015

@davies The description is updated

@davies
Copy link
Contributor

davies commented Nov 5, 2015

Merged into master, thanks!

@asfgit asfgit closed this in a752dda Nov 5, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants