Split MinMaxByAggregates.cpp for faster compilation times (facebookin…

…cubator#11098) Summary: Pull Request resolved: facebookincubator#11098 Splitting the .cpp into two compilation unitst (one for min one for max) so that compilation can be parallelized. In my development environment, recompilations of this unit went from 1m50s to about 40s. Reviewed By: xiaoxmeng Differential Revision: D63433609 fbshipit-source-id: 5402a280dfa3f5e65ffa10c49102579820e1eb4c
xiaoxmeng · Sep 26, 2024 · 050839c · 050839c
1 parent afa43a7
commit 050839c
Show file tree

Hide file tree

Showing 5 changed files with 118 additions and 64 deletions.
diff --git a/velox/functions/prestosql/aggregates/CMakeLists.txt b/velox/functions/prestosql/aggregates/CMakeLists.txt
@@ -36,7 +36,8 @@ velox_add_library(
   MapUnionSumAggregate.cpp
   MaxSizeForStatsAggregate.cpp
   MinMaxAggregates.cpp
-  MinMaxByAggregates.cpp
+  MaxByAggregate.cpp
+  MinByAggregate.cpp
   MultiMapAggAggregate.cpp
   PrestoHasher.cpp
   ReduceAgg.cpp

diff --git a/velox/functions/prestosql/aggregates/MaxByAggregate.cpp b/velox/functions/prestosql/aggregates/MaxByAggregate.cpp
@@ -0,0 +1,53 @@
+/*
+ * Copyright (c) Facebook, Inc. and its affiliates.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include "velox/functions/prestosql/aggregates/AggregateNames.h"
+#include "velox/functions/prestosql/aggregates/MinMaxByAggregateBase.h"
+
+namespace facebook::velox::aggregate::prestosql {
+
+template <typename V, typename C>
+class MaxByNAggregate : public MinMaxByNAggregate<V, C, Greater<V, C>> {
+ public:
+  explicit MaxByNAggregate(TypePtr resultType)
+      : MinMaxByNAggregate<V, C, Greater<V, C>>(resultType) {}
+};
+
+template <typename C>
+class MaxByNAggregate<ComplexType, C>
+    : public MinMaxByNAggregate<
+          ComplexType,
+          C,
+          Greater<HashStringAllocator::Position, C>> {
+ public:
+  explicit MaxByNAggregate(TypePtr resultType)
+      : MinMaxByNAggregate<
+            ComplexType,
+            C,
+            Greater<HashStringAllocator::Position, C>>(resultType) {}
+};
+
+void registerMaxByAggregates(
+    const std::string& prefix,
+    bool withCompanionFunctions,
+    bool overwrite) {
+  registerMinMaxBy<
+      functions::aggregate::MinMaxByAggregateBase,
+      true,
+      MaxByNAggregate>(prefix + kMaxBy, withCompanionFunctions, overwrite);
+}
+
+} // namespace facebook::velox::aggregate::prestosql
diff --git a/velox/functions/prestosql/aggregates/MinByAggregate.cpp b/velox/functions/prestosql/aggregates/MinByAggregate.cpp
@@ -0,0 +1,53 @@
+/*
+ * Copyright (c) Facebook, Inc. and its affiliates.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include "velox/functions/prestosql/aggregates/AggregateNames.h"
+#include "velox/functions/prestosql/aggregates/MinMaxByAggregateBase.h"
+
+namespace facebook::velox::aggregate::prestosql {
+
+template <typename V, typename C>
+class MinByNAggregate : public MinMaxByNAggregate<V, C, Less<V, C>> {
+ public:
+  explicit MinByNAggregate(TypePtr resultType)
+      : MinMaxByNAggregate<V, C, Less<V, C>>(resultType) {}
+};
+
+template <typename C>
+class MinByNAggregate<ComplexType, C>
+    : public MinMaxByNAggregate<
+          ComplexType,
+          C,
+          Less<HashStringAllocator::Position, C>> {
+ public:
+  explicit MinByNAggregate(TypePtr resultType)
+      : MinMaxByNAggregate<
+            ComplexType,
+            C,
+            Less<HashStringAllocator::Position, C>>(resultType) {}
+};
+
+void registerMinByAggregates(
+    const std::string& prefix,
+    bool withCompanionFunctions,
+    bool overwrite) {
+  registerMinMaxBy<
+      functions::aggregate::MinMaxByAggregateBase,
+      false,
+      MinByNAggregate>(prefix + kMinBy, withCompanionFunctions, overwrite);
+}
+
+} // namespace facebook::velox::aggregate::prestosql
diff --git a/...estosql/aggregates/MinMaxByAggregates.cpp → ...stosql/aggregates/MinMaxByAggregateBase.h b/...estosql/aggregates/MinMaxByAggregates.cpp → ...stosql/aggregates/MinMaxByAggregateBase.h
@@ -21,12 +21,8 @@
 #include "velox/functions/prestosql/aggregates/AggregateNames.h"
 #include "velox/type/FloatingPointUtil.h"
 
-using namespace facebook::velox::functions::aggregate;
-
 namespace facebook::velox::aggregate::prestosql {
 
-namespace {
-
 /// Returns true if the value in 'index' row of 'newComparisons' is strictly
 /// greater than or less than the value in the 'accumulator'.
 template <bool greaterThan, typename T, typename TAccumulator>
@@ -36,7 +32,7 @@ struct Comparator {
       const DecodedVector& newComparisons,
       vector_size_t index,
       bool isFirstValue) {
-    if constexpr (isNumeric<T>()) {
+    if constexpr (functions::aggregate::isNumeric<T>()) {
       if (isFirstValue) {
         return true;
       }
@@ -1017,48 +1013,6 @@ class MinMaxByNAggregate : public exec::Aggregate {
   DecodedVector decodedIntermediates_;
 };
 
-template <typename V, typename C>
-class MinByNAggregate : public MinMaxByNAggregate<V, C, Less<V, C>> {
- public:
-  explicit MinByNAggregate(TypePtr resultType)
-      : MinMaxByNAggregate<V, C, Less<V, C>>(resultType) {}
-};
-
-template <typename C>
-class MinByNAggregate<ComplexType, C>
-    : public MinMaxByNAggregate<
-          ComplexType,
-          C,
-          Less<HashStringAllocator::Position, C>> {
- public:
-  explicit MinByNAggregate(TypePtr resultType)
-      : MinMaxByNAggregate<
-            ComplexType,
-            C,
-            Less<HashStringAllocator::Position, C>>(resultType) {}
-};
-
-template <typename V, typename C>
-class MaxByNAggregate : public MinMaxByNAggregate<V, C, Greater<V, C>> {
- public:
-  explicit MaxByNAggregate(TypePtr resultType)
-      : MinMaxByNAggregate<V, C, Greater<V, C>>(resultType) {}
-};
-
-template <typename C>
-class MaxByNAggregate<ComplexType, C>
-    : public MinMaxByNAggregate<
-          ComplexType,
-          C,
-          Greater<HashStringAllocator::Position, C>> {
- public:
-  explicit MaxByNAggregate(TypePtr resultType)
-      : MinMaxByNAggregate<
-            ComplexType,
-            C,
-            Greater<HashStringAllocator::Position, C>>(resultType) {}
-};
-
 template <template <typename U, typename V> class NAggregate, typename W>
 std::unique_ptr<exec::Aggregate> createNArg(
     TypePtr resultType,
@@ -1142,7 +1096,7 @@ std::unique_ptr<exec::Aggregate> createNArg(
   }
 }
 
-std::string toString(const std::vector<TypePtr>& types) {
+inline std::string toString(const std::vector<TypePtr>& types) {
   std::ostringstream out;
   for (auto i = 0; i < types.size(); ++i) {
     if (i > 0) {
@@ -1239,24 +1193,12 @@ exec::AggregateRegistrationResult registerMinMaxBy(
           return createNArg<NAggregate>(
               resultType, argTypes[0], argTypes[1], errorMessage);
         } else {
-          return create<Aggregate, Comparator, isMaxFunc>(
+          return functions::aggregate::create<Aggregate, Comparator, isMaxFunc>(
               resultType, argTypes[0], argTypes[1], errorMessage, true);
         }
       },
       withCompanionFunctions,
       overwrite);
 }
 
-} // namespace
-
-void registerMinMaxByAggregates(
-    const std::string& prefix,
-    bool withCompanionFunctions,
-    bool overwrite) {
-  registerMinMaxBy<MinMaxByAggregateBase, true, MaxByNAggregate>(
-      prefix + kMaxBy, withCompanionFunctions, overwrite);
-  registerMinMaxBy<MinMaxByAggregateBase, false, MinByNAggregate>(
-      prefix + kMinBy, withCompanionFunctions, overwrite);
-}
-
 } // namespace facebook::velox::aggregate::prestosql
diff --git a/velox/functions/prestosql/aggregates/RegisterAggregateFunctions.cpp b/velox/functions/prestosql/aggregates/RegisterAggregateFunctions.cpp
@@ -129,7 +129,11 @@ extern void registerMinMaxAggregates(
     const std::string& prefix,
     bool withCompanionFunctions,
     bool overwrite);
-extern void registerMinMaxByAggregates(
+extern void registerMaxByAggregates(
+    const std::string& prefix,
+    bool withCompanionFunctions,
+    bool overwrite);
+extern void registerMinByAggregates(
     const std::string& prefix,
     bool withCompanionFunctions,
     bool overwrite);
@@ -176,7 +180,8 @@ void registerAllAggregateFunctions(
   registerSumDataSizeForStatsAggregate(
       prefix, withCompanionFunctions, overwrite);
   registerMinMaxAggregates(prefix, withCompanionFunctions, overwrite);
-  registerMinMaxByAggregates(prefix, withCompanionFunctions, overwrite);
+  registerMaxByAggregates(prefix, withCompanionFunctions, overwrite);
+  registerMinByAggregates(prefix, withCompanionFunctions, overwrite);
   registerReduceAgg(prefix, withCompanionFunctions, overwrite);
   registerSetAggAggregate(prefix, withCompanionFunctions, overwrite);
   registerSetUnionAggregate(prefix, withCompanionFunctions, overwrite);