From e1614c7075428f7dca3704eeaddc3e26ededa3fe Mon Sep 17 00:00:00 2001 From: "joey.ljy" Date: Wed, 28 Feb 2024 10:31:42 +0800 Subject: [PATCH] Fix comments --- velox/docs/develop/aggregate-functions.rst | 16 ++++++++++++---- .../sparksql/aggregates/DecimalSumAggregate.h | 4 ++++ .../sparksql/aggregates/SumAggregate.cpp | 16 +++++----------- 3 files changed, 21 insertions(+), 15 deletions(-) diff --git a/velox/docs/develop/aggregate-functions.rst b/velox/docs/develop/aggregate-functions.rst index 572c7446e16a..b9dd5775c849 100644 --- a/velox/docs/develop/aggregate-functions.rst +++ b/velox/docs/develop/aggregate-functions.rst @@ -254,6 +254,9 @@ For aggregaiton functions of default-null behavior, the author defines an // Optional. Default is false. static constexpr bool use_external_memory_ = true; + // Optional. Default is false. + static constexpr bool aligned_accumulator_ = true; + explicit AccumulatorType(HashStringAllocator* allocator); void addInput(HashStringAllocator* allocator, exec::arg_type value1, ...); @@ -274,7 +277,9 @@ The author defines an optional flag `is_fixed_size_` indicating whether the every accumulator takes fixed amount of memory. This flag is true by default. Next, the author defines another optional flag `use_external_memory_` indicating whether the accumulator uses memory that is not tracked by Velox. -This flag is false by default. +This flag is false by default. Then, the author can define optional flag +`aligned_accumulator_` indicationg whether the accumulator requires aligned +access. This flag is false by default. The author defines a constructor that takes a single argument of `HashStringAllocator*`. This constructor is called before aggregation starts to @@ -345,6 +350,9 @@ For aggregaiton functions of non-default-null behavior, the author defines an // Optional. Default is false. static constexpr bool use_external_memory_ = true; + // Optional. Default is false. + static constexpr bool aligned_accumulator_ = true; + explicit AccumulatorType(HashStringAllocator* allocator); bool addInput(HashStringAllocator* allocator, exec::optional_arg_type value1, ...); @@ -361,9 +369,9 @@ For aggregaiton functions of non-default-null behavior, the author defines an void destroy(HashStringAllocator* allocator); }; -The definition of `is_fixed_size_`, `use_external_memory_`, the constructor, -and the `destroy` method are exactly the same as those for default-null -behavior. +The definition of `is_fixed_size_`, `use_external_memory_`, +`aligned_accumulator_` the constructor, and the `destroy` method are exactly +the same as those for default-null behavior. On the other hand, the C++ function signatures of `addInput`, `combine`, `writeIntermediateResult`, and `writeFinalResult` are different. diff --git a/velox/functions/sparksql/aggregates/DecimalSumAggregate.h b/velox/functions/sparksql/aggregates/DecimalSumAggregate.h index 45d38e5ee47b..638f78753367 100644 --- a/velox/functions/sparksql/aggregates/DecimalSumAggregate.h +++ b/velox/functions/sparksql/aggregates/DecimalSumAggregate.h @@ -121,6 +121,10 @@ class DecimalSumAggregate { // isEmpty is never null. VELOX_CHECK(otherIsEmpty.has_value()); + if (isEmpty && otherIsEmpty.value()) { + // Both accumulators are empty, no need to do the combination. + return false; + } bool currentOverflow = !isEmpty && !sum.has_value(); bool otherOverflow = !otherIsEmpty.value() && !otherSum.has_value(); diff --git a/velox/functions/sparksql/aggregates/SumAggregate.cpp b/velox/functions/sparksql/aggregates/SumAggregate.cpp index 281e5232e20b..78b05a1935ee 100644 --- a/velox/functions/sparksql/aggregates/SumAggregate.cpp +++ b/velox/functions/sparksql/aggregates/SumAggregate.cpp @@ -26,16 +26,10 @@ namespace { template using SumAggregate = SumAggregateBase; -TypePtr getDecimalSumType( - const TypePtr& resultType, - core::AggregationNode::Step step) { - if (exec::isPartialOutput(step)) { - return resultType->childAt(0); - } - if (step == core::AggregationNode::Step::kSingle && resultType->isRow()) { - // For companion function of decimal sum. For a partial companion function - // of single step, the result type is like partial output, consisting of sum - // and isEmpty. +TypePtr getDecimalSumType(const TypePtr& resultType) { + if (resultType->isRow()) { + // If the resultType is ROW, then the type if sum is the type of the first + // child of the ROW. return resultType->childAt(0); } return resultType; @@ -105,7 +99,7 @@ exec::AggregateRegistrationResult registerSum( BIGINT()); case TypeKind::BIGINT: { if (inputType->isShortDecimal()) { - auto const sumType = getDecimalSumType(resultType, step); + auto const sumType = getDecimalSumType(resultType); if (sumType->isShortDecimal()) { return std::make_unique>>(resultType);