Refactor

src/engine/GroupBy.cpp

@@ -812,52 +812,35 @@ bool GroupBy::hasAnyType(const auto& expr) {
 }
 
 // _____________________________________________________________________________
-GroupBy::HashMapAggregateKind GroupBy::getAggregateKind(
+std::optional<GroupBy::HashMapAggregateKind> GroupBy::isSupportedAggregate(
     sparqlExpression::SparqlExpression* expr) {
   using namespace sparqlExpression;
 
-  // TODO: Combine this with isUnsupportedAggregate into one function,
-  //       so that we do not check the type multiple times
-  if (hasType<AvgExpression>(expr)) return HashMapAggregateKind::AVG;
-  if (hasType<CountExpression>(expr)) return HashMapAggregateKind::COUNT;
+  // `expr` is not a distinct aggregate
+  if (expr->isDistinct()) return std::nullopt;
 
-  AD_THROW(
-      "Unsupported aggregate provided in Hash Map optimization of Group By.");
-}
+  // `expr` is not a nested aggregated
+  if (expr->children().front()->containsAggregate()) return std::nullopt;
 
-// _____________________________________________________________________________
-bool GroupBy::isUnsupportedAggregate(sparqlExpression::SparqlExpression* expr) {
-  using namespace sparqlExpression;
-
-  // `expr` is not an aggregate, so it cannot be an unsupported aggregate
-  if (!expr->isAggregate()) return false;
+  if (hasType<AvgExpression>(expr)) return HashMapAggregateKind::AVG;
+  if (hasType<CountExpression>(expr)) return HashMapAggregateKind::COUNT;
 
   // `expr` is an unsupported aggregate
-  if (hasAnyType<SumExpression, MinExpression, MaxExpression,
-                 GroupConcatExpression>(expr))
-    return true;
-
-  // `expr` is a distinct aggregate
-  return expr->isDistinct();
+  return std::nullopt;
 }
 
 // _____________________________________________________________________________
 bool GroupBy::findAggregatesImpl(
     sparqlExpression::SparqlExpression* expr,
     std::optional<ParentAndChildIndex> parentAndChildIndex,
     std::vector<GroupBy::HashMapAggregateInformation>& info) {
-  // Unsupported aggregates
-  if (isUnsupportedAggregate(expr)) return false;
-
   if (expr->isAggregate()) {
-    auto aggregateKind = getAggregateKind(expr);
-
-    info.emplace_back(expr, 0, aggregateKind, parentAndChildIndex);
-
-    // Make sure this is not a nested aggregate.
-    if (expr->children().front()->containsAggregate()) return false;
-
-    return true;
+    if (auto aggregateKind = isSupportedAggregate(expr)) {
+      info.emplace_back(expr, 0, aggregateKind.value(), parentAndChildIndex);
+      return true;
+    } else {
+      return false;
+    }
   }
 
   auto children = expr->children();

src/engine/GroupBy.h

@@ -242,10 +242,6 @@ class GroupBy : public Operation {
     }
   };
 
-  // Gets the aggregate kind of given supported aggregate expression.
-  static HashMapAggregateKind getAggregateKind(
-      sparqlExpression::SparqlExpression* expr);
-
   // Stores alias information, especially all aggregates contained
   // in an alias.
   struct HashMapAliasInformation {
@@ -380,9 +376,8 @@ class GroupBy : public Operation {
   static bool hasAnyType(const auto& expr);
 
   // Check if an expression is a currently supported aggregate.
-  // TODO<kcaliban> As soon as all aggregates are supported, implement and use a
-  //                `isAggregate` function in SparqlExpressions instead.
-  static bool isUnsupportedAggregate(sparqlExpression::SparqlExpression* expr);
+  static std::optional<GroupBy::HashMapAggregateKind> isSupportedAggregate(
+      sparqlExpression::SparqlExpression* expr);
 
   // Find all occurrences of grouped by variable for expression `expr`.
   GroupBy::GroupedByVariableSubstitutions findGroupedVariable(