Repository: ad-freiburg/qlever (language: python)

Issue:
Wrong dates for birth date property
QLever outputs the same date for all results in this query and they are all wrong.
https://qlever.cs.uni-freiburg.de/wikidata/f6DCiu


Discussion / hints:
@dpriskorn Your query is missing the `GROUP BY`. QLever's only mistake for his query is that it doesn't reject it as malformed. If you ask the same query on WDQS, you will get an error message.

Also, the labels should be added after the `GROUP BY`.

Here is the corrected query: https://qlever.cs.uni-freiburg.de/wikidata/tTyH5S

Fix patch:
diff --git a/src/engine/sparqlExpressions/SampleExpression.cpp b/src/engine/sparqlExpressions/SampleExpression.cpp
index 928678407..e32c0e6bb 100644
--- a/src/engine/sparqlExpressions/SampleExpression.cpp
+++ b/src/engine/sparqlExpressions/SampleExpression.cpp
@@ -1,9 +1,6 @@
-//  Copyright 2021, University of Freiburg, Chair of Algorithms and Data
-//  Structures. Author: Johannes Kalmbach <kalmbacj@cs.uni-freiburg.de>
-
-//
-// Created by johannes on 15.09.21.
-//
+//  Copyright 2021, University of Freiburg,
+//                  Chair of Algorithms and Data Structures.
+//  Author: Johannes Kalmbach <kalmbacj@cs.uni-freiburg.de>
 
 #include "./SampleExpression.h"
 
@@ -16,15 +13,18 @@ using namespace sparqlExpression::detail;
 ExpressionResult SampleExpression::evaluate(EvaluationContext* context) const {
   auto evaluator =
       [context]<typename T>(const T& childResult) -> ExpressionResult {
+    if (getResultSize(*context, childResult) == 0) {
+      return Id::makeUndefined();
+    }
     if constexpr (std::is_same_v<T, ad_utility::SetOfIntervals>) {
       // If there exists an element that is true, return true.
       return Id::makeFromBool(!childResult._intervals.empty());
     } else if constexpr (isVectorResult<T>) {
-      AD_CONTRACT_CHECK(!childResult.empty());
+      AD_CORRECTNESS_CHECK(!childResult.empty());
       return childResult[0];
     } else if constexpr (std::is_same_v<T, ::Variable>) {
       // TODO<joka921> Can't this be a simpler function (getIdAt)
-      AD_CONTRACT_CHECK(context->_endIndex > context->_beginIndex);
+      AD_CORRECTNESS_CHECK(context->_endIndex > context->_beginIndex);
       std::span<const ValueId> idOfFirstAsVector = detail::getIdsFromVariable(
           childResult, context, context->_beginIndex, context->_endIndex);
       return ExpressionResult{idOfFirstAsVector[0]};
diff --git a/src/engine/sparqlExpressions/SampleExpression.h b/src/engine/sparqlExpressions/SampleExpression.h
index 8fb0e2fe6..11dce6aff 100644
--- a/src/engine/sparqlExpressions/SampleExpression.h
+++ b/src/engine/sparqlExpressions/SampleExpression.h
@@ -1,12 +1,7 @@
 //  Copyright 2021, University of Freiburg, Chair of Algorithms and Data
 //  Structures. Author: Johannes Kalmbach <kalmbacj@cs.uni-freiburg.de>
 
-//
-// Created by johannes on 15.09.21.
-//
-
-#ifndef QLEVER_SAMPLEEXPRESSION_H
-#define QLEVER_SAMPLEEXPRESSION_H
+#pragma once
 
 #include "./SparqlExpression.h"
 #include "absl/strings/str_cat.h"
@@ -37,9 +32,10 @@ class SampleExpression : public SparqlExpression {
   // __________________________________________________________________________
   std::span<Ptr> childrenImpl() override { return {&_child, 1}; }
 
+  // SAMPLE is an aggregate.
+  bool isAggregate() const override { return true; }
+
  private:
   Ptr _child;
 };
 }  // namespace sparqlExpression
-
-#endif  // QLEVER_SAMPLEEXPRESSION_H


Test patch:
diff --git a/test/AggregateExpressionTest.cpp b/test/AggregateExpressionTest.cpp
index e4fdbebcb..9a9a16d91 100644
--- a/test/AggregateExpressionTest.cpp
+++ b/test/AggregateExpressionTest.cpp
@@ -10,6 +10,7 @@
 #include "engine/ValuesForTesting.h"
 #include "engine/sparqlExpressions/AggregateExpression.h"
 #include "engine/sparqlExpressions/CountStarExpression.h"
+#include "engine/sparqlExpressions/SampleExpression.h"
 #include "gtest/gtest.h"
 
 using namespace sparqlExpression;
@@ -145,7 +146,7 @@ TEST(AggregateExpression, max) {
   testMaxId({}, U);
 }
 
-// ______________________________________________________________________________
+// _____________________________________________________________________________
 TEST(AggregateExpression, CountStar) {
   auto t = TestContext{};
   // A matcher that first clears the cache and then checks that the result of
@@ -227,7 +228,7 @@ TEST(AggregateExpression, CountStar) {
   EXPECT_THAT(m, matcher(0));
 }
 
-// ______________________________________________________________________________
+// _____________________________________________________________________________
 TEST(AggregateExpression, CountStarSimpleMembers) {
   using namespace sparqlExpression;
   auto m = makeCountStarExpression(false);
@@ -240,3 +241,54 @@ TEST(AggregateExpression, CountStarSimpleMembers) {
   EXPECT_THAT(m->getUnaggregatedVariables(), ::testing::IsEmpty());
   EXPECT_TRUE(m->isAggregate());
 }
+
+// _____________________________________________________________________________
+TEST(AggregateExpression, SampleExpression) {
+  using namespace sparqlExpression;
+  auto makeSample = [](ExpressionResult result) {
+    return std::make_unique<SampleExpression>(
+        false, std::make_unique<SingleUseExpression>(std::move(result)));
+  };
+
+  auto testSample = [&](ExpressionResult input, ExpressionResult expected) {
+    TestContext testContext;
+    std::visit(
+        [&testContext]<typename T>(const T& t) {
+          if constexpr (ad_utility::isInstantiation<T, VectorWithMemoryLimit>) {
+            testContext.context._endIndex = t.size();
+          }
+        },
+        input);
+    auto result = makeSample(std::move(input))->evaluate(&testContext.context);
+    EXPECT_EQ(result, expected);
+  };
+
+  testSample(I(3), I(3));
+  auto v = VectorWithMemoryLimit<Id>(makeAllocator());
+  v.push_back(I(34));
+  v.push_back(I(42));
+  testSample(v.clone(), I(34));
+  testSample(ad_utility::SetOfIntervals{}, BoolId(false));
+  testSample(ad_utility::SetOfIntervals{{{3, 17}}}, BoolId(true));
+  v.clear();
+  testSample(v.clone(), U);
+  // The first value of the ?ints variable inside the `TestContext` is `1`.
+  testSample(Variable{"?ints"}, I(1));
+}
+
+// _____________________________________________________________________________
+TEST(AggregateExpression, SampleExpressionSimpleMembers) {
+  using namespace sparqlExpression;
+  auto makeSample = [](Id result) {
+    return std::make_unique<SampleExpression>(
+        false, std::make_unique<IdExpression>(std::move(result)));
+  };
+
+  auto sample = makeSample(I(3478));
+  EXPECT_TRUE(sample->isAggregate());
+  EXPECT_TRUE(sample->getUnaggregatedVariables().empty());
+  EXPECT_EQ(sample->children().size(), 1u);
+  using namespace ::testing;
+  EXPECT_THAT(sample->getCacheKey({}),
+              AllOf(HasSubstr("SAMPLE"), HasSubstr("#valueId")));
+}