-
Notifications
You must be signed in to change notification settings - Fork 28.9k
[SPARK-5213] [SQL] Pluggable SQL Parser Support #5827
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.
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
Conversation
|
Test build #31518 has finished for PR 5827 at commit
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this comment? It's probably confused once this PR merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
|
Thank you @scwf for the fixing. :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chenghao-intel this sqlparser actually will not be used for now, place here just to fix mima test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we'd better keep it, not just for the mima test, but also for the sub class of Dialect. e.g. we have to specify the SparkSQLParser for HiveQLDialect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree to keep it, and in dialect parser we should not use SparkSQLParser. Dialect give a fallback(string -> logicalplan) and we call it in sqlParser
|
Retest this please |
|
Test build #31566 has finished for PR 5827 at commit
|
|
Test build #31573 timed out for PR 5827 at commit |
|
retest this please |
|
Test build #31605 has finished for PR 5827 at commit
|
|
@marmbrus any comment here? |
This is a follow up of #5827 to remove the additional `SparkSQLParser` Author: Cheng Hao <[email protected]> Closes #5965 from chenghao-intel/remove_sparksqlparser and squashes the following commits: 509a233 [Cheng Hao] Remove the HiveQlQueryExecution a5f9e3b [Cheng Hao] Remove the duplicated SparkSQLParser (cherry picked from commit 074d75d) Signed-off-by: Michael Armbrust <[email protected]>
This is a follow up of #5827 to remove the additional `SparkSQLParser` Author: Cheng Hao <[email protected]> Closes #5965 from chenghao-intel/remove_sparksqlparser and squashes the following commits: 509a233 [Cheng Hao] Remove the HiveQlQueryExecution a5f9e3b [Cheng Hao] Remove the duplicated SparkSQLParser
based on apache#4015, we should not delete `sqlParser` from sqlcontext, that leads to mima failed. Users implement dialect to give a fallback for `sqlParser` and we should construct `sqlParser` in sqlcontext according to the dialect `protected[sql] val sqlParser = new SparkSQLParser(getSQLDialect().parse(_))` Author: Cheng Hao <[email protected]> Author: scwf <[email protected]> Closes apache#5827 from scwf/sqlparser1 and squashes the following commits: 81b9737 [scwf] comment fix 0878bd1 [scwf] remove comments c19780b [scwf] fix mima tests c2895cf [scwf] Merge branch 'master' of https://github.com/apache/spark into sqlparser1 493775c [Cheng Hao] update the code as feedback 81a731f [Cheng Hao] remove the unecessary comment aab0b0b [Cheng Hao] polish the code a little bit 49b9d81 [Cheng Hao] shrink the comment for rebasing
This is a follow up of apache#5827 to remove the additional `SparkSQLParser` Author: Cheng Hao <[email protected]> Closes apache#5965 from chenghao-intel/remove_sparksqlparser and squashes the following commits: 509a233 [Cheng Hao] Remove the HiveQlQueryExecution a5f9e3b [Cheng Hao] Remove the duplicated SparkSQLParser
based on apache#4015, we should not delete `sqlParser` from sqlcontext, that leads to mima failed. Users implement dialect to give a fallback for `sqlParser` and we should construct `sqlParser` in sqlcontext according to the dialect `protected[sql] val sqlParser = new SparkSQLParser(getSQLDialect().parse(_))` Author: Cheng Hao <[email protected]> Author: scwf <[email protected]> Closes apache#5827 from scwf/sqlparser1 and squashes the following commits: 81b9737 [scwf] comment fix 0878bd1 [scwf] remove comments c19780b [scwf] fix mima tests c2895cf [scwf] Merge branch 'master' of https://github.com/apache/spark into sqlparser1 493775c [Cheng Hao] update the code as feedback 81a731f [Cheng Hao] remove the unecessary comment aab0b0b [Cheng Hao] polish the code a little bit 49b9d81 [Cheng Hao] shrink the comment for rebasing
This is a follow up of apache#5827 to remove the additional `SparkSQLParser` Author: Cheng Hao <[email protected]> Closes apache#5965 from chenghao-intel/remove_sparksqlparser and squashes the following commits: 509a233 [Cheng Hao] Remove the HiveQlQueryExecution a5f9e3b [Cheng Hao] Remove the duplicated SparkSQLParser
based on apache#4015, we should not delete `sqlParser` from sqlcontext, that leads to mima failed. Users implement dialect to give a fallback for `sqlParser` and we should construct `sqlParser` in sqlcontext according to the dialect `protected[sql] val sqlParser = new SparkSQLParser(getSQLDialect().parse(_))` Author: Cheng Hao <[email protected]> Author: scwf <[email protected]> Closes apache#5827 from scwf/sqlparser1 and squashes the following commits: 81b9737 [scwf] comment fix 0878bd1 [scwf] remove comments c19780b [scwf] fix mima tests c2895cf [scwf] Merge branch 'master' of https://github.com/apache/spark into sqlparser1 493775c [Cheng Hao] update the code as feedback 81a731f [Cheng Hao] remove the unecessary comment aab0b0b [Cheng Hao] polish the code a little bit 49b9d81 [Cheng Hao] shrink the comment for rebasing
This is a follow up of apache#5827 to remove the additional `SparkSQLParser` Author: Cheng Hao <[email protected]> Closes apache#5965 from chenghao-intel/remove_sparksqlparser and squashes the following commits: 509a233 [Cheng Hao] Remove the HiveQlQueryExecution a5f9e3b [Cheng Hao] Remove the duplicated SparkSQLParser
|
@scwf are you guys using this feature? I'm thinking about just removing it in Spark 2.0. @chenghao-intel who wanted it in the first place no longer needs it. |
|
@rxin, yes we used this and we implements a new sqlparser based on this interface to support ANSI tpcds sql. |
|
What's different from the one in Spark master now? It would be great to contribute the parser changes back now we have a full fledged parser in Spark, and going towards more ANSI compatibility is definitely on the roadmap. |
|
@rxin Our parser is a extended version of the I noticed that there are some PRs for these features, i will take a look at that PRs when i have time and see what i can do. |
|
FYI we are going to remove this pluggability. It is extra overhead to maintain, and actually encourages projects to not contribute their improvements upstream, which is bad. |
|
Actually we were trying to contribute this improvements, unfortunately the community do not want them for maintain(or compatibility with hive ql) reason in the past:). I am glad that spark sql use a single parser such that people can make contributions and make it more and more powerful. |
|
Yup thanks. That's why we are only removing it now :) |
based on #4015, we should not delete
sqlParserfrom sqlcontext, that leads to mima failed. Users implement dialect to give a fallback forsqlParserand we should constructsqlParserin sqlcontext according to the dialectprotected[sql] val sqlParser = new SparkSQLParser(getSQLDialect().parse(_))