This could work now much better...

Author: joka921

src/engine/CheckUsePatternTrick.cpp

@@ -121,7 +121,6 @@ std::optional<PatternTrickTuple> checkUsePatternTrick(
       }
       const auto& subAndPred = patternTrickTuple.value();
       // First try to find a triple for which we can get the special column.
-      // TODO<joka921> Also add the column for the object triple.
       auto tripleBackup = std::move(*it);
       triples.erase(it);
       // TODO<joka921> Code duplication

src/engine/idTable/CompressedExternalIdTable.h

@@ -332,6 +332,8 @@ class CompressedExternalIdTableBase {
     this->currentBlock_.reserve(blocksize_);
     AD_CONTRACT_CHECK(NumStaticCols == 0 || NumStaticCols == numCols);
   }
+  // TODO<joka921> Shouldn't be public.
+    std::atomic<bool> isFirstMerge = true;
   // Add a single row to the input. The type of `row` needs to be something that
   // can be `push_back`ed to a `IdTable`.
   void push(const auto& row) requires requires { currentBlock_.push_back(row); }
@@ -364,6 +366,7 @@ class CompressedExternalIdTableBase {
     }
     writer_.clear();
     numBlocksPushed_ = 0;
+    isFirstMerge = true;
   }
 
  protected:
@@ -401,6 +404,9 @@ class CompressedExternalIdTableBase {
   // until the pushing is actually finished, and return `true`. Using this
   // function allows for an efficient usage of this class for very small inputs.
   bool transformAndPushLastBlock() {
+    if (!isFirstMerge) {
+      return numBlocksPushed_ != 0;
+    }
     // If we have pushed at least one (complete) block, then the last future
     // from pushing a block is still in flight. If we have never pushed a block,
     // then also the future cannot be valid.
@@ -411,7 +417,7 @@ class CompressedExternalIdTableBase {
     if (numBlocksPushed_ == 0) {
       AD_CORRECTNESS_CHECK(this->numElementsPushed_ ==
                            this->currentBlock_.size());
-      blockTransformation_(this->currentBlock_);
+        blockTransformation_(this->currentBlock_);
       return false;
     }
     pushBlock(std::move(this->currentBlock_));
@@ -511,7 +517,7 @@ class CompressedExternalIdTableSorterTypeErased {
 // false positives in the memory limit mechanism, so setting the following
 // variable to `true` allows to disable the memory limit.
 inline std::atomic<bool>
-    EXTERNAL_ID_TABLE_SORTER_IGNORE_MEMORY_LIMIT_FOR_TESTING = true;
+    EXTERNAL_ID_TABLE_SORTER_IGNORE_MEMORY_LIMIT_FOR_TESTING = false;
 
 // The implementation of sorting a single block
 template <typename Comparator>
@@ -604,6 +610,7 @@ class CompressedExternalIdTableSorter
              std::max(1, numBufferedOutputBlocks_ - 2))) {
       co_yield block;
     }
+    this->isFirstMerge = false;
     mergeIsActive_.store(false);
   }
 
@@ -623,36 +630,25 @@ class CompressedExternalIdTableSorter
   }
 
  private:
-  // TODO<joka921> Implement `CallFixedSize` optimization also for the merging.
   // Transition from the input phase, where `push()` may be called, to the
   // output phase and return a generator that yields the sorted elements. This
   // function may be called exactly once.
   template <size_t N = NumStaticCols>
   requires(N == NumStaticCols || N == 0)
   cppcoro::generator<IdTableStatic<N>> sortedBlocks(
       std::optional<size_t> blocksize = std::nullopt) {
-    auto impl = [blocksize, this]<size_t I>() {
-      if constexpr (NumStaticCols == 0 || NumStaticCols == I) {
-        return sortedBlocksImpl<I>(blocksize);
-      } else {
-        AD_FAIL();
-        return sortedBlocksImpl<0>(blocksize);
-      }
-    };
-    auto generator =
-        ad_utility::callFixedSize(this->writer_.numColumns(), impl);
-    for (auto& block : generator) {
-      co_yield std::move(block).template toStatic<N>();
-    }
-    /*
     if (!this->transformAndPushLastBlock()) {
       // There was only one block, return it. If a blocksize was explicitly
       // requested for the output, and the single block is larger than this
       // blocksize, we manually have to split it into chunks.
-      auto& block = this->currentBlock_;
+      // TODO<joka921> doesn't need to be const...
+      const auto& block = this->currentBlock_;
       const auto blocksizeOutput = blocksize.value_or(block.numRows());
       if (block.numRows() <= blocksizeOutput) {
-        co_yield std::move(this->currentBlock_).template toStatic<N>();
+        // TODO<joka921> We don't need the copy if we only want to iterate once, make this configurable.
+        auto blockAsStatic = IdTableStatic<N>(this->currentBlock_.clone().template toStatic<N>());
+        co_yield blockAsStatic;
+        //co_yield std::move(this->currentBlock_).template toStatic<N>();
       } else {
         for (size_t i = 0; i < block.numRows(); i += blocksizeOutput) {
           size_t upper = std::min(i + blocksizeOutput, block.numRows());
@@ -713,84 +709,15 @@ class CompressedExternalIdTableSorter
     numPopped += result.numRows();
     co_yield std::move(result).template toStatic<N>();
     AD_CORRECTNESS_CHECK(numPopped == this->numElementsPushed_);
-     */
-  }
-
-  // TODO<joka921> Implement `CallFixedSize` optimization also for the merging.
-  // Transition from the input phase, where `push()` may be called, to the
-  // output phase and return a generator that yields the sorted elements. This
-  // function may be called exactly once.
-  template <size_t N>
-  cppcoro::generator<IdTableStatic<NumStaticCols>> sortedBlocksImpl(
-      std::optional<size_t> blocksize = std::nullopt) {
-    if (!this->transformAndPushLastBlock()) {
-      // There was only one block, return it.
-      co_yield std::move(this->currentBlock_)
-          .template toStatic<NumStaticCols>();
-      co_return;
-    }
-    auto rowGenerators = this->writer_.template getAllRowGenerators<N>();
-
-    const size_t blockSizeOutput =
-        blocksize.value_or(computeBlockSizeForMergePhase(rowGenerators.size()));
-
-    using P = std::pair<decltype(rowGenerators[0].begin()),
-                        decltype(rowGenerators[0].end())>;
-    auto projection = [](const auto& el) -> decltype(auto) {
-      return *el.first;
-    };
-    // NOTE: We have to switch the arguments, because the heap operations by
-    // default order descending...
-    auto comp = [&, this](const auto& a, const auto& b) {
-      return comparator_(projection(b), projection(a));
-    };
-    std::vector<P> pq;
-
-    for (auto& gen : rowGenerators) {
-      pq.emplace_back(gen.begin(), gen.end());
-    }
-    std::ranges::make_heap(pq, comp);
-    IdTableStatic<N> result(this->writer_.numColumns(),
-                            this->writer_.allocator());
-    result.reserve(blockSizeOutput);
-    size_t numPopped = 0;
-    while (!pq.empty()) {
-      std::ranges::pop_heap(pq, comp);
-      auto& min = pq.back();
-      result.push_back(*min.first);
-      ++(min.first);
-      if (min.first == min.second) {
-        pq.pop_back();
-      } else {
-        std::ranges::push_heap(pq, comp);
-      }
-      if (result.size() >= blockSizeOutput) {
-        numPopped += result.numRows();
-        co_yield std::move(result).template toStatic<NumStaticCols>();
-        // The `result` will be moved away, so we have to reset it again.
-        result = IdTableStatic<N>(this->writer_.numColumns(),
-                                  this->writer_.allocator());
-        result.reserve(blockSizeOutput);
-      }
-    }
-    numPopped += result.numRows();
-    co_yield std::move(result).template toStatic<NumStaticCols>();
-    AD_CORRECTNESS_CHECK(numPopped == this->numElementsPushed_);
   }
 
   // _____________________________________________________________
   void sortBlockInPlace(IdTableStatic<NumStaticCols>& block) const {
-    auto doSort = [&]<size_t I>() {
-      auto staticBlock = std::move(block).template toStatic<I>();
 #ifdef _PARALLEL_SORT
-      ad_utility::parallel_sort(staticBlock.begin(), staticBlock.end(),
-                                comparator_);
+    ad_utility::parallel_sort(block.begin(), block.end(), comparator_);
 #else
-      std::ranges::sort(staticBlock, comparator_);
+    std::ranges::sort(block, comparator_);
 #endif
-      block = std::move(staticBlock).template toStatic<NumStaticCols>();
-    };
-    ad_utility::callFixedSize(block.numColumns(), doSort);
   }
 
   // A function with this name is needed by the mixin base class.
@@ -837,4 +764,4 @@ class CompressedExternalIdTableSorter
 };
 }  // namespace ad_utility
 
-#endif  // QLEVER_COMPRESSEDEXTERNALIDTABLE_H
+#endif  // QLEVER_COMPRESSEDEXTERNALIDTABLE_H

src/index/IndexImpl.cpp

@@ -260,6 +260,8 @@ std::unique_ptr<ExternalSorter<SortByPSO, 5>> IndexImpl::buildOspWithPatterns(
       makeSorterPtr<ThirdPermutation, NumColumnsIndexBuilding + 2>("third");
   createSecondPermutationPair(NumColumnsIndexBuilding + 2, isQleverInternalId,
                               std::move(blockGenerator), *thirdSorter);
+
+  makeIndexFromAdditionalTriples(std::move(*hasPatternPredicateSortedByPSO));
   return thirdSorter;
 }
 // _____________________________________________________________________________
@@ -755,8 +757,8 @@ void IndexImpl::createFromOnDiskIndex(const string& onDiskBase) {
   totalVocabularySize_ = vocab_.size() + vocab_.getExternalVocab().size();
   LOG(DEBUG) << "Number of words in internal and external vocabulary: "
              << totalVocabularySize_ << std::endl;
-  pso_.loadFromDisk(onDiskBase_);
-  pos_.loadFromDisk(onDiskBase_);
+  pso_.loadFromDisk(onDiskBase_, false, usePatterns());
+  pos_.loadFromDisk(onDiskBase_, false, usePatterns());
 
   if (loadAllPermutations_) {
     ops_.loadFromDisk(onDiskBase_);
@@ -1545,17 +1547,13 @@ std::optional<PatternCreatorNew::TripleSorter> IndexImpl::createSPOAndSOP(
     // For now (especially for testing) We build the new pattern format as well
     // as the old one to see that they match.
     PatternCreatorNew patternCreator{
-        onDiskBase_ + ".index.patterns.new",
+        onDiskBase_ + ".index.patterns",
         memoryLimitIndexBuilding() / NUM_EXTERNAL_SORTERS_AT_SAME_TIME};
-    PatternCreator patternCreatorOld{onDiskBase_ + ".index.patterns"};
-    auto pushTripleToPatterns = [&patternCreator, &patternCreatorOld,
+    auto pushTripleToPatterns = [&patternCreator,
                                  &isInternalId](const auto& triple) {
       bool ignoreForPatterns = std::ranges::any_of(triple, isInternalId);
       auto tripleArr = std::array{triple[0], triple[1], triple[2]};
       patternCreator.processTriple(tripleArr, ignoreForPatterns);
-      if (!ignoreForPatterns) {
-        patternCreatorOld.processTriple(tripleArr);
-      }
     };
     createPermutationPair(numColumns, AD_FWD(sortedTriples), spo_, sop_,
                           nextSorter.makePushCallback()...,
@@ -1626,7 +1624,7 @@ void IndexImpl::makeIndexFromAdditionalTriples(
     ExternalSorter<SortByPSO>&& additionalTriples) {
   auto onDiskBaseCpy = onDiskBase_;
   onDiskBase_ += ADDITIONAL_TRIPLES_SUFFIX;
-  createPermutationPair(2, std::move(additionalTriples).getSortedBlocks<0>(),
+  createPermutationPair(3, std::move(additionalTriples).getSortedBlocks<0>(),
                         pso_, pos_);
   onDiskBase_ = onDiskBaseCpy;
 }

src/index/Permutation.cpp

@@ -22,7 +22,7 @@ Permutation::Permutation(Enum permutation, Allocator allocator,
 
 // _____________________________________________________________________
 void Permutation::loadFromDisk(const std::string& onDiskBase,
-                               bool onlyLoadAdditional) {
+                               bool onlyLoadAdditional, bool dontLoadAdditional) {
   if (!onlyLoadAdditional) {
     if constexpr (MetaData::_isMmapBased) {
       meta_.setup(onDiskBase + ".index" + fileSuffix_ + MMAP_FILE_SUFFIX,
@@ -46,9 +46,11 @@ void Permutation::loadFromDisk(const std::string& onDiskBase,
               << " permutation: " << meta_.statistics() << std::endl;
     isLoaded_ = true;
   }
-  if (additionalPermutation_) {
+  if (additionalPermutation_ && !dontLoadAdditional) {
     additionalPermutation_->loadFromDisk(onDiskBase + ADDITIONAL_TRIPLES_SUFFIX,
                                          false);
+  } else {
+    additionalPermutation_ = nullptr;
   }
 }
 

src/index/Permutation.h

@@ -57,8 +57,9 @@ class Permutation {
                        HasAdditionalTriples hasAdditionalTriples);
 
   // everything that has to be done when reading an index from disk
+  // TODO<joka921> Why do we need the second argument.
   void loadFromDisk(const std::string& onDiskBase,
-                    bool onlyLoadAdditional = false);
+                    bool onlyLoadAdditional = false, bool dontLoadAdditional = false);
 
   // For a given ID for the col0, retrieve all IDs of the col1 and col2.
   // If `col1Id` is specified, only the col2 is returned for triples that

src/util/Views.h

@@ -10,7 +10,6 @@
 
 #include "util/Generator.h"
 #include "util/Log.h"
-#include "util/Timer.h"
 
 namespace ad_utility {
 
@@ -92,9 +91,7 @@ cppcoro::generator<typename SortedBlockView::value_type> uniqueBlockView(
   size_t numUnique = 0;
   std::optional<ValueType> lastValueFromPreviousBlock = std::nullopt;
 
-  ad_utility::Timer t{ad_utility::Timer::Started};
   for (auto& block : view) {
-    t.cont();
     if (block.empty()) {
       continue;
     }
@@ -109,13 +106,10 @@ cppcoro::generator<typename SortedBlockView::value_type> uniqueBlockView(
     block.erase(it, block.end());
     block.erase(block.begin(), beg);
     numUnique += block.size();
-    t.stop();
     co_yield block;
   }
   LOG(DEBUG) << "Number of inputs to `uniqueView`: " << numInputs << '\n';
   LOG(INFO) << "Number of unique elements: " << numUnique << std::endl;
-  LOG(INFO) << "Time actually spent for unique computation: "
-            << t.msecs().count() << "ms" << std::endl;
 }
 
 // A view that owns its underlying storage. It is a rather simple drop-in

test/AddCombinedRowToTableTest.cpp

@@ -26,7 +26,7 @@ TEST(AddCombinedRowToTable, OneJoinColumn) {
         makeIdTableFromVector({{7, 14, 0}, {9, 10, 1}, {14, 8, 2}, {33, 5, 3}});
     auto result = makeIdTableFromVector({});
     result.setNumColumns(4);
-    auto adder = ad_utility::AddCombinedRowToIdTable<ad_utility::Noop>(
+    auto adder = ad_utility::AddCombinedRowToIdTable(
         1, left.asStaticView<0>(), right.asStaticView<0>(), std::move(result),
         bufferSize);
     adder.addRow(1, 0);

test/CMakeLists.txt

@@ -225,9 +225,6 @@ addLinkAndDiscoverTest(VocabularyTest index)
 
 addLinkAndDiscoverTest(IteratorTest)
 
-# Here we also seem to have race conditions on the tests
-addLinkAndDiscoverTestSerial(PatternCreatorTest index)
-
 # Stxxl currently always uses a file ./-stxxl.disk for all indices, which
 # makes it impossible to run the test cases for the Index class in parallel.
 # TODO<qup42, joka921> fix this

test/HasPredicateScanTest.cpp

@@ -2,6 +2,7 @@
 // Chair of Algorithms and Data Structures.
 // Author: Florian Kramer ([email protected])
 
+#if false
 #include <gtest/gtest.h>
 
 #include <algorithm>
@@ -361,3 +362,5 @@ TEST(CountAvailablePredicates, patternTrickTest) {
   ASSERT_EQ(V(4u), result[4][0]);
   ASSERT_EQ(Int(3u), result[4][1]);
 }
+
+#endif

test/IndexTest.cpp

@@ -194,7 +194,7 @@ TEST(CreatePatterns, createPatterns) {
         "<a2> <d>  <c2> .";
 
     const Index& indexNoImpl = getQec(kb)->getIndex();
-    const IndexImpl& index = indexNoImpl.getImpl();
+    //const IndexImpl& index = indexNoImpl.getImpl();
 
     auto getId = ad_utility::testing::makeGetId(indexNoImpl);
     // Pattern p0 (for subject <a>) consists of <b> and <b2)