changes for code-review (1)

Author: realHannes

src/engine/sparqlExpressions/NaryExpressionImpl.h

@@ -41,8 +41,7 @@ class NaryExpression : public SparqlExpression {
       const VariableToColumnMap& varColMap) const override;
 
   // _________________________________________________________________________
-  std::optional<SparqlExpression*> getChildAtIndex(
-      size_t childIndex) const override {
+  std::optional<SparqlExpression*> getChildAtIndex(size_t childIndex) const {
     return childIndex < N ? std::make_optional(children_[childIndex].get())
                           : std::nullopt;
   }

src/engine/sparqlExpressions/PrefilterExpressionIndex.cpp

@@ -156,7 +156,8 @@ static std::string getRelationalOpStr(const CompOp relOp) {
     case GT:
       return "GT(>)";
     default:
-      AD_FAIL();
+      return absl::StrCat("Undefined CompOp value: ", static_cast<int>(relOp),
+                          ".");
   }
 }
 
@@ -501,11 +502,15 @@ void checkPropertiesForPrefilterConstruction(
 }
 
 //______________________________________________________________________________
-template <CompOp comparison>
-static std::unique_ptr<PrefilterExpression> makePrefilterExpressionYearImpl(
-    const int year) {
+std::unique_ptr<PrefilterExpression> makePrefilterExpressionYearImpl(
+    CompOp comparison, const int year) {
   using GeExpr = GreaterEqualExpression;
   using LtExpr = LessThanExpression;
+
+  // `getDateId` returns an `Id` containing the smallest possible `Date`
+  // (`xsd::date`) for which `YEAR(Id) == adjustedYear` is valid. This `Id` acts
+  // as a reference bound for the actual `DateYearOrDuration` prefiltering
+  // procedure.
   const auto getDateId = [](const int adjustedYear) {
     return Id::makeFromDate(DateYearOrDuration(Date(adjustedYear, 0, 0)));
   };
@@ -526,42 +531,69 @@ static std::unique_ptr<PrefilterExpression> makePrefilterExpressionYearImpl(
       return make<OrExpression>(make<LtExpr>(getDateId(year)),
                                 make<GeExpr>(getDateId(year + 1)));
     default:
-      AD_FAIL();
+      throw std::runtime_error(
+          absl::StrCat("Set unkown (relational) comparison operator for "
+                       "the creation of PrefilterExpression on date-values: ",
+                       getRelationalOpStr(comparison)));
   }
 };
 
 //______________________________________________________________________________
 template <CompOp comparison>
 static std::unique_ptr<PrefilterExpression> makePrefilterExpressionVecImpl(
-    const IdOrLocalVocabEntry& referenceValue, bool prefilterDate) {
+    const IdOrLocalVocabEntry& referenceValue, bool prefilterDateByYear) {
+  using enum Datatype;
   // Standard pre-filtering procedure.
-  if (!prefilterDate) {
+  if (!prefilterDateByYear) {
     return make<RelationalExpression<comparison>>(referenceValue);
   }
-
+  // Helper to savely retrieve `ValueId/Id` values from the provided
+  // `IdOrLocalVocabEntry referenceValue` if contained. Given no `ValueId` is
+  // contained, a explanatory message per `std::runtime_error` is thrown.
+  const auto retrieveValueIdOrThrowErr =
+      [](const IdOrLocalVocabEntry& referenceValue) {
+        return std::visit(
+            []<typename T>(const T& value) -> ValueId {
+              if constexpr (std::is_same_v<std::decay_t<T>, ValueId>) {
+                return value;
+              } else if constexpr (std::is_same_v<std::decay_t<T>,
+                                                  LocalVocabEntry>) {
+                throw std::runtime_error(absl::StrCat(
+                    "Provided Literal or Iri with value: ",
+                    value.asLiteralOrIri().toStringRepresentation(),
+                    ". This is an invalid reference value for filtering date "
+                    "values over expression YEAR. Please provide an integer "
+                    "value as reference year."));
+              }
+              throw std::runtime_error(
+                  "Reference value IdOrLocalVocabEntry contains unknown type.");
+            },
+            referenceValue);
+      };
   // Handle year extraction and return a date-value adjusted
   // `PrefilterExpression` if possible. Given an unsuitable reference value was
   // provided, throw a std::runtime_error with an explanatory message.
-  const auto retrieveYearIntOrThrowRuntimerErr =
-      [](const IdOrLocalVocabEntry& referenceValue) {
-        using enum Datatype;
-        if (auto* valueId = std::get_if<ValueId>(&referenceValue);
-            valueId && valueId->getDatatype() == Int) {
-          return valueId->getInt();
+  const auto retrieveYearIntOrThrowErr =
+      [&retrieveValueIdOrThrowErr](const IdOrLocalVocabEntry& referenceValue) {
+        const ValueId& valueId = retrieveValueIdOrThrowErr(referenceValue);
+        if (valueId.getDatatype() == Int) {
+          return valueId.getInt();
         }
-        throw std::runtime_error(
-            "Pre-filtering DATETIME/DATE values over YEAR failed: requires "
-            "INTEGER reference value.");
+        throw std::runtime_error(absl::StrCat(
+            "Reference value for filtering date values over "
+            "expression YEAR is of invalid datatype: ",
+            toString(valueId.getDatatype()),
+            ".\nPlease provide an integer value as reference year."));
       };
-  return makePrefilterExpressionYearImpl<comparison>(
-      retrieveYearIntOrThrowRuntimerErr(referenceValue));
+  return makePrefilterExpressionYearImpl(
+      comparison, retrieveYearIntOrThrowErr(referenceValue));
 };
 
 //______________________________________________________________________________
 template <CompOp comparison>
 std::vector<PrefilterExprVariablePair> makePrefilterExpressionVec(
     const IdOrLocalVocabEntry& referenceValue, const Variable& variable,
-    bool mirrored, bool prefilterDate) {
+    bool mirrored, bool prefilterDateByYear) {
   using enum CompOp;
   std::vector<PrefilterExprVariablePair> resVec{};
   if (mirrored) {
@@ -573,12 +605,12 @@ std::vector<PrefilterExprVariablePair> makePrefilterExpressionVec(
     constexpr ad_utility::ConstexprMap<CompOp, CompOp, 6> mirrorMap(
         {P{LT, GT}, P{LE, GE}, P{GE, LE}, P{GT, LT}, P{EQ, EQ}, P{NE, NE}});
     resVec.emplace_back(
-        makePrefilterExpressionVecImpl<mirrorMap.at(comparison)>(referenceValue,
-                                                                 prefilterDate),
+        makePrefilterExpressionVecImpl<mirrorMap.at(comparison)>(
+            referenceValue, prefilterDateByYear),
         variable);
   } else {
     resVec.emplace_back(makePrefilterExpressionVecImpl<comparison>(
-                            referenceValue, prefilterDate),
+                            referenceValue, prefilterDateByYear),
                         variable);
   }
   return resVec;

src/engine/sparqlExpressions/PrefilterExpressionIndex.h

@@ -252,19 +252,34 @@ using PrefilterExprVariablePair =
 void checkPropertiesForPrefilterConstruction(
     const std::vector<PrefilterExprVariablePair>& vec);
 
+//______________________________________________________________________________
+// Use for testing only. The definition here is necessary to call
+// `makePrefilterYearImpl` with an invalid `CompOp` argument given that it is
+// internally in use over the `CompOp` save template instantiated
+// `makePrefilterExpressionVec`. Use `makePrefilterExpressionVec` to retrieve
+// the actual `PrefilterExpression` (with corresponding `Variable`).
+std::unique_ptr<PrefilterExpression> makePrefilterExpressionYearImpl(
+    CompOp comparison, const int year);
+
 //______________________________________________________________________________
 // Creates a `RelationalExpression<comparison>` prefilter expression based on
-// the templated `CompOp` comparison operation and the reference
+// the specified `CompOp` comparison operation and the reference
 // `IdOrLocalVocabEntry` value. With the next step, the corresponding
 // `<RelationalExpression<comparison>, Variable>` pair is created, and finally
-// returned in a vector. The `mirrored` flag indicates if the given
-// `RelationalExpression<comparison>` should be mirrored. The mirroring
-// procedure changes the (asymmetrical) comparison operations:
-// e.g. `5 < ?x` is changed to `?x > 5`.
+// returned in a vector.
+// The `mirrored` flag indicates if the given `RelationalExpression<comparison>`
+// should be mirrored. The mirroring procedure changes the (asymmetrical)
+// comparison operations: e.g. `5 < ?x` is changed to `?x > 5`.
+// The `prefilterDateByYear` flag indicates that the constructed
+// `PrefilterExpression<comp>` needs to consider `Date`/`DateYearOrDuration`
+// value ranges for the `BlockMetadata` pre-selection (w.r.t. the year provided
+// as an Int-`ValueId` over `referenceValue`). This requires a slightly
+// different logic when constructing the expression, hence set
+// `prefilterDateByYear` as an indicator.
 template <CompOp comparison>
 std::vector<PrefilterExprVariablePair> makePrefilterExpressionVec(
     const IdOrLocalVocabEntry& referenceValue, const Variable& variable,
-    bool mirrored, bool prefilterDate = false);
+    bool mirrored, bool prefilterDateByYear = false);
 
 }  // namespace detail
 }  // namespace prefilterExpressions

src/engine/sparqlExpressions/RelationalExpressions.cpp

@@ -425,6 +425,33 @@ ExpressionResult InExpression::evaluate(
   return result;
 }
 
+// _____________________________________________________________________________
+// (1) the provided `SparqlExpression* child` is a direct `Variable` expression
+// (e.g. `?x`), return `<Variable, false>`.
+// (2) the provided `SparqlExpression* child` is an expression `YEAR` which
+// referes to a `Variable` value (e.g. `YEAR(?x)`), return `<Variable, true>`.
+// (3) None of the previous expression cases, return default `std::nullopt`.
+// The `bool` flag is relevant later on to differentiate w.r.t. the logic that
+// needs to be applied for the creation of `PrefilterExpression`.
+static std::optional<std::pair<Variable, bool>> getOptVariableAndIsYear(
+    const SparqlExpression* child) {
+  // (1) The direct child already contains the Variable.
+  if (auto optVariable = child->getVariableOrNullopt(); optVariable) {
+    return std::make_pair(optVariable.value(), false);
+  }
+  if (!child->isYearExpression()) return std::nullopt;
+  // (2) The direct child is an expression YEAR();
+  // If possible retrieve the Variable related to YEAR().
+  const auto& grandChild = child->children();
+  // The expression YEAR() should by definition hold a single child.
+  AD_CORRECTNESS_CHECK(grandChild.size() == 1);
+  if (auto optVariable = grandChild[0]->getVariableOrNullopt(); optVariable) {
+    return std::make_pair(optVariable.value(), true);
+  }
+  // No Variable for pre-filtering available.
+  return std::nullopt;
+}
+
 // _____________________________________________________________________________
 template <Comparison comp>
 std::vector<PrefilterExprVariablePair>
@@ -434,44 +461,17 @@ RelationalExpression<comp>::getPrefilterExpressionForMetadata(
   const SparqlExpression* child0 = children_.at(0).get();
   const SparqlExpression* child1 = children_.at(1).get();
 
-  const auto getOptVariableAndIsYear = [](const SparqlExpression* child)
-      -> std::optional<std::pair<Variable, bool>> {
-    // (1) The direct child already contains the Variable.
-    if (auto optVariable = child->getVariableOrNullopt(); optVariable) {
-      return std::make_pair(optVariable.value(), false);
-    }
-    // (2) The direct child is an expression YEAR();
-    // If possible retrieve the Variable related to YEAR().
-    if (child->isYearExpression()) {
-      std::optional<SparqlExpression*> optGrandChild =
-          child->getChildAtIndex(0);
-      // The expression YEAR() should by definition hold a child.
-      AD_CORRECTNESS_CHECK(optGrandChild.has_value());
-      if (auto optVariable = optGrandChild.value()->getVariableOrNullopt();
-          optVariable) {
-        return std::make_pair(optVariable.value(), true);
-      }
-    }
-    // No Variable for pre-filtering available.
-    return std::nullopt;
-  };
-
   const auto tryGetPrefilterExprVariablePairVec =
-      [&getOptVariableAndIsYear](
-          const SparqlExpression* child0, const SparqlExpression* child1,
-          bool reversed) -> std::vector<PrefilterExprVariablePair> {
-    if (auto optVariableIsYearPair = getOptVariableAndIsYear(child0);
-        optVariableIsYearPair) {
-      const auto& [variable, prefilterDate] = optVariableIsYearPair.value();
-      const auto& optReferenceValue =
-          detail::getIdOrLocalVocabEntryFromLiteralExpression(child1);
-      if (!optReferenceValue.has_value()) {
-        return {};
-      }
-      return prefilterExpressions::detail::makePrefilterExpressionVec<comp>(
-          optReferenceValue.value(), variable, reversed, prefilterDate);
-    }
-    return {};
+      [](const SparqlExpression* child0, const SparqlExpression* child1,
+         bool reversed) -> std::vector<PrefilterExprVariablePair> {
+    const auto& optVariableIsYearPair = getOptVariableAndIsYear(child0);
+    if (!optVariableIsYearPair.has_value()) return {};
+    const auto& [variable, prefilterDate] = optVariableIsYearPair.value();
+    const auto& optReferenceValue =
+        detail::getIdOrLocalVocabEntryFromLiteralExpression(child1);
+    if (!optReferenceValue.has_value()) return {};
+    return prefilterExpressions::detail::makePrefilterExpressionVec<comp>(
+        optReferenceValue.value(), variable, reversed, prefilterDate);
   };
   // Option 1:
   // RelationalExpression containing a VariableExpression as the first child

src/engine/sparqlExpressions/SparqlExpression.cpp

@@ -65,12 +65,6 @@ std::unique_ptr<SparqlExpression> SparqlExpression::replaceChild(
   return std::exchange(children()[childIndex], std::move(newExpression));
 }
 
-// _____________________________________________________________________________
-std::optional<SparqlExpression*> SparqlExpression::getChildAtIndex(
-    [[maybe_unused]] size_t childIndex) const {
-  return std::nullopt;
-};
-
 // _____________________________________________________________________________
 const string& SparqlExpression::descriptor() const { return _descriptor; }
 

src/engine/sparqlExpressions/SparqlExpression.h

@@ -60,13 +60,6 @@ class SparqlExpression {
   virtual std::unique_ptr<SparqlExpression> replaceChild(
       size_t childIndex, std::unique_ptr<SparqlExpression> newExpression);
 
-  // Returns the child at index `childIndex` for this expression.
-  // By default the implementation of this method returns `std::nullopt`.
-  // If the given `childIndex` is out of bounds for the respective expression,
-  // `std::nullopt` will be returned as well.
-  virtual std::optional<SparqlExpression*> getChildAtIndex(
-      size_t childIndex) const;
-
   // Get a unique identifier for this expression, used as cache key.
   virtual string getCacheKey(const VariableToColumnMap& varColMap) const = 0;
 

test/GetPrefilterExpressionFromSparqlExpressionTest.cpp

@@ -523,24 +523,37 @@ TEST(GetPrefilterExpressionFromSparqlExpression, tryGetPrefilterExprForDate) {
   evalAndEqualityCheck(
       eqSprql(yearSprqlExpr(ltSprql(var, IntId(2025))), IntId(2025)));
 
-  auto assertThrowsError = [](std::unique_ptr<SparqlExpression> expr) {
-    try {
-      expr->getPrefilterExpressionForMetadata();
-      FAIL() << "Expected std::runtime_error";
-    } catch (const std::runtime_error& err) {
-      EXPECT_STREQ(err.what(),
-                   "Pre-filtering DATETIME/DATE values over YEAR failed: "
-                   "requires INTEGER reference value.");
-    } catch (...) {
-      FAIL() << "Different error to std::runtime_error was thrown.";
-    }
+  auto assertThrowsError = [](std::unique_ptr<SparqlExpression> expr,
+                              const std::string& runtimeErrorMessage) {
+    AD_EXPECT_THROW_WITH_MESSAGE(expr->getPrefilterExpressionForMetadata(),
+                                 ::testing::Eq(runtimeErrorMessage));
   };
   // Test SparqlExpressions for which we expect that the reference value-type
   // error is thrown.
-  assertThrowsError(eqSprql(yearSprqlExpr(var), I("<iri>")));
-  assertThrowsError(gtSprql(yearSprqlExpr(var), I("<iri>")));
-  assertThrowsError(ltSprql(yearSprqlExpr(var), Id::makeFromBool(false)));
-  assertThrowsError(neqSprql(yearSprqlExpr(var), Id::makeUndefined()));
+  assertThrowsError(
+      eqSprql(yearSprqlExpr(var), I("<iri>")),
+      "Provided Literal or Iri with value: <iri>. This is an invalid reference "
+      "value for filtering date values over expression YEAR. Please provide an "
+      "integer value as reference year.");
+  assertThrowsError(
+      gtSprql(yearSprqlExpr(var), I("<iri>")),
+      "Provided Literal or Iri with value: <iri>. This is an invalid reference "
+      "value for filtering date values over expression YEAR. Please provide an "
+      "integer value as reference year.");
+  assertThrowsError(
+      neqSprql(yearSprqlExpr(var), L("\"lit value\"")),
+      "Provided Literal or Iri with value: \"lit value\". This is an invalid "
+      "reference "
+      "value for filtering date values over expression YEAR. Please provide an "
+      "integer value as reference year.");
+  assertThrowsError(ltSprql(yearSprqlExpr(var), Id::makeFromBool(false)),
+                    "Reference value for filtering date values over expression "
+                    "YEAR is of invalid datatype: Bool.\nPlease provide an "
+                    "integer value as reference year.");
+  assertThrowsError(neqSprql(yearSprqlExpr(var), Id::makeUndefined()),
+                    "Reference value for filtering date values over expression "
+                    "YEAR is of invalid datatype: Undefined.\nPlease provide "
+                    "an integer value as reference year.");
 }
 
 // Test that the conditions required for a correct merge of child

test/PrefilterExpressionIndexTest.cpp

@@ -765,10 +765,9 @@ TEST_F(PrefilterExpressionOnMetadataTest, testEqualityOperator) {
 // Test PrefilterExpression content formatting for debugging.
 TEST(PrefilterExpressionExpressionOnMetadataTest,
      checkPrintFormattedPrefilterExpression) {
-  auto exprToString = [](const PrefilterExpression& expr) {
+  auto exprToString = [](const auto& expr) {
     return (std::stringstream{} << expr).str();
   };
-
   auto matcher = [&exprToString](const std::string& substring) {
     return ::testing::ResultOf(exprToString, ::testing::Eq(substring));
   };
@@ -827,3 +826,15 @@ TEST(PrefilterExpressionExpressionOnMetadataTest,
           ".\n}child2 {Prefilter RelationalExpression<NE(!=)>\nreferenceValue_ "
           ": <iri/custom/v66> .\n}\n.\n"));
 }
+
+//______________________________________________________________________________
+// Test PrefilterExpression unkown `CompOp comparison` value detection.
+TEST(PrefilterExpressionExpressionOnMetadataTest,
+     checkMakePrefilterVecDetectsAndThrowsForInvalidComparisonOp) {
+  using namespace prefilterExpressions::detail;
+  AD_EXPECT_THROW_WITH_MESSAGE(
+      makePrefilterExpressionYearImpl(static_cast<CompOp>(10), 0),
+      ::testing::HasSubstr(
+          "Set unkown (relational) comparison operator for the creation of "
+          "PrefilterExpression on date-values: Undefined CompOp value: 10."));
+}