Attach Diagnostic to syntax errors by logan-keede · Pull Request #15680 · apache/datafusion

logan-keede · 2025-04-10T20:34:37Z

Which issue does this PR close?

Closes Attach Diagnostic to 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?

logan-keede · 2025-04-13T18:01:58Z

cc @eliaperantoni

alamb

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

    
           fn test_qualified_column_not_found() -> Result<()> { 
        
               let query = "SELECT /*a*/person.first_namex/*a*/ FROM person"; 
        
               let spans = get_spans(query); 
        
               let diag = do_query(query); 
        
               assert_snapshot!(diag.message, @"column 'first_namex' not found in 'person'"); 
        
               assert_eq!(diag.span, Some(spans["a"])); 
        
               Ok(()) 
        
           }

?

logan-keede · 2025-04-14T21:23:14Z

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

fn test_qualified_column_not_found() -> Result<()> {

let query = "SELECT /*a*/person.first_namex/*a*/ FROM person";

let spans = get_spans(query);

let diag = do_query(query);

assert_snapshot!(diag.message, @"column 'first_namex' not found in 'person'");

assert_eq!(diag.span, Some(spans["a"]));

Ok(())

}

?

Yes, I will do that. Thanks for the review!!

logan-keede · 2025-04-15T21:38:16Z

datafusion/sql/tests/cases/diagnostic.rs

-    let statement = Parser::new(&dialect)
-        .try_with_sql(sql)
+    // let dialect = GenericDialect {};
+    let statement = DFParserBuilder::new(sql)


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.

alamb

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")

alamb · 2025-04-15T22:03:29Z

datafusion/sql/src/parser.rs

+        // Convert TokenizerError -> ParserError
+        let tokens = tokenizer
+            .tokenize_with_location()
+            .map_err(|e| ParserError::TokenizerError(e.to_string()))?;


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)

alamb · 2025-04-15T22:08:52Z

datafusion/sql/src/parser.rs

+            found.span.start
+        ))
+        .map_err(|e| {
+            let e: DataFusionError = e.into();


Here is an alternate syntax that I find more explicit, but totally uneeded

Suggested change

let e: DataFusionError = e.into();

let e = DataFusionError::from(e);

alamb · 2025-04-15T22:09:03Z

datafusion/sql/src/parser.rs

-            return Err(ParserError::ParserError(
-                "Missing TO clause in COPY statement".into(),
-            ));
+            return parser_err!(format!("Missing TO clause in COPY statement"))?;


nice cleanups

alamb · 2025-04-15T22:10:18Z

datafusion/sql/src/parser.rs

-                    return Err(ParserError::ParserError(format!(
-                        "Unexpected token {token}"
-                    )));
+                    return parser_err!(format!("Unexpected token {token}"))?;


The parser_err macro already handles the string interpolation so you can do this insted:

Suggested change

return parser_err!(format!("Unexpected token {token}"))?;

return parser_err!("Unexpected token {token}")?;

(There are a few other examples of this below)

we can probably use expected function here. I will try to commit that and other
suggestions tomorrow. (It's quite late here)

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.

alamb

Nice!

comphead · 2025-04-16T20:50:16Z

datafusion/sql/src/parser.rs

-        parser_err!(format!("Expected {expected}, found: {found}"))
+    ) -> Result<T, DataFusionError> {
+        let sql_parser_span = found.span;
+        parser_err!(format!(


this actually gives me an idea that err! should support diagnostics as optional param, so we can remove extra map_err

* ParserError->DataFusionError+attach a diagnostic * fix: ci * fix: fmt * fix:clippy * does this fix ci test? * this fixes sqllogictest * fix: cargo test * fix: fmt * add tests * cleanup * suggestions + expect EOF nicely * fix: clippy

ParserError->DataFusionError+attach a diagnostic

4c0114d

github-actions bot added the sql SQL Planner label Apr 10, 2025

logan-keede mentioned this pull request Apr 10, 2025

Attach Diagnostic to syntax errors #14437

Closed

logan-keede added 7 commits April 13, 2025 17:19

fix: ci

fc22ac2

fix: fmt

30b21ea

fix:clippy

d2bace2

does this fix ci test?

a0139d3

this fixes sqllogictest

3150fee

fix: cargo test

f2d483a

fix: fmt

07878df

logan-keede marked this pull request as ready for review April 13, 2025 18:01

logan-keede changed the title ~~WIP: Attach Diagnostic to syntax errors~~ Attach Diagnostic to syntax errors Apr 13, 2025

alamb reviewed Apr 14, 2025

View reviewed changes

logan-keede added 2 commits April 16, 2025 02:11

add tests

8ab8c7f

cleanup

52467c2

logan-keede commented Apr 15, 2025

View reviewed changes

alamb approved these changes Apr 15, 2025

View reviewed changes

alamb added the api change Changes the API exposed to users of the crate label Apr 15, 2025

suggestions + expect EOF nicely

8aefb89

github-actions bot added the sqllogictest SQL Logic Tests (.slt) label Apr 16, 2025

fix: clippy

6656f9d

alamb approved these changes Apr 16, 2025

View reviewed changes

comphead reviewed Apr 16, 2025

View reviewed changes

comphead merged commit 42a45d1 into apache:main Apr 16, 2025
27 checks passed

logan-keede mentioned this pull request Apr 21, 2025

Make Diagnostic easy/convinient to attach by using macro and avoiding map_err #15796

Merged

alamb mentioned this pull request Jun 5, 2025

chore: update to DataFusion 48.0.0 / arrow to 55.2.0 delta-io/delta-rs#3520

Merged

	fn test_qualified_column_not_found() -> Result<()> {
	let query = "SELECT /a/person.first_namex/a/ FROM person";
	let spans = get_spans(query);
	let diag = do_query(query);
	assert_snapshot!(diag.message, @"column 'first_namex' not found in 'person'");
	assert_eq!(diag.span, Some(spans["a"]));
	Ok(())
	}

	let e: DataFusionError = e.into();
	let e = DataFusionError::from(e);

	return parser_err!(format!("Unexpected token {token}"))?;
	return parser_err!("Unexpected token {token}")?;

Comments

Conversation

logan-keede commented Apr 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

logan-keede commented Apr 13, 2025

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

logan-keede commented Apr 14, 2025

Uh oh!

logan-keede Apr 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

alamb Apr 15, 2025

Choose a reason for hiding this comment

Uh oh!

alamb Apr 15, 2025

Choose a reason for hiding this comment

Uh oh!

alamb Apr 15, 2025

Choose a reason for hiding this comment

Uh oh!

alamb Apr 15, 2025

Choose a reason for hiding this comment

Uh oh!

logan-keede Apr 15, 2025

Choose a reason for hiding this comment

Uh oh!

alamb Apr 15, 2025

Choose a reason for hiding this comment

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

comphead Apr 16, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

logan-keede commented Apr 10, 2025 •

edited

Loading

logan-keede Apr 15, 2025 •

edited

Loading