Fix a bug in the VariableToColumnMap of TransitivePath (#1432)

There was a subtle and long-standing bug in the computation of the result width of a transititve path operation. This is now fixed. This fixes queries like https://qlever.cs.uni-freiburg.de/wikidata/zCHu9V (which without this fix has its third column wrong) and https://qlever.cs.uni-freiburg.de/wikidata/S1XA29 (which without this fix runs into an assertion failue).
ad-freiburg · Aug 3, 2024 · 8a68213 · 8a68213
1 parent 76bb412
commit 8a68213
Show file tree

Hide file tree

Showing 2 changed files with 56 additions and 25 deletions.
diff --git a/src/engine/TransitivePathBase.cpp b/src/engine/TransitivePathBase.cpp
@@ -378,8 +378,8 @@ std::shared_ptr<TransitivePathBase> TransitivePathBase::bindLeftOrRightSide(
 
     AD_CORRECTNESS_CHECK(!p->variableColumns_.contains(variable));
     p->variableColumns_[variable] = columnIndexWithType;
-    p->resultWidth_++;
   }
+  p->resultWidth_ += leftOrRightOp->getResultWidth() - 1;
   return std::move(p);
 }
 
@@ -397,6 +397,8 @@ void TransitivePathBase::copyColumns(const IdTableView<INPUT_WIDTH>& inputTable,
                                      size_t skipCol) const {
   size_t inCol = 0;
   size_t outCol = 2;
+  AD_CORRECTNESS_CHECK(skipCol < inputTable.numColumns());
+  AD_CORRECTNESS_CHECK(inputTable.numColumns() + 1 == outputTable.numColumns());
   while (inCol < inputTable.numColumns() && outCol < outputTable.numColumns()) {
     if (skipCol == inCol) {
       inCol++;

diff --git a/test/TransitivePathTest.cpp b/test/TransitivePathTest.cpp
@@ -247,14 +247,27 @@ TEST_P(TransitivePathTest, idToLeftBound) {
 
   TransitivePathSide left(std::nullopt, 0, Variable{"?start"}, 0);
   TransitivePathSide right(std::nullopt, 1, V(4), 1);
-  auto T = makePathLeftBound(
-      std::move(sub), {Variable{"?start"}, Variable{"?target"}},
-      std::move(leftOpTable), 1, {Variable{"?x"}, Variable{"?start"}},
-      std::move(left), std::move(right), 0, std::numeric_limits<size_t>::max());
-
-  auto resultTable = T->computeResultOnlyForTesting();
-  ASSERT_THAT(resultTable.idTable(),
-              ::testing::UnorderedElementsAreArray(expected));
+  {
+    auto T = makePathLeftBound(
+        sub.clone(), {Variable{"?start"}, Variable{"?target"}},
+        leftOpTable.clone(), 1, {Variable{"?x"}, Variable{"?start"}}, left,
+        right, 0, std::numeric_limits<size_t>::max());
+
+    auto resultTable = T->computeResultOnlyForTesting();
+    ASSERT_THAT(resultTable.idTable(),
+                ::testing::UnorderedElementsAreArray(expected));
+  }
+  {
+    auto T = makePathLeftBound(
+        std::move(sub), {Variable{"?start"}, Variable{"?target"}},
+        std::move(leftOpTable), 1, {std::nullopt, Variable{"?start"}},
+        std::move(left), std::move(right), 0,
+        std::numeric_limits<size_t>::max());
+
+    auto resultTable = T->computeResultOnlyForTesting();
+    ASSERT_THAT(resultTable.idTable(),
+                ::testing::UnorderedElementsAreArray(expected));
+  }
 }
 
 TEST_P(TransitivePathTest, idToRightBound) {
@@ -280,14 +293,27 @@ TEST_P(TransitivePathTest, idToRightBound) {
 
   TransitivePathSide left(std::nullopt, 0, V(0), 0);
   TransitivePathSide right(std::nullopt, 1, Variable{"?target"}, 1);
-  auto T = makePathRightBound(
-      std::move(sub), {Variable{"?start"}, Variable{"?target"}},
-      std::move(rightOpTable), 0, {Variable{"?target"}, Variable{"?x"}},
-      std::move(left), std::move(right), 0, std::numeric_limits<size_t>::max());
-
-  auto resultTable = T->computeResultOnlyForTesting();
-  ASSERT_THAT(resultTable.idTable(),
-              ::testing::UnorderedElementsAreArray(expected));
+  {
+    auto T = makePathRightBound(
+        sub.clone(), {Variable{"?start"}, Variable{"?target"}},
+        rightOpTable.clone(), 0, {Variable{"?target"}, Variable{"?x"}}, left,
+        right, 0, std::numeric_limits<size_t>::max());
+
+    auto resultTable = T->computeResultOnlyForTesting();
+    ASSERT_THAT(resultTable.idTable(),
+                ::testing::UnorderedElementsAreArray(expected));
+  }
+  {
+    auto T = makePathRightBound(
+        std::move(sub), {Variable{"?start"}, Variable{"?target"}},
+        std::move(rightOpTable), 0, {Variable{"?target"}, std::nullopt},
+        std::move(left), std::move(right), 0,
+        std::numeric_limits<size_t>::max());
+
+    auto resultTable = T->computeResultOnlyForTesting();
+    ASSERT_THAT(resultTable.idTable(),
+                ::testing::UnorderedElementsAreArray(expected));
+  }
 }
 
 TEST_P(TransitivePathTest, leftBoundToVar) {
@@ -318,14 +344,17 @@ TEST_P(TransitivePathTest, leftBoundToVar) {
 
   TransitivePathSide left(std::nullopt, 0, Variable{"?start"}, 0);
   TransitivePathSide right(std::nullopt, 1, Variable{"?target"}, 1);
-  auto T = makePathLeftBound(
-      std::move(sub), {Variable{"?start"}, Variable{"?target"}},
-      std::move(leftOpTable), 1, {Variable{"?x"}, Variable{"?start"}},
-      std::move(left), std::move(right), 0, std::numeric_limits<size_t>::max());
-
-  auto resultTable = T->computeResultOnlyForTesting();
-  ASSERT_THAT(resultTable.idTable(),
-              ::testing::UnorderedElementsAreArray(expected));
+  {
+    auto T = makePathLeftBound(
+        std::move(sub), {Variable{"?start"}, Variable{"?target"}},
+        std::move(leftOpTable), 1, {Variable{"?x"}, Variable{"?start"}},
+        std::move(left), std::move(right), 0,
+        std::numeric_limits<size_t>::max());
+
+    auto resultTable = T->computeResultOnlyForTesting();
+    ASSERT_THAT(resultTable.idTable(),
+                ::testing::UnorderedElementsAreArray(expected));
+  }
 }
 
 TEST_P(TransitivePathTest, rightBoundToVar) {