From d0cdbac5517265ab6c470a7a01447fc307f7eb16 Mon Sep 17 00:00:00 2001 From: Johannes Kalmbach Date: Thu, 30 Jan 2025 09:17:02 +0100 Subject: [PATCH 1/3] Don't use the size estimate from the cache. Signed-off-by: Johannes Kalmbach --- src/engine/QueryExecutionTree.cpp | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/src/engine/QueryExecutionTree.cpp b/src/engine/QueryExecutionTree.cpp index 9b58d0bb78..49ceb340aa 100644 --- a/src/engine/QueryExecutionTree.cpp +++ b/src/engine/QueryExecutionTree.cpp @@ -91,15 +91,11 @@ size_t QueryExecutionTree::getCostEstimate() { // _____________________________________________________________________________ size_t QueryExecutionTree::getSizeEstimate() { if (!sizeEstimate_.has_value()) { - if (cachedResult_) { - AD_CORRECTNESS_CHECK(cachedResult_->isFullyMaterialized()); - sizeEstimate_ = cachedResult_->idTable().size(); - } else { - // if we are in a unit test setting and there is no QueryExecutionContest - // specified it is the rootOperation_'s obligation to handle this case - // correctly - sizeEstimate_ = rootOperation_->getSizeEstimate(); - } + // Note: Previously we used the exact size instead of the estimate for + // results that were already in the cache. This however often lead to poor + // planning, because the query planner compared exact sizes with estimates, + // which lead to worse plans than just conistently choosing the estimate. + sizeEstimate_ = rootOperation_->getSizeEstimate(); } return sizeEstimate_.value(); } From 24f2812daadd679f456169b6d15b0881447ce2e9 Mon Sep 17 00:00:00 2001 From: Johannes Kalmbach Date: Thu, 30 Jan 2025 15:27:34 +0100 Subject: [PATCH 2/3] Fixed the test failure Signed-off-by: Johannes Kalmbach --- test/OperationTest.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/OperationTest.cpp b/test/OperationTest.cpp index 2afbe38a83..e39b9312e0 100644 --- a/test/OperationTest.cpp +++ b/test/OperationTest.cpp @@ -242,12 +242,12 @@ TEST(OperationTest, estimatesForCachedResults) { [[maybe_unused]] auto res = qet->getResult(); } // The result is now cached inside the static execution context, if we create - // the same operation again, the cost estimate is 0 and the size estimate is - // exact (3 rows). + // the same operation again, the cost estimate is 0. The size estimate doesn't + // change (see the `getCostEstimate` function for details on why). { auto qet = makeQet(); EXPECT_EQ(qet->getCacheKey(), qet->getRootOperation()->getCacheKey()); - EXPECT_EQ(qet->getSizeEstimate(), 3u); + EXPECT_EQ(qet->getSizeEstimate(), 24u); EXPECT_EQ(qet->getCostEstimate(), 0u); } } From e731afecfc773443dcde43e7d5135ab75fb4ed87 Mon Sep 17 00:00:00 2001 From: Johannes Kalmbach Date: Thu, 30 Jan 2025 18:56:32 +0100 Subject: [PATCH 3/3] Also update the GitHub artifact action. Signed-off-by: Johannes Kalmbach --- .github/workflows/sparql-conformance.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/sparql-conformance.yml b/.github/workflows/sparql-conformance.yml index 38152b5c79..3e4bdfd63d 100644 --- a/.github/workflows/sparql-conformance.yml +++ b/.github/workflows/sparql-conformance.yml @@ -80,7 +80,7 @@ jobs: echo ${{github.event.pull_request.head.sha}} > ./conformance-report/sha mv ${{ github.workspace}}/qlever-test-suite/results/${{ github.sha }}.json.bz2 conformance-report/${{ github.event.pull_request.head.sha }}.json.bz2 - name: Upload coverage artifact - uses: actions/upload-artifact@v3 + uses: actions/upload-artifact@v4 with: name: conformance-report path: conformance-report/ \ No newline at end of file