Feat union #151

floriankramer · 2018-11-14T13:36:30Z

This pr adds support for the UNION feature.

niklas88 · 2018-11-14T13:45:17Z

Since this adds End-to-End Tests, you could give #150 a quick review and we could merge this and change the new tests on a rebase.

niklas88 · 2018-11-14T13:59:18Z

So it looks like we have a test failure and also the following compiler warning still persits:

/app/src/parser/ParsedQuery.cpp: In member function ‘std::__cxx11::string SparqlFilter::asString() const’:
/app/src/parser/ParsedQuery.cpp:82:10: warning: enumeration value ‘PREFIX’ not handled in switch [-Wswitch]
   switch (_type) {

floriankramer · 2018-11-14T14:31:02Z

The test failure is really strange. I suddenly got errors in that test locally due to the order of nodes of the triple graph being permuted, which then broke the string based comparison used in the test.
I've reverted the test to its current master branch state which will hopefully stop the tests from failing for travis, but the better solution would probably to make them order invariant.

niklas88 · 2018-11-14T14:56:35Z

@floriankramer I've seen something similar when changing the implementation of the hash map. When hash map iteration is randomized (as for example in abseil::flat_map) the QueryPlannerTest fails sometimes.

niklas88

Being able to add a complex new feature like UNION now and with relatively manageable changes is a really great sign that we're on a good track with QLever. Well done! I'm still a bit confused about a few details but overall this already looks quite good.

niklas88 · 2018-11-14T15:40:13Z

e2e/scientists_queries.yaml

+    sparql: |
+      SELECT ?a ?t WHERE {
+        {?a <Height> ?t .}
+        uNiON


I think if one of the UNION keywords is written in weird case that's nice to test case insensitivity but then the other one should use the normal all uppercase

Also since I find UNION in SPARQL a bit weird maybe a comment to the spec could be added in a comment

I changed the case of the second e2e test UNION keyword and added an explanation for UNIONS and OPTIONAL to the README. I can add a comment into the code as well, if you think that's necessary.

@floriankramer I think you might have forgotten to push those changes or are you still working on this?

I was still working on the changes, I pushed them just now.

niklas88 · 2018-11-14T15:48:07Z

src/engine/QueryPlanner.cpp

@@ -72,8 +76,40 @@ QueryExecutionTree QueryPlanner::createExecutionTree(ParsedQuery& pq) const {

    LOG(DEBUG) << "Creating execution plan.\n";
    childPlans.clear();
-    for (const ParsedQuery::GraphPattern* child : pattern->_children) {
-      childPlans.push_back(&patternPlans[child->_id]);
+    std::vector<SubtreePlan> unionPlans;


Since this is only done at the top level I'd guess we currently don't support nested UNION, though these are also part of the spec? How about OPTIONAL inside UNION? Honestly by now I'm quite confused about what actually happens in createExecutionTree() and how ParsedQuery::GraphPatternOperation and SubtreePlan interact. Any chance we can split this in a couple of more manageable methods? I mean it's obviously not your fault that this code is now quite hard to understand

That said I think we could also do the simplification in a separate PR but we should probably do something to make these methods more understandable

The query is parsed recursively, so any nesting of unions and optionals is parsed corectly. The result of that are ParsedQueries, which contain a tree of alternating GraphPatterns and GraphPatternOperations, where the GraphPattern represents a set of triples, filters and further oeprations while the GraphPatternOperation represents either an OPTIONAL or a UNION (and in the future possibly also a subquery). I added the additional layer of GraphPatternOperations as UNIONs require two GraphPatterns a children while OPTIONAL only requires one, but I wanted a unified abstraction for the two, to keep the query planning simpler.
The optimizer then creates a topological ordering of that tree, effectively skipping over GraphPatternOperations and only looking at the layers containing GraphPatterns. Every element of that ordering is then optimized separately, starting at the back. This way, whenever a GraphPattern is optimized all of its children have already been optimized. When optimizing a GraphPattern all GraphPatternOperations SubtreePlans are added to the childPlans vector, and then effectively treated as if they were another node in the triple graph.

I added some more comments for now, but I can definitely try to make the method easier to read and understand in a later PR.

niklas88 · 2018-11-14T16:02:13Z

src/engine/Union.cpp

+}
+
+size_t Union::getResultWidth() const {
+  // the width depends on how many columns use the same variable


So you mean the width is the number of unique columns? Maybe reformulate that comment. Also what's a columnOrigin?

I changed the comment. The columnOrigin vector stores from what column in each of the subtrees the values for a column in the result come.

niklas88 · 2018-11-14T16:03:38Z

src/engine/Union.cpp

+vector<size_t> Union::resultSortedOn() const { return {}; }
+
+std::unordered_map<string, size_t> Union::getVariableColumns() const {
+  std::unordered_map<string, size_t> variableColumns(


ad_utility::HashMap though that also depends on whether we want to merge #137

niklas88 · 2018-11-14T16:10:32Z

src/engine/Union.h

+      vector<Res>* res, const vector<L>* left, const vector<R>* right,
+      const std::vector<std::array<size_t, 2>>& columnOrigins);
+
+  /*template <typename T>


remove unused code

niklas88 · 2018-11-14T16:10:52Z

src/engine/Union.h

+
+  virtual vector<size_t> resultSortedOn() const override;
+
+  std::unordered_map<string, size_t> getVariableColumns() const;


see above about ad_utility::HashMap

niklas88 · 2018-11-14T16:15:04Z

src/parser/SparqlParser.cpp

-        currentPattern->_children.back()->_id = query._numGraphPatterns;
+        size_t cb = ad_utility::findClosingBracket(inner, ob, '{', '}');
+        currentPattern->_children.push_back(
+            new ParsedQuery::GraphPatternOperation(


where are these freed? Can't find it in GraphPattern and that has std::vector of raw pointers not std::shared_ptr

The GraphPattern destructor deletes them.

niklas88 · 2018-11-14T16:15:27Z

src/parser/SparqlParser.cpp

+        currentPattern->_children.push_back(
+            new ParsedQuery::GraphPatternOperation(
+                ParsedQuery::GraphPatternOperation::Type::OPTIONAL,
+                {new ParsedQuery::GraphPattern()}));


and where are these freed?

The GraphPatternOperation destructor deletes them

niklas88

LGTM but I propose we merge the HashMap/HashSet changes first, then rebase this on them and also change the unordered_map uses

Also added a Union operation that is not yet used.

The changes are necessary for the test to complete locally but seem to break it for travis.

floriankramer requested a review from niklas88 November 14, 2018 13:36

floriankramer force-pushed the feat_union branch 2 times, most recently from 2f48760 to 698855b Compare November 14, 2018 14:27

niklas88 reviewed Nov 14, 2018

View reviewed changes

niklas88 approved these changes Nov 15, 2018

View reviewed changes

floriankramer added 6 commits November 18, 2018 11:59

Changed the ParsedQuery to allow for representation of unions.

21de3ba

Also added a Union operation that is not yet used.

Added support for parsing union and unit tests for the union operation.

a46a75b

Added e2e tests.

b864708

Reverted the changes to the QueryPlannerTest.

134db96

The changes are necessary for the test to complete locally but seem to break it for travis.

Some bugfixes in the query planning and more comments.

35b787f

Switched Unions from std::unorderd_map to ad_utility::HashMap

6d9edb5

floriankramer force-pushed the feat_union branch from 0f6d746 to 6d9edb5 Compare November 18, 2018 11:07

floriankramer merged commit 368ef9c into ad-freiburg:master Nov 18, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat union #151

Feat union #151

floriankramer commented Nov 14, 2018

niklas88 commented Nov 14, 2018

niklas88 commented Nov 14, 2018

floriankramer commented Nov 14, 2018

niklas88 commented Nov 14, 2018

niklas88 left a comment

niklas88 Nov 14, 2018

niklas88 Nov 14, 2018

floriankramer Nov 15, 2018

niklas88 Nov 15, 2018

floriankramer Nov 15, 2018

niklas88 Nov 14, 2018

niklas88 Nov 14, 2018

floriankramer Nov 15, 2018

floriankramer Nov 15, 2018

niklas88 Nov 14, 2018

floriankramer Nov 15, 2018

niklas88 Nov 14, 2018

niklas88 Nov 14, 2018

floriankramer Nov 15, 2018

niklas88 Nov 14, 2018

niklas88 Nov 14, 2018

floriankramer Nov 15, 2018

niklas88 Nov 14, 2018

floriankramer Nov 15, 2018

niklas88 left a comment


		virtual vector<size_t> resultSortedOn() const override;

		std::unordered_map<string, size_t> getVariableColumns() const;

Feat union #151

Feat union #151

Conversation

floriankramer commented Nov 14, 2018

niklas88 commented Nov 14, 2018

niklas88 commented Nov 14, 2018

floriankramer commented Nov 14, 2018

niklas88 commented Nov 14, 2018

niklas88 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niklas88 left a comment

Choose a reason for hiding this comment