F.refactor scans #214

joka921 · 2019-03-21T17:51:35Z

Refactored the Scan operation.

Now we have everything in one place to reason about. This makes the code cleaner and more maintainable.

niklas88

I'm really glad you're tackling this it looks much cleaner already. I've added a few comments for the first change. I also propose we completely remove the -a flag. QLever now focuses more on being a general SPARQL engine and with your previous changes the cost isn't al that high. Also it's a source of frustration if you forget the flag.

niklas88 · 2019-03-22T09:32:55Z

src/index/Index.h

@@ -599,4 +599,82 @@ class Index {
    }
    LOG(INFO) << "Done" << std::endl;
  }
+
+  // _____________________________________________________________________________


Can you add a method comment. I know we haven't had one here previously but it would be great to see this improve. Also I'd propose to use the format Florian used in recent PRs instead of the //! we have in some parts.

niklas88 · 2019-03-22T09:33:51Z

src/index/Index.h

+            const PermutationInfo& perm, const MetaData& meta,
+            ad_utility::File& file) const {
+    LOG(DEBUG) << "Performing " << perm._readableName << "  scan of relation "
+               << keyFirst << " with fixed subject: " << keySecond << "...\n";


This isn't always the subject, right?

niklas88 · 2019-03-22T09:39:04Z

src/index/Index.h

+  void scan(const string& key, IdTable* result, const MetaData& meta,
+            ad_utility::File& file, const string& name) const {
+    if constexpr (checkPredicateVariable) {
+      if (!file.isOpen()) {


Can't we just do the file.isOpen() check for every one of the permutations and just warn that it doesn't exist? This would be basically free and useful to know even the file was just deleted. On top of this, seeing as the focus of QLever has shifted more towards being a full featured SPARQL engine and that your changes have made it a lot cheaper, I would propose we just remove that -a switch completely.

niklas88 · 2019-03-22T09:46:20Z

src/index/Index.h

+    LOG(DEBUG) << "Performing " << perm._readableName << "  scan of relation "
+               << keyFirst << " with fixed subject: " << keySecond << "...\n";
+    Id relId;
+    Id subjId;


These two are now also only sometimes relation and subject, right? So maybe they should be renamed

niklas88 · 2019-03-22T10:04:19Z

src/index/Index.h

+
+  //_____________________________________________________________________________
+  template <class MetaData>
+  void scanImpl(Id key, IdTable* result, const MetaData& meta,


I think this should have a name that better distinguishes it from the scan() function. It's not really an …Impl as it isn't used to implement scan() right? Maybe scanFull() because it scans a full "relation"?

I have a different suggestion: Everything is a scan. The overload is sufficient to disambiguate:

1 or 2 keys

are the keys strings (have to be looked up in vocab) or Ids (this has already been done)

ghost · 2019-03-22T12:12:57Z

DeepCode encountered a problem when analyzing this pull request. If you want to retry, create a comment: "Retry Deepcode".

joka921 · 2019-03-22T12:39:48Z

Retry Deepcode

niklas88

Wow, your changes remove twice as many lines as they add. I really like how much cleaner this looks now. I've added a few suggestions but nothing major.

niklas88 · 2019-03-25T13:10:40Z

src/engine/Join.cpp

@@ -272,26 +273,37 @@ void Join::computeResultForJoinWithFullScanDummy(ResultTable* result) const {
 Join::ScanMethodType Join::getScanMethod(
    std::shared_ptr<QueryExecutionTree> fullScanDummyTree) const {
  ScanMethodType scanMethod;
+  using namespace std::placeholders;


Is this used? I only know of _1,… in this namespace but I can't see them anywhere

I was thinking about std::bind at some point but found another solution, it is removed now.

niklas88 · 2019-03-25T13:11:06Z

src/engine/Join.h

@@ -75,7 +75,8 @@ class Join : public Operation {

  void computeResultForJoinWithFullScanDummy(ResultTable* result) const;

-  typedef void (Index::*ScanMethodType)(Id, IdTable*) const;
+  // typedef void (Index::*ScanMethodType)(Id, IdTable*) const;


Remove the comment

niklas88 · 2019-03-25T13:12:34Z

src/engine/Join.h

@@ -75,7 +75,8 @@ class Join : public Operation {

  void computeResultForJoinWithFullScanDummy(ResultTable* result) const;

-  typedef void (Index::*ScanMethodType)(Id, IdTable*) const;
+  // typedef void (Index::*ScanMethodType)(Id, IdTable*) const;
+  using ScanMethodType = std::function<void(Id, IdTable*)>;


Are we sure the std::function doesn't have an overhead here? I'd guess it specializes for a standard function pointer but I'm not sure. Then again the scans are a lot slower than that overhead

You answered your question:
The std::function overhead is basically a virtual function call. It is called once per scan, and scans are on disk operations. So I think in this case it is perfectly fine. Other solutions seem much more hacky to me.

niklas88 · 2019-03-25T13:17:07Z

src/index/Index.h

-                    const MetaData Index::*metaPtr,
-                    ad_utility::File Index::*filePtr)
+                    array<unsigned short, 3> order, const MetaData& meta,
+                    ad_utility::File& file)


We try not to use non-const references (its forbidden in the Google styleguide though in the past we have been less strict). I think copying an ad_utility::File and passing by value should work unless you aleady mmap()ed it at this point.

Check out my latest commits, I have moved the MetaData and the file completely into the permutation class.

src/index/Index.h

niklas88 · 2019-03-25T13:27:16Z

src/engine/Join.cpp

+
+  // this works because the join operations execution Context never changes
+  // during its lifetime
+  auto genericLambda = [this](const auto& perm) {


can this be const auto? also I would call it scanLambda. Also can't we use std::bind() here? Is that why you have a using namespace std::placeholders? Alternatively instead of capturing this we could capture a const auto& idx = this->_executionContext->getIndex() which would make the lambda simpler too.

fixed, it is indeed much more readable if we only capture what we need.

niklas88 · 2019-03-25T15:47:08Z

src/index/Permutations.h

+    }
+    auto filename = string(onDiskBase + ".index" + _fileSuffix);
+    _file.open(filename, "r");
+    AD_CHECK(_file.isOpen());


I would turn this into an if with a proper error message about missing the file and that this means the index is broken.
Since you have this here I think you should remove the isOpen() check from the scans of course that makes the point about missing that in the two parameter case moot.

niklas88 · 2019-03-25T15:49:24Z

test/IndexTest.cpp

@@ -287,38 +289,38 @@ TEST(IndexTest, createFromTsvTest) {
    // Lhs info
    //    ASSERT_EQ(0u, *reinterpret_cast<Id*>(buf + bytesDone));
    //    bytesDone += sizeof(Id);
-    //    ASSERT_EQ(index._posMeta.getRmd(7)._rmdBlocks->_startRhs,
+    //    ASSERT_EQ(index.POS().metaData.getRmd(7)._rmdBlocks->_startRhs,


I think we should remove the commented code outright, git will remember it in case we ever need it again

joka921 · 2019-03-26T17:20:50Z

Retry Deepcode

niklas88 · 2019-03-28T15:59:51Z

test/IndexTest.cpp

-    //    bytesDone += sizeof(Id);
+    //    ASSERT_EQ(bytesDone,
+    //    index._PSO.metaData().getRmd(7)._rmdBlocks->_startRhs); ASSERT_EQ(0u,
+    //    *reinterpret_cast<Id*>(buf + bytesDone)); bytesDone += sizeof(Id);


Remove commented section

niklas88

I really really like these PRs that remove hundreds of lines of code while making the rest more maintainable.

joka921 · 2019-03-28T17:55:50Z

Retry Deepcode

- The templated Permutation class now owns the Permutations meta data and file - The scan method now is templated on the permutations and takes it as an argument - This removes a lot of duplicated code in the Index class. - Idea for further improvements: The scan methods could also be inside the Permutations, this further reduces the complexity per file

niklas88 self-requested a review March 22, 2019 09:26

niklas88 reviewed Mar 22, 2019

View reviewed changes

niklas88 reviewed Mar 25, 2019

View reviewed changes

niklas88 reviewed Mar 28, 2019

View reviewed changes

niklas88 approved these changes Mar 28, 2019

View reviewed changes

joka921 force-pushed the f.refactorScans branch from 33d7222 to 863e7aa Compare March 28, 2019 18:20

joka921 merged commit 9269d2a into ad-freiburg:master Mar 28, 2019

joka921 deleted the f.refactorScans branch March 28, 2019 19:58

niklas88 mentioned this pull request Mar 29, 2019

Can't use read-only index mount with latest version #218

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

F.refactor scans #214

F.refactor scans #214

joka921 commented Mar 21, 2019

niklas88 left a comment

niklas88 Mar 22, 2019

niklas88 Mar 22, 2019

niklas88 Mar 22, 2019

niklas88 Mar 22, 2019

niklas88 Mar 22, 2019

joka921 Mar 22, 2019

ghost commented Mar 22, 2019

joka921 commented Mar 22, 2019

niklas88 left a comment

niklas88 Mar 25, 2019

joka921 Mar 25, 2019

niklas88 Mar 25, 2019

niklas88 Mar 25, 2019

joka921 Mar 25, 2019

niklas88 Mar 25, 2019

joka921 Mar 25, 2019

niklas88 Mar 25, 2019

joka921 Mar 25, 2019

niklas88 Mar 25, 2019

niklas88 Mar 25, 2019

joka921 commented Mar 26, 2019

niklas88 Mar 28, 2019

niklas88 left a comment

joka921 commented Mar 28, 2019

F.refactor scans #214

F.refactor scans #214

Conversation

joka921 commented Mar 21, 2019

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

ghost commented Mar 22, 2019

joka921 commented Mar 22, 2019

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

joka921 commented Mar 26, 2019

Choose a reason for hiding this comment

niklas88 left a comment

Choose a reason for hiding this comment

joka921 commented Mar 28, 2019