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

Parse WHERE clause using ANTLR #749

Merged
merged 33 commits into from
Aug 26, 2022
Merged

Conversation

Qup42
Copy link
Member

@Qup42 Qup42 commented Aug 22, 2022

Parse whereClause in ANTLR. This is one fairly big connected chunk of the grammar.

The parsing should already work. It is live in the SparqlParser.

TODO:

  • more Tests
  • negative Tests
  • Combine std::pair<...> return types into structs.
  • Research valuesClause

Copy link
Member

@joka921 joka921 left a comment

Choose a reason for hiding this comment

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

This already looks very nice. I gave it a first long round of reviews, let me know when you want to talk about any of it.

src/util/Algorithm.h Outdated Show resolved Hide resolved
src/util/Algorithm.h Outdated Show resolved Hide resolved
src/util/Algorithm.h Show resolved Hide resolved
src/util/Algorithm.h Outdated Show resolved Hide resolved
src/util/Algorithm.h Show resolved Hide resolved
src/parser/sparqlParser/SparqlQleverVisitor.cpp Outdated Show resolved Hide resolved
src/parser/sparqlParser/SparqlQleverVisitor.cpp Outdated Show resolved Hide resolved
Copy link
Member

@joka921 joka921 left a comment

Choose a reason for hiding this comment

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

Very nice, this is really getting in shape.
I have left some remarks, and some of this we should maybe discuss in person.
But I think you should have enough to do:)

src/parser/sparqlParser/SparqlQleverVisitor.cpp Outdated Show resolved Hide resolved
src/parser/sparqlParser/SparqlQleverVisitor.cpp Outdated Show resolved Hide resolved
src/parser/sparqlParser/SparqlQleverVisitor.cpp Outdated Show resolved Hide resolved
test/SparqlAntlrParserTest.cpp Show resolved Hide resolved
test/SparqlAntlrParserTestHelpers.h Show resolved Hide resolved
test/SparqlAntlrParserTestHelpers.h Outdated Show resolved Hide resolved
test/SparqlAntlrParserTestHelpers.h Outdated Show resolved Hide resolved
test/SparqlParserTest.cpp Outdated Show resolved Hide resolved
@Qup42 Qup42 marked this pull request as ready for review August 25, 2022 18:48
Copy link
Member

@joka921 joka921 left a comment

Choose a reason for hiding this comment

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

This is almost done. We have already tried this out yesterday and especially @hannahbast liked it very much. There is one slight issue with the new way you call addLanguageFilter but that is because that function was always kind of broken and will already be fixed before this gets merged (not your fault).

src/parser/sparqlParser/SparqlQleverVisitor.cpp Outdated Show resolved Hide resolved
src/parser/sparqlParser/SparqlQleverVisitor.cpp Outdated Show resolved Hide resolved
src/parser/sparqlParser/SparqlQleverVisitor.cpp Outdated Show resolved Hide resolved
src/parser/sparqlParser/SparqlQleverVisitor.cpp Outdated Show resolved Hide resolved
src/parser/sparqlParser/SparqlQleverVisitor.cpp Outdated Show resolved Hide resolved
test/CompactStringVectorTest.cpp Outdated Show resolved Hide resolved
test/CompactStringVectorTest.cpp Outdated Show resolved Hide resolved
test/SparqlAntlrParserTest.cpp Outdated Show resolved Hide resolved
test/SparqlAntlrParserTestHelpers.h Outdated Show resolved Hide resolved
test/SparqlAntlrParserTest.cpp Show resolved Hide resolved
Copy link
Member

@joka921 joka921 left a comment

Choose a reason for hiding this comment

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

Really nice,
I think we are converging towards merging:)

src/parser/ParsedQuery.h Outdated Show resolved Hide resolved
src/parser/sparqlParser/SparqlQleverVisitor.cpp Outdated Show resolved Hide resolved
src/parser/sparqlParser/SparqlQleverVisitor.cpp Outdated Show resolved Hide resolved
src/parser/sparqlParser/SparqlQleverVisitor.cpp Outdated Show resolved Hide resolved
src/parser/sparqlParser/SparqlQleverVisitor.cpp Outdated Show resolved Hide resolved
src/parser/sparqlParser/SparqlQleverVisitor.cpp Outdated Show resolved Hide resolved
src/parser/sparqlParser/SparqlQleverVisitor.cpp Outdated Show resolved Hide resolved
src/parser/sparqlParser/SparqlQleverVisitor.cpp Outdated Show resolved Hide resolved
src/parser/sparqlParser/SparqlQleverVisitor.cpp Outdated Show resolved Hide resolved
test/SparqlAntlrParserTestHelpers.h Show resolved Hide resolved
Copy link
Member

@joka921 joka921 left a comment

Choose a reason for hiding this comment

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

Great! This is a very important milestone towards more correct and cleaner parsing!

@joka921 joka921 changed the title Parse whereClause in ANTLR Parse WHERE clause using ANTLR Aug 26, 2022
@joka921 joka921 merged commit 484ab24 into ad-freiburg:master Aug 26, 2022
@hannahbast
Copy link
Member

hannahbast commented Aug 29, 2022

@Qup42 BTW, I already told Johannes, but I didn't tell you yet, Julian: Thank you very much for your work on this PR and the associated ones! It's a major milestone for QLever. In particular, it allowed me a happy update of this page: https://github.com/ad-freiburg/qlever/wiki/Current-deviations-from-the-SPARQL-1.1-standard . And it's the basis for many other improvements that had to wait so far because we didn't have a proper SPARQL parser.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants