Fix comments

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<T1> 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<T1> 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.

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();

velox/functions/sparksql/aggregates/SumAggregate.cpp

@@ -26,16 +26,10 @@ namespace {
 template <typename TInput, typename TAccumulator, typename ResultType>
 using SumAggregate = SumAggregateBase<TInput, TAccumulator, ResultType, true>;
 
-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<exec::SimpleAggregateAdapter<
                     DecimalSumAggregate<int64_t, int64_t>>>(resultType);