alamb commented on code in PR #15680:
URL: https://github.com/apache/datafusion/pull/15680#discussion_r2045605667
##########
datafusion/sql/src/parser.rs:
##########
@@ -356,9 +355,12 @@ impl<'a> DFParserBuilder<'a> {
self
}
- pub fn build(self) -> Result<DFParser<'a>, ParserError> {
+ pub fn build(self) -> Result<DFParser<'a>, DataFusionError> {
let mut tokenizer = Tokenizer::new(self.dialect, self.sql);
- let tokens = tokenizer.tokenize_with_location()?;
+ // Convert TokenizerError -> ParserError
+ let tokens = tokenizer
+ .tokenize_with_location()
+ .map_err(|e| ParserError::TokenizerError(e.to_string()))?;
Review Comment:
this is weird that `TokenizerError` is different than
`ParserError::TokenizerErrir` and that this is the right way to convert between
them. Nothing to change in this PR, maybe something to make nicer . Ideally I
would love to see something like:
```rust
.map_err(ParserError::From)
```
##########
datafusion/sql/src/parser.rs:
##########
@@ -561,17 +577,13 @@ impl<'a> DFParser<'a> {
if token == Token::EOF || token == Token::SemiColon {
break;
} else {
- return Err(ParserError::ParserError(format!(
- "Unexpected token {token}"
- )));
+ return parser_err!(format!("Unexpected token {token}"))?;
}
}
}
let Some(target) = builder.target else {
- return Err(ParserError::ParserError(
- "Missing TO clause in COPY statement".into(),
- ));
+ return parser_err!(format!("Missing TO clause in COPY
statement"))?;
Review Comment:
nice cleanups
##########
datafusion/sql/src/parser.rs:
##########
@@ -561,17 +577,13 @@ impl<'a> DFParser<'a> {
if token == Token::EOF || token == Token::SemiColon {
break;
} else {
- return Err(ParserError::ParserError(format!(
- "Unexpected token {token}"
- )));
+ return parser_err!(format!("Unexpected token {token}"))?;
Review Comment:
The parser_err macro already handles the string interpolation so you can do
this insted:
```suggestion
return parser_err!("Unexpected token {token}")?;
```
(There are a few other examples of this below)
##########
datafusion/sql/src/parser.rs:
##########
@@ -438,12 +440,26 @@ impl<'a> DFParser<'a> {
&self,
expected: &str,
found: TokenWithSpan,
- ) -> Result<T, ParserError> {
- parser_err!(format!("Expected {expected}, found: {found}"))
+ ) -> Result<T, DataFusionError> {
+ let sql_parser_span = found.span;
+ parser_err!(format!(
+ "Expected: {expected}, found: {found}{}",
+ found.span.start
+ ))
+ .map_err(|e| {
+ let e: DataFusionError = e.into();
Review Comment:
Here is an alternate syntax that I find more explicit, but totally uneeded
```suggestion
let e = DataFusionError::from(e);
```
--
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.
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org
