You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2020/08/08 05:32:07 UTC
[GitHub] [arrow] wqc200 opened a new pull request #7916: ARROW-9673: [Rust] [DataFusion] Add a param "dialect" for DFParser::parse_sql
wqc200 opened a new pull request #7916:
URL: https://github.com/apache/arrow/pull/7916
i use func 'DFParser::parse_sql' in outsie app with mysql dialect, but parser_sql use generic dialect default, we need to add a parameter so that we can pass in the dialect.
thanks.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] andygrove closed pull request #7916: ARROW-9673: [Rust] [DataFusion] Add a param "dialect" for DFParser::parse_sql
Posted by GitBox <gi...@apache.org>.
andygrove closed pull request #7916:
URL: https://github.com/apache/arrow/pull/7916
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] wqc200 closed pull request #7916: ARROW-9673: [Rust] [DataFusion] Add a param "dialect" for DFParser::parse_sql
Posted by GitBox <gi...@apache.org>.
wqc200 closed pull request #7916:
URL: https://github.com/apache/arrow/pull/7916
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] andygrove commented on pull request #7916: ARROW-9673: [Rust] [DataFusion] Add a param "dialect" for DFParser::parse_sql
Posted by GitBox <gi...@apache.org>.
andygrove commented on pull request #7916:
URL: https://github.com/apache/arrow/pull/7916#issuecomment-673476363
@wqc200 I noticed that you are using `git merge` and that won't work well on this project. You will need to rebase against master instead. I wrote up some notes about this that may be helpful: https://andygrove.io/apache_arrow_git_tips/
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] wqc200 commented on a change in pull request #7916: ARROW-9673: [Rust] [DataFusion] Add a param "dialect" for DFParser::parse_sql
Posted by GitBox <gi...@apache.org>.
wqc200 commented on a change in pull request #7916:
URL: https://github.com/apache/arrow/pull/7916#discussion_r470619153
##########
File path: rust/datafusion/src/sql/parser.rs
##########
@@ -77,20 +77,20 @@ pub struct DFParser {
impl DFParser {
/// Parse the specified tokens
- pub fn new(sql: &str) -> Result<Self, ParserError> {
- let dialect = GenericDialect {};
- let mut tokenizer = Tokenizer::new(&dialect, sql);
+ pub fn new(sql: &str, dialect: &dyn Dialect) -> Result<Self, ParserError> {
+ let mut tokenizer = Tokenizer::new(dialect, sql);
let tokens = tokenizer.tokenize()?;
Ok(DFParser {
parser: Parser::new(tokens),
})
}
/// Parse a SQL statement and produce a set of statements
- pub fn parse_sql(sql: &str) -> Result<Vec<Statement>, ParserError> {
- let mut tokenizer = Tokenizer::new(&GenericDialect {}, &sql);
- tokenizer.tokenize()?;
- let mut parser = DFParser::new(sql)?;
+ pub fn parse_sql(
+ sql: &str,
+ dialect: &dyn Dialect,
Review comment:
I think dialect is an important optional parameter and will only be used when initializing, so I think it should be entered in the entry
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] wqc200 commented on pull request #7916: ARROW-9673: [Rust] [DataFusion] Add a param "dialect" for DFParser::parse_sql
Posted by GitBox <gi...@apache.org>.
wqc200 commented on pull request #7916:
URL: https://github.com/apache/arrow/pull/7916#issuecomment-670973826
> @wqc200 please have a look at this, I think you reverted your changes out of the PR
Yes, because i find a error in cicd, but i don't konw why, so i revert, i want to know how the error cause.
I didn't change anything, but the error still happened.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] andygrove commented on pull request #7916: ARROW-9673: [Rust] [DataFusion] Add a param "dialect" for DFParser::parse_sql
Posted by GitBox <gi...@apache.org>.
andygrove commented on pull request #7916:
URL: https://github.com/apache/arrow/pull/7916#issuecomment-672910543
@wqc200 I think this is a good change to make. Please see my comments though.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] andygrove commented on a change in pull request #7916: ARROW-9673: [Rust] [DataFusion] Add a param "dialect" for DFParser::parse_sql
Posted by GitBox <gi...@apache.org>.
andygrove commented on a change in pull request #7916:
URL: https://github.com/apache/arrow/pull/7916#discussion_r469307058
##########
File path: rust/datafusion/src/sql/parser.rs
##########
@@ -77,20 +77,20 @@ pub struct DFParser {
impl DFParser {
/// Parse the specified tokens
- pub fn new(sql: &str) -> Result<Self, ParserError> {
- let dialect = GenericDialect {};
- let mut tokenizer = Tokenizer::new(&dialect, sql);
+ pub fn new(sql: &str, dialect: &dyn Dialect) -> Result<Self, ParserError> {
+ let mut tokenizer = Tokenizer::new(dialect, sql);
let tokens = tokenizer.tokenize()?;
Ok(DFParser {
parser: Parser::new(tokens),
})
}
/// Parse a SQL statement and produce a set of statements
- pub fn parse_sql(sql: &str) -> Result<Vec<Statement>, ParserError> {
- let mut tokenizer = Tokenizer::new(&GenericDialect {}, &sql);
- tokenizer.tokenize()?;
- let mut parser = DFParser::new(sql)?;
+ pub fn parse_sql(
+ sql: &str,
+ dialect: &dyn Dialect,
Review comment:
Perhaps we should store the dialect in the DFParser struct so we don't have to pass it in here as well?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] wqc200 closed pull request #7916: ARROW-9673: [Rust] [DataFusion] Add a param "dialect" for DFParser::parse_sql
Posted by GitBox <gi...@apache.org>.
wqc200 closed pull request #7916:
URL: https://github.com/apache/arrow/pull/7916
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nevi-me commented on pull request #7916: ARROW-9673: [Rust] [DataFusion] Add a param "dialect" for DFParser::parse_sql
Posted by GitBox <gi...@apache.org>.
nevi-me commented on pull request #7916:
URL: https://github.com/apache/arrow/pull/7916#issuecomment-670967067
@wqc200 please have a look at this, I think you reverted your changes out of the PR
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] andygrove commented on a change in pull request #7916: ARROW-9673: [Rust] [DataFusion] Add a param "dialect" for DFParser::parse_sql
Posted by GitBox <gi...@apache.org>.
andygrove commented on a change in pull request #7916:
URL: https://github.com/apache/arrow/pull/7916#discussion_r471009928
##########
File path: rust/datafusion/src/sql/parser.rs
##########
@@ -77,20 +77,20 @@ pub struct DFParser {
impl DFParser {
/// Parse the specified tokens
- pub fn new(sql: &str) -> Result<Self, ParserError> {
- let dialect = GenericDialect {};
- let mut tokenizer = Tokenizer::new(&dialect, sql);
+ pub fn new(sql: &str, dialect: &dyn Dialect) -> Result<Self, ParserError> {
+ let mut tokenizer = Tokenizer::new(dialect, sql);
let tokens = tokenizer.tokenize()?;
Ok(DFParser {
parser: Parser::new(tokens),
})
}
/// Parse a SQL statement and produce a set of statements
- pub fn parse_sql(sql: &str) -> Result<Vec<Statement>, ParserError> {
- let mut tokenizer = Tokenizer::new(&GenericDialect {}, &sql);
- tokenizer.tokenize()?;
- let mut parser = DFParser::new(sql)?;
+ pub fn parse_sql(
+ sql: &str,
+ dialect: &dyn Dialect,
Review comment:
Actually, I had misread this. I had thought that `parse_sql` was a method that took `&self` but it does not, so I think this is fine.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #7916: ARROW-9673: [Rust] [DataFusion] Add a param "dialect" for DFParser::parse_sql
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #7916:
URL: https://github.com/apache/arrow/pull/7916#issuecomment-670828493
https://issues.apache.org/jira/browse/ARROW-9673
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] andygrove commented on a change in pull request #7916: ARROW-9673: [Rust] [DataFusion] Add a param "dialect" for DFParser::parse_sql
Posted by GitBox <gi...@apache.org>.
andygrove commented on a change in pull request #7916:
URL: https://github.com/apache/arrow/pull/7916#discussion_r469305811
##########
File path: rust/datafusion/src/sql/planner.rs
##########
@@ -815,7 +816,8 @@ mod tests {
fn logical_plan(sql: &str) -> Result<LogicalPlan> {
let planner = SqlToRel::new(MockSchemaProvider {});
- let ast = DFParser::parse_sql(&sql).unwrap();
+ let dialet = &GenericDialect {};
Review comment:
typo, should be `dialect`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] andygrove commented on a change in pull request #7916: ARROW-9673: [Rust] [DataFusion] Add a param "dialect" for DFParser::parse_sql
Posted by GitBox <gi...@apache.org>.
andygrove commented on a change in pull request #7916:
URL: https://github.com/apache/arrow/pull/7916#discussion_r469306474
##########
File path: rust/datafusion/src/sql/parser.rs
##########
@@ -77,20 +77,20 @@ pub struct DFParser {
impl DFParser {
/// Parse the specified tokens
- pub fn new(sql: &str) -> Result<Self, ParserError> {
- let dialect = GenericDialect {};
- let mut tokenizer = Tokenizer::new(&dialect, sql);
+ pub fn new(sql: &str, dialect: &dyn Dialect) -> Result<Self, ParserError> {
Review comment:
Rather than make this breaking change, let's add a new constructor named `with_dialect` instead.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org