Attach Diagnostic to syntax errors #15680
Conversation
logan-keede
changed the title
There was a problem hiding this comment.
Thank you @logan-keede !
Is it possible to add a test for this feature, perhaps in
datafusion/datafusion/sql/tests/cases/diagnostic.rs
Lines 156 to 163 in 63f37a3
Yes, I will do that. Thanks for the review!! |
| let statement = Parser::new(&dialect) | ||
| .try_with_sql(sql) | ||
| // let dialect = GenericDialect {}; | ||
| let statement = DFParserBuilder::new(sql) |
There was a problem hiding this comment.
This is somewhat out of the way, but I think using DFParser makes more sense here, as that's what is ultimately expected to be used by user.
There was a problem hiding this comment.
Thank you @logan-keede -- this looks great (as always).
Since DFParser is a public API this is technically a breaking change, so I will mark the PR as such
I also tested this in datafusion-cli locally and it works great:
DataFusion CLI v46.0.1
> select
; 🤔 Invalid statement: SQL error: ParserError("Expected: an expression, found: ; at Line: 1, Column: 7")
datafusion/sql/src/parser.rs
Outdated
| // Convert TokenizerError -> ParserError | ||
| let tokens = tokenizer | ||
| .tokenize_with_location() | ||
| .map_err(|e| ParserError::TokenizerError(e.to_string()))?; |
There was a problem hiding this 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:
.map_err(ParserError::From)
datafusion/sql/src/parser.rs
Outdated
| found.span.start | ||
| )) | ||
| .map_err(|e| { | ||
| let e: DataFusionError = e.into(); |
There was a problem hiding this comment.
Here is an alternate syntax that I find more explicit, but totally uneeded
| let e: DataFusionError = e.into(); | |
| let e = DataFusionError::from(e); |
datafusion/sql/src/parser.rs
Outdated
| return Err(ParserError::ParserError( | ||
| "Missing TO clause in COPY statement".into(), | ||
| )); | ||
| return parser_err!(format!("Missing TO clause in COPY statement"))?; |
datafusion/sql/src/parser.rs
Outdated
| return Err(ParserError::ParserError(format!( | ||
| "Unexpected token {token}" | ||
| ))); | ||
| return parser_err!(format!("Unexpected token {token}"))?; |
There was a problem hiding this comment.
The parser_err macro already handles the string interpolation so you can do this insted:
| return parser_err!(format!("Unexpected token {token}"))?; | |
| return parser_err!("Unexpected token {token}")?; |
(There are a few other examples of this below)
There was a problem hiding this comment.
we can probably use expected function here. I will try to commit that and other
suggestions tomorrow. (It's quite late here)
There was a problem hiding this comment.
No rush -- I think we are going to hold of merging any more code changes until we make a 47.0.0 release branch / candidate.
| parser_err!(format!("Expected {expected}, found: {found}")) | ||
| ) -> Result<T, DataFusionError> { | ||
| let sql_parser_span = found.span; | ||
| parser_err!(format!( |
There was a problem hiding this comment.
this actually gives me an idea that err! should support diagnostics as optional param, so we can remove extra map_err
Which issue does this PR close?
Diagnosticto syntax errors #14437.Rationale for this change
converting ParserError into DataFusionError to attach diagnostic through
expected.What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?