From 050839ca37e7882b9e1f1177e12b667c2671f836 Mon Sep 17 00:00:00 2001 From: Pedro Eugenio Rocha Pedreira Date: Thu, 26 Sep 2024 11:06:29 -0700 Subject: [PATCH] Split MinMaxByAggregates.cpp for faster compilation times (#11098) Summary: Pull Request resolved: https://github.com/facebookincubator/velox/pull/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 --- .../prestosql/aggregates/CMakeLists.txt | 3 +- .../prestosql/aggregates/MaxByAggregate.cpp | 53 +++++++++++++++ .../prestosql/aggregates/MinByAggregate.cpp | 53 +++++++++++++++ ...Aggregates.cpp => MinMaxByAggregateBase.h} | 64 +------------------ .../aggregates/RegisterAggregateFunctions.cpp | 9 ++- 5 files changed, 118 insertions(+), 64 deletions(-) create mode 100644 velox/functions/prestosql/aggregates/MaxByAggregate.cpp create mode 100644 velox/functions/prestosql/aggregates/MinByAggregate.cpp rename velox/functions/prestosql/aggregates/{MinMaxByAggregates.cpp => MinMaxByAggregateBase.h} (95%) diff --git a/velox/functions/prestosql/aggregates/CMakeLists.txt b/velox/functions/prestosql/aggregates/CMakeLists.txt index 4e467bab1f66..986de438474e 100644 --- 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 new file mode 100644 index 000000000000..60cc72f008b4 --- /dev/null +++ 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 +class MaxByNAggregate : public MinMaxByNAggregate> { + public: + explicit MaxByNAggregate(TypePtr resultType) + : MinMaxByNAggregate>(resultType) {} +}; + +template +class MaxByNAggregate + : public MinMaxByNAggregate< + ComplexType, + C, + Greater> { + public: + explicit MaxByNAggregate(TypePtr resultType) + : MinMaxByNAggregate< + ComplexType, + C, + Greater>(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 new file mode 100644 index 000000000000..6d47f73867dd --- /dev/null +++ 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 +class MinByNAggregate : public MinMaxByNAggregate> { + public: + explicit MinByNAggregate(TypePtr resultType) + : MinMaxByNAggregate>(resultType) {} +}; + +template +class MinByNAggregate + : public MinMaxByNAggregate< + ComplexType, + C, + Less> { + public: + explicit MinByNAggregate(TypePtr resultType) + : MinMaxByNAggregate< + ComplexType, + C, + Less>(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/velox/functions/prestosql/aggregates/MinMaxByAggregates.cpp b/velox/functions/prestosql/aggregates/MinMaxByAggregateBase.h similarity index 95% rename from velox/functions/prestosql/aggregates/MinMaxByAggregates.cpp rename to velox/functions/prestosql/aggregates/MinMaxByAggregateBase.h index 71f020ca398c..5f6c57690594 100644 --- a/velox/functions/prestosql/aggregates/MinMaxByAggregates.cpp +++ b/velox/functions/prestosql/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 @@ -36,7 +32,7 @@ struct Comparator { const DecodedVector& newComparisons, vector_size_t index, bool isFirstValue) { - if constexpr (isNumeric()) { + if constexpr (functions::aggregate::isNumeric()) { if (isFirstValue) { return true; } @@ -1017,48 +1013,6 @@ class MinMaxByNAggregate : public exec::Aggregate { DecodedVector decodedIntermediates_; }; -template -class MinByNAggregate : public MinMaxByNAggregate> { - public: - explicit MinByNAggregate(TypePtr resultType) - : MinMaxByNAggregate>(resultType) {} -}; - -template -class MinByNAggregate - : public MinMaxByNAggregate< - ComplexType, - C, - Less> { - public: - explicit MinByNAggregate(TypePtr resultType) - : MinMaxByNAggregate< - ComplexType, - C, - Less>(resultType) {} -}; - -template -class MaxByNAggregate : public MinMaxByNAggregate> { - public: - explicit MaxByNAggregate(TypePtr resultType) - : MinMaxByNAggregate>(resultType) {} -}; - -template -class MaxByNAggregate - : public MinMaxByNAggregate< - ComplexType, - C, - Greater> { - public: - explicit MaxByNAggregate(TypePtr resultType) - : MinMaxByNAggregate< - ComplexType, - C, - Greater>(resultType) {} -}; - template