Skip to content
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

Implment backtrack for parser and add a human-friendly pretty printer for errors #4045

Merged
merged 13 commits into from Feb 6, 2022

Conversation

andylokandy
Copy link
Collaborator

I hereby agree to the terms of the CLA available at: https://databend.rs/dev/policies/cla/

Summary

Now the SQL parser keeps tracking the grammar path it tried and return the longest possible (but failed) path. Also, a pretty printer is added to render the error path into a human-friendly error report.

For example, parsing the statement truncate table a will produce:

error: 
  --> SQL:1:17
  |
1 | truncate table a
  | --------        ^ expected token ";"
  | |               
  | while parsing TRUNCATE TABLE statement

Changelog

  • New Feature

Related Issues

Blocked by #4039
Ref #866

Test Plan

Unit Tests

@databend-bot databend-bot added the pr-feature this PR introduces a new feature to the codebase label Feb 2, 2022
@databend-bot
Copy link
Member

Thanks for the contribution!
I have applied any labels matching special text in your PR Changelog.

Please review the labels and make any necessary changes.

1 similar comment
@databend-bot
Copy link
Member

Thanks for the contribution!
I have applied any labels matching special text in your PR Changelog.

Please review the labels and make any necessary changes.

@vercel
Copy link

vercel bot commented Feb 2, 2022

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/databend/databend/6HcokCKyMmvT6oGYb7sJ5vXwkH47
✅ Preview: https://databend-git-fork-andylokandy-error-databend.vercel.app

[Deployment for e4b83c2 canceled]

@codecov-commenter
Copy link

codecov-commenter commented Feb 3, 2022

Codecov Report

Merging #4045 (09859e3) into main (76e831b) will increase coverage by 0%.
The diff coverage is 83%.

Impacted file tree graph

@@          Coverage Diff           @@
##            main   #4045    +/-   ##
======================================
  Coverage     57%     57%            
======================================
  Files        821     824     +3     
  Lines      43468   43995   +527     
======================================
+ Hits       24805   25294   +489     
- Misses     18663   18701    +38     
Impacted Files Coverage Δ
common/ast/src/parser/rule/error.rs 79% <79%> (ø)
common/ast/src/parser/rule/util.rs 78% <80%> (ø)
common/ast/src/parser/rule/expr.rs 75% <82%> (+75%) ⬆️
common/ast/src/parser/ast/expression.rs 65% <100%> (+18%) ⬆️
common/ast/src/parser/rule/statement.rs 100% <100%> (ø)
common/ast/src/parser/token.rs 81% <0%> (-19%) ⬇️
metasrv/src/network.rs 97% <0%> (-2%) ⬇️
common/containers/src/pool.rs 79% <0%> (+2%) ⬆️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 76e831b...09859e3. Read the comment docs.

@BohuTANG
Copy link
Member

BohuTANG commented Feb 3, 2022

Cool, #4039 merged :)

@andylokandy
Copy link
Collaborator Author

@BohuTANG thanks! This is also ready for review.

Comment on lines +5 to +13
---------- AST ------------
ColumnRef {
database: None,
table: None,
column: Identifier {
name: "a",
quote: None,
},
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, it's better to take normalized AST(i.e. the Debug string of AST) as output, since structure of AST may change in the future.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is the debug string of AST.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, my fault. I mean the Display string, which is normalized SQL string of an AST.

@databend-bot
Copy link
Member

Wait for another reviewer approval

@BohuTANG
Copy link
Member

BohuTANG commented Feb 6, 2022

/lgtm

Thank you @andylokandy !

@databend-bot
Copy link
Member

CI Passed
Reviewers Approved
Let's Merge
Thank you for the PR @andylokandy

@databend-bot databend-bot merged commit 91e5969 into datafuselabs:main Feb 6, 2022
@leiysky leiysky added this to Done in SQL engine refactor May 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-feature this PR introduces a new feature to the codebase
Projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants