-
Notifications
You must be signed in to change notification settings - Fork 58
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
Parse complete SPARQL queries using ANTLR #790
Conversation
qlever/src/parser/SparqlParser.cpp Lines 18 to 20 in 0692f13
The lexer is now only used in SparqlParser::parseFilter and SparqlParser::parseFilter . The creation of the lexer could maybe be postponed to a later time.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is already looking really nice. I have some suggestions considering the code structure and better comments as usual, but it looks really nice that the old Parser is now almost completely gone.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor suggestions, and of course the stuff that remains from my last review.
Condition was inverted
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only very small suggestions remain, this will soon be ready to merge.
Co-authored-by: joka921 <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The very last changes, mostly it's about making the e2e tests pass again, then we can still merge this today.
auto addVariable = [&variable](auto& clause) { | ||
clause.addVisibleVariable(variable); | ||
}; | ||
std::visit(addVariable, _clause); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
auto addVariable = [&variable](auto& clause) { | |
clause.addVisibleVariable(variable); | |
}; | |
std::visit(addVariable, _clause); | |
} | |
std::visit(&parsedQuery::ClauseBase::addvisibleVariable, _clause); | |
} |
I thought I suggested this before, but it seems like I didn't.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You did but i had to revert it because it does not work. The lambda also captures the variable that is passed to addVisibleVariable.
Another thing that may be worth testing for in the future is the visible variables. They are now in every parsed query and use in quite some places. I'm currently trying if that can be done easily. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much. I think we can now merge it and move the rest to another PR.
The ANTLR based parser now parses the complete query string into a `ParsedQuery`. The only thing that is till done via the old parser are FILTERs. Also add many checks for invariants of a valid SPARQL query into the `ParsedQuery` class and improve the unit tests for the parsing.
Complete the last two pieces of query parsing in ANTLR:
ConstructQuery
andQuery
.TODO: