SparqlExpressions! And a properParser! (both at least partially, but with a general, extendable framework) #405

joka921 · 2021-05-18T16:57:13Z

A first stub of a proper Parser based on ANTLR4, used to parse Expressions.

A first implementation of expressions with an extendable framework.

Currently, we only support + - * / ! || and && , and those only in BIND expressions, but this is a good start.

TODO: - Actually implement a parser - Write a readme on how to reproduce the .h and .cpp files once the Grammar changes or ANTLR is updated.

…ilding.

…e manually created files in separate folders.

…ring with the rest of Sparql.

… ANTLR parser.

TODO: proper AND and OR bor Binary Ranges.

… Expressions.

Next step: Actually parse/Create these...

… parser.

First draft of the Visitor rules for sparql expressions.

…Github Actions.

hannahbast

Began review in 1-1 with Johannes

hannahbast · 2021-05-19T17:20:08Z

src/util/ConstexprSmallString.h

+  }
+
+  /// Construct at runtime from a string_view
+  ConstexprSmallString(std::string_view input) : _size(input.size()) {


Make explicit and have another operator== for string_view?

hannahbast · 2021-05-19T17:26:02Z

src/util/Log.h

 #define DEBUG 4
 #define INFO 3
 #define WARN 2
-#define ERROR 1
+static constexpr size_t ERROR = 1;


Why not do it for all then? Add comment

// This used to be #define, but there was a collision with a #define for ERROR in our third-party code.

hannahbast · 2021-05-19T17:36:32Z

test/SparqlAntlrParserTest.cpp

+struct ParserAndVisitor {
+ private:
+  string input;
+  ANTLRInputStream stream{input};
+  SparqlAutomaticLexer lexer{&stream};
+  CommonTokenStream tokens{&lexer};
+
+ public:
+  SparqlAutomaticParser parser{&tokens};
+  SparqlQleverVisitor visitor;
+  explicit ParserAndVisitor(string toParse) : input{std::move(toParse)} {}
+};


Don't you need this several times?

hannahbast · 2021-05-19T17:45:50Z

test/SparqlAntlrParserTest.cpp

+  LOG(INFO) << context->getText() << std::endl;
+  LOG(INFO) << p.parser.getTokenStream()
+                   ->getTokenSource()
+                   ->getInputStream()
+                   ->toString()
+            << std::endl;
+  LOG(INFO) << p.parser.getCurrentToken()->getStartIndex() << std::endl;


LOG(DEBUG) or LOG(TRACE) ?

hannahbast · 2021-05-19T17:47:01Z

test/SparqlAntlrParserTest.cpp

+  auto result = p.visitor.visitExpression(context);
+  auto expr = std::move(result.as<sparqlExpression::SparqlExpression::Ptr>());
+
+  QueryExecutionContext* ctxt = nullptr;


hannahbast · 2021-05-19T17:59:29Z

test/SparqlExpressionTest.cpp

+
+using namespace sparqlExpression;
+
+struct DummyExpression : public SparqlExpression {


TextExpression

hannahbast · 2021-05-19T18:00:46Z

test/SparqlExpressionTest.cpp

+  std::vector<double> d{1.0, 2.0, 0.0, 0.0};
+  std::vector<bool> b{false, false, true, false};


d -> input1
b -> input2

hannahbast · 2021-05-19T18:01:55Z

src/parser/sparqlParser/SparqlQleverVisitor.cpp

+
+// Generated from SparqlAutomatic.g4 by ANTLR 4.9.2
+
+#include "SparqlQleverVisitor.h"


Everything in the header?

hannahbast · 2021-05-19T18:06:40Z

src/parser/SparqlParserHelpers.cpp

+      : input{std::move(toParse)}, visitor{std::move(prefixMap)} {}
+
+  template <typename ResultType, typename ContextType>
+  auto parseStuff(const std::string& input,


parseStuff -> parseWithGivenRule

hannahbast · 2021-05-19T18:14:17Z

src/parser/SparqlParserHelpers.cpp

+  auto actualResult = p.parseStuff<sparqlExpression::SparqlExpression::Ptr>(
+      input, &SparqlAutomaticParser::expression);
+
+  return ParseResultAndRemainingText{sparqlExpression::SparqlExpressionWrapper{


SparqlExpressionPimpl

…gregates.

…the remaining types into the BIND operation and use our new expressions in the GROUP_BY operation.

TODO: Fix the tests (GROUPBY [outcommented] and SparqlParser [NotWorking]

hannahbast

First round of comments from 1-1 with Johannes

hannahbast · 2021-06-16T15:59:39Z

Dockerfile

 COPY . /app/

 # Check formatting with the .clang-format project style
 WORKDIR /app/
 ENV DEBIAN_FRONTEND=noninteractive

 WORKDIR /app/build/
-RUN cmake -DCMAKE_BUILD_TYPE=Release -DLOGLEVEL=DEBUG -DUSE_PARALLEL=true .. && make -j $(nproc) && make test
+RUN cmake -DCMAKE_BUILD_TYPE=Release -DLOGLEVEL=DEBUG -DUSE_PARALLEL=true .. && make -j $(nproc)
+# RUN make test


hannahbast · 2021-06-16T16:01:35Z

e2e/scientists_queries.yaml

+#      - num_rows: 97
+#      - selected: [ "?x", "?y", "?z" ]
+#      - contains_row: [ "<Albert_Einstein>", "<Mileva_Marić>", "<Al_Gore>" ]
+#      - order_string: { "dir": "ASC", "var": "?x" }


What about this query?

hannahbast · 2021-06-16T16:05:14Z

src/parser/SparqlExpression.h

+/// Virtual base class for an arbitrary Sparql Expression which holds the
+/// structure of the expression as well as the logic to evaluate this expression
+/// on a given intermediate result


Move comment

hannahbast · 2021-06-16T16:07:07Z

src/parser/SparqlExpression.h

+/// structure of the expression as well as the logic to evaluate this expression
+/// on a given intermediate result
+
+template <typename T>


// Like std::vector, but obtaining memory via AllocatorWithLimits and without copy constructor.

Suggestion: rename to VectorWithLimit (for consistency)

hannahbast · 2021-06-16T16:10:40Z

src/parser/SparqlExpression.h

+  // Copying is expensive, we never want to do this in the expression
+  // evaluation.


// Disable copy constructor and copy assignment operator (copying is too expensive in the setting where we want to use this class and not necessary).

TODO: move the two lines with = delete together

hannahbast · 2021-06-16T18:03:47Z

src/parser/SparqlExpression.h

+                             "&&">;
+
+/// Unary Negation
+inline auto unaryNegate = [](bool a) -> bool { return !a; };


Why no range?

hannahbast · 2021-06-16T18:05:33Z

src/parser/SparqlExpression.h

+  return a * b;
+};
+inline auto divide = [](const auto& a, const auto& b) -> double {
+  return static_cast<double>(a) / b;


What happens when dividing by zero? quiet_NaN or error?

hannahbast · 2021-06-16T18:08:19Z

src/parser/SparqlExpression.h

+                                decltype(detail::noop), "SUM">;
+
+inline auto averageFinalOp = [](const auto& aggregation, size_t numElements) {
+  return numElements ? static_cast<double>(aggregation) /


See above (regarding division by zero)

hannahbast · 2021-06-16T18:12:11Z

src/parser/SparqlExpression.h

+template <>
+struct hash<sparqlExpression::StrongId> {
+  size_t operator()(const sparqlExpression::StrongId& x) const {
+    return std::hash<Id>{}(x._value);


Use absl::hash instead of std::hash

hannahbast · 2021-06-16T18:12:45Z

src/parser/SparqlExpression.h

+    return std::hash<sparqlExpression::StrongId>{}(x._id) ^
+           std::hash<size_t>{}(static_cast<size_t>(x._type));
+  }


hannahbast

Second round of comments from 1-1 with Johannes

hannahbast · 2021-06-17T16:00:59Z