Add support for Decimal Type Sum Aggregation #2163

majetideepak · 2022-08-01T14:31:24Z

Improve Decimal Type Function signature to re-use variables.

velox/exec/GroupingSet.cpp

mbasmanova

@majetideepak Overall looks good. Some comments below.

mbasmanova · 2022-08-02T02:45:49Z

velox/type/ShortDecimal.h

 struct ShortDecimal {
 public:
  // Default required for creating vector with NULL values.
  ShortDecimal() = default;
-  constexpr explicit ShortDecimal(int64_t value) : unscaledValue_(value) {}
+  constexpr ShortDecimal(int64_t value) : unscaledValue_(value) {}


I think you still need 'explicit'

My idea was to allow all integer types to implicitly cast to ShortDecimal and LongDecimal.
I can add explicit constructors if that is a best practice.

Why would we want to implicitly cast any integer to a decimal? XxxDecimal classes are a bit weird as they hold unscaled values which cannot be properly interpreted without extract information about scale. Implicitly casing an integer to XxxDecimal produces a value that is not equal to the original integer unless scale of 0 is assumed. I wonder if renaming these classes to UnscaledXxxDecimal would make the code a bit clearer.

My interpretation is that in the input to XxxDecimal are always unscaled values as there is no precision, scale info like you said. But I see your view as well. I guess renaming to UnscaledXxxDecimal clarifies this and we can allow implicit integer cast to UnscaledXxxDecimal.
I will make this change in a separate PR.

CC: @karteekmurthys

mbasmanova · 2022-08-02T02:46:17Z

velox/type/ShortDecimal.h

 struct ShortDecimal {
 public:
  // Default required for creating vector with NULL values.
  ShortDecimal() = default;
-  constexpr explicit ShortDecimal(int64_t value) : unscaledValue_(value) {}
+  constexpr ShortDecimal(int64_t value) : unscaledValue_(value) {}


nit: Add empty lines between methods.

mbasmanova · 2022-08-02T02:46:41Z

velox/type/ShortDecimal.h

 struct ShortDecimal {
 public:
  // Default required for creating vector with NULL values.
  ShortDecimal() = default;
-  constexpr explicit ShortDecimal(int64_t value) : unscaledValue_(value) {}
+  constexpr ShortDecimal(int64_t value) : unscaledValue_(value) {}
+  ShortDecimal(LongDecimal& value);


What is this constructor for? Is it valid to create a short decimal value from a long decimal value?

This is required for the sum aggregate template setup. The initialization 0 is a LongDecimal which gets passed as a ShortDecimal value. I added a check to ensure validity.

We can specialize there if needed. But I thought this won't hurt as long as there is a validity check.

Is it this code?

*exec::Aggregate::value<ResultType>(groups[i]) = 0;

Can it be changed to = ResultType(0)?

The issue is with the template parameter types

velox/velox/functions/prestosql/aggregates/SimpleNumericAggregate.h

Lines 130 to 151 in 717c5a9

template <

typename TData = TResult,

typename UpdateSingle,

typename UpdateDuplicate>

void updateOneGroup(

char* group,

const SelectivityVector& rows,

const VectorPtr& arg,

UpdateSingle updateSingleValue,

UpdateDuplicate updateDuplicateValues,

bool /*mayPushdown*/,

TData initialValue) {

DecodedVector decoded(*arg, rows);

// Do row by row if not all rows are selected.

if (decoded.isConstantMapping()) {

if (!decoded.isNullAt(0)) {

updateDuplicateValues(

initialValue, decoded.valueAt<TInput>(0), rows.countSelected());

updateNonNullValue<true, TData>(group, initialValue, updateSingleValue);

}

} else if (decoded.mayHaveNulls()) {

TData initialValue is LongDecimal type which gets passed to updateNonNullValue(char* group, TInput value, Update updateValue) where TInput is ShortDecimal type.

mbasmanova · 2022-08-02T02:47:01Z

velox/type/ShortDecimal.h

@@ -57,9 +60,21 @@ struct ShortDecimal {
    return unscaledValue_ >= other.unscaledValue_;
  }

+  ShortDecimal& operator+=(const ShortDecimal& other) {
+    unscaledValue_ += other.unscaledValue_;


Do we need to check for overflow?

Probably not. I think the operator type inference will ensure values fit. But doesn't hurt to add. I will add.

I think the operator type inference will ensure values fit.

Would you explain how this works?

We do not check for overflow for integer values right? So ShortDecimal is similar. We just treat them as any regular integers.
By operator type inference, I mean at the high level, for example, sum aggregate ensures the intermediate type is LongDecimal and the values will fit based on the precision, and scale.

We didn't check for overflow in SUM, but turns out we should. Presto implementation checks, hence, we need to match. #2010 adds these checks.

mbasmanova · 2022-08-02T02:47:46Z

velox/type/ShortDecimal.h

 private:
  int64_t unscaledValue_;
 };
+
+static inline ShortDecimal operator*(


Why is this operator defined differently from the + operator?

Apparently, only unary operators can be defined inside the class. Binary operators must be defined outside.

Thank you for clarifying. Would you add a comment explaining why we need to define this operator? It is not obvious because the operator is defined only for 32-bit integers.

This requirement is coming from

velox/velox/functions/prestosql/aggregates/SumAggregate.h

Line 87 in cb32115

[](TAccumulator& result, TInput value, int n) { result += n * value; },

mbasmanova · 2022-08-02T02:51:09Z

velox/functions/prestosql/aggregates/tests/SumTest.cpp

+           DECIMAL(23, 4))});
+
+  auto expected = makeRowVector(
+      {makeLongDecimalFlatVector({15000}, DECIMAL(38, 1)),


nit: 15'000 for readability

mbasmanova · 2022-08-02T02:51:39Z

velox/functions/prestosql/aggregates/tests/SumTest.cpp

+      {makeLongDecimalFlatVector({15000}, DECIMAL(38, 1)),
+       makeLongDecimalFlatVector({buildInt128(50, 1500)}, DECIMAL(38, 4))});
+
+  // Global final aggregation.


Let's use testAggregations to have better test coverage.

mbasmanova · 2022-08-02T02:53:23Z

velox/functions/prestosql/aggregates/SumAggregate.h

@@ -166,6 +166,13 @@ bool registerSumAggregate(const std::string& name) {
          .intermediateType("double")
          .argumentType("double")
          .build(),
+      exec::AggregateFunctionSignatureBuilder()
+          .argumentType("DECIMAL(a_precision, a_scale)")
+          .intermediateType("DECIMAL(38, i_scale)")


Looks like we can combine i_scale and r_scale into a single variable and use 'r_scale' in both places. This way, it will be clearer that intermediate and return types are the same.

mbasmanova · 2022-08-02T02:55:04Z

velox/exec/GroupingSet.cpp

  //  - fixed-width accumulators - one per aggregate
  //
  // Here we always make space for a row size since we only have one
  // row and no RowContainer.
  int32_t rowSizeOffset = bits::nbytes(aggregates_.size());
-  int32_t offset = rowSizeOffset + sizeof(int32_t);
+  // Fixed-width accumulators must be word aligned.


Where does this requirement come from and why do we round to 16 and not 8?

Without this, the LongDecimal accumulator is segfaulting as the memory address is not aligned. Hence, rounding off to 2 words, the size of LongDecimal.

The requirement comes from the implementation of

void extractAccumulators(char** groups, int32_t numGroups, VectorPtr* result) override { BaseAggregate::template doExtractValues<TAccumulator>( groups, numGroups, result, [&](char* group) { return *BaseAggregate::Aggregate::template value<TAccumulator>(group); }); }

where

T* value(char* group) const { return reinterpret_cast<T*>(group + offset_); }

Thank you for explaining. It appears to me that this code / requirement is highly-specific to accumulators of type LongDecimal which require 16-byte alignment. At the same time the comment sounds generic and suggests that any fixed-width accumulator must be work aligned. Here, we have only one type of accumulators requiring two word alignment. I wonder if there is a way to generalize this code. If not, let's clarify in the comment that we require 16-byte (2-word) alignment to support accumulator types that use __uint128_t.

I can make this comment. But in general, I thought unaligned access is not portable across architectures. So it should apply to all types.
We can tighten this if you would like by checking all the types in the vector of accumulators and aligning by the largest type.

in general, I thought unaligned access is not portable across architectures

This is so, but the comment says "word aligned", while the code makes it "two-word aligned".

We can tighten this ... by checking all the types in the vector of accumulators and aligning by the largest type.

This might be more robust and less specific to int128_t used by LongDecimal accumulator.

mbasmanova · 2022-08-02T02:55:42Z

velox/exec/tests/utils/QueryAssertions.cpp

+    } else if (child->isShortDecimal() || child->isLongDecimal()) {
+      int precision;
+      int scale;
+      getDecimalPrecisionScale(*child, precision, scale);


nit: Would be nice to change the signature of this function to return a pair of precision and scale.

auto [precision, scale] = getDecimalPrecisionScale(*child);

The decimal add PR has this change. I will rebase after that merge or fix in that PR.

majetideepak · 2022-08-05T11:10:28Z

velox/common/base/CheckedArithmetic.h

@@ -33,6 +34,30 @@ T checkedPlus(const T& a, const T& b) {
  return result;
 }

+template <>


I will move these to a separate PR and add tests.

velox/functions/prestosql/aggregates/tests/SumTest.cpp

facebook-github-bot · 2022-08-18T18:56:31Z

@xiaoxmeng has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

netlify · 2022-08-22T00:53:58Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`2d95838`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/630d18f83b59b0000a150860

xiaoxmeng

Looks great. Thanks for effort and offline discussions. Please also have @mbasmanova to take a look before landing.

velox/common/base/CheckedArithmetic.h

velox/vector/tests/VectorMaker-inl.h

xiaoxmeng · 2022-08-22T05:29:41Z

velox/vector/FlatVector.h

@@ -107,7 +107,11 @@ class FlatVector final : public SimpleVector<T> {
        rawValues_(values_.get() ? const_cast<T*>(values_->as<T>()) : nullptr) {
    setStringBuffers(std::move(stringBuffers));
    VELOX_DCHECK_GE(stringBuffers_.size(), stringBufferSet_.size());
-
+    if constexpr (std::is_same<T, UnscaledShortDecimal>::value) {
+      VELOX_CHECK(type->isShortDecimal());


Consider to use DCHECK here?

VELOX_DCHECK_EQ(std::is_same_v<xxx>, type->isShortDecimal()); VELOX_DCHECK_EQ(std::is_same_v<yyy>, type->isLongDecimal());

Also please use std::is_same_v as to be consistent with other part of code (there are some ongoing efforts to refactoring this)

velox/vector/tests/VectorTestBase.h

xiaoxmeng · 2022-08-22T05:35:43Z

velox/functions/prestosql/aggregates/SumAggregate.h

+}
+
+/// Override 'accumulatorAlignmentSize' for UnscaledLongDecimal values as it
+/// uses int128_t type.


uses int128_t type. Some CPU doesn't support misaligned access to int128_t type.

velox/functions/prestosql/aggregates/SumAggregate.h

velox/exec/Aggregate.h

facebook-github-bot · 2022-08-22T05:47:11Z

@xiaoxmeng has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

mbasmanova

@majetideepak Looks good to % a couple of small questions.

mbasmanova · 2022-08-23T02:55:34Z

velox/exec/tests/utils/QueryAssertions.cpp

@@ -44,6 +44,9 @@ std::string makeCreateTableSql(
    } else if (child->isMap()) {
      sql << "MAP(" << child->asMap().keyType()->kindName() << ", "
          << child->asMap().valueType()->kindName() << ")";
+    } else if (child->isShortDecimal() || child->isLongDecimal()) {
+      const auto& [precision, scale] = getDecimalPrecisionScale(*child);
+      sql << "decimal(" << precision << ", " << scale << ")";


nit: DECIMAL for consistency

mbasmanova · 2022-08-23T02:56:42Z

velox/exec/tests/utils/QueryAssertions.cpp

+  uint8_t precision;
+  uint8_t scale;
+  value.type().GetDecimalProperties(precision, scale);
+  auto duckType = DECIMAL(precision, scale);


duckType -> veloxType or just 'type'

mbasmanova · 2022-08-23T02:59:40Z

velox/functions/prestosql/aggregates/SumAggregate.h

@@ -196,6 +243,12 @@ bool registerSumAggregate(const std::string& name) {
          .intermediateType("double")
          .argumentType("double")
          .build(),
+      exec::AggregateFunctionSignatureBuilder()
+          .argumentType("DECIMAL(a_precision, a_scale)")
+          .intermediateType("DECIMAL(38, r_scale)")


Can we use "a_scale" here?

velox/type/UnscaledShortDecimal.h

majetideepak · 2022-08-23T15:37:51Z

@mbasmanova I addressed your review comments. Thanks.

mbasmanova · 2022-08-23T16:15:28Z

@majetideepak Thank you, Deepak.

@xiaoxmeng Meng, would you help land this PR?

facebook-github-bot · 2022-08-23T17:51:39Z

@xiaoxmeng has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

xiaoxmeng · 2022-08-23T22:22:18Z

@majetideepak Thank you, Deepak.

@xiaoxmeng Meng, would you help land this PR?

Yes, I am working on it. Thanks!

xiaoxmeng · 2022-08-24T16:20:52Z

velox/common/base/CheckedArithmetic.h

@@ -33,6 +35,38 @@ T checkedPlus(const T& a, const T& b) {
  return result;
 }

+template <>
+inline UnscaledShortDecimal checkedPlus(


Some template is missing:

Command failed with exit code 1. stderr: In file included from velox/functions/prestosql/aggregates/SumAggregate.cpp:16: In file included from velox/functions/prestosql/aggregates/SumAggregate.h:20: In file included from velox/functions/prestosql/aggregates/SimpleNumericAggregate.h:19: velox/exec/AggregationHook.h:106:14: error: no matching function for call to 'checkedPlus' result = checkedPlus<TOutput>(result, value); ^~~~~~~~~~~~~~~~~~~~ velox/exec/AggregationHook.h:144:5: note: in instantiation of function template specialization 'facebook::velox::aggregate::(anonymous namespace)::updateSingleValue<facebook::velox::UnscaledLongDecimal, facebook::velox::UnscaledShortDecimal>' requested here updateSingleValue( ^ velox/exec/AggregationHook.h:112:7: note: in instantiation of member function 'facebook::velox::aggregate::SumHook<facebook::velox::UnscaledShortDecimal, facebook::velox::UnscaledLongDecimal>::addValue' requested here class SumHook final : public AggregationHook { ^ velox/functions/prestosql/aggregates/SumAggregate.h:124:31: note: in instantiation of function template specialization 'facebook::velox::aggregate::SimpleNumericAggregate<facebook::velox::UnscaledShortDecimal, facebook::velox::UnscaledLongDecimal, facebook::velox::UnscaledLongDecimal>::pushdown<facebook::velox::aggregate::SumHook<facebook::velox::UnscaledShortDecimal, facebook::velox::UnscaledLongDecimal>>' requested here BaseAggregate::template pushdown<SumHook<TInput, TData>>( ^ velox/functions/prestosql/aggregates/SumAggregate.h:71:5: note: in instantiation of function template specialization 'facebook::velox::aggregate::SumAggregate<facebook::velox::UnscaledShortDecimal, facebook::velox::UnscaledLongDecimal, facebook::velox::UnscaledLongDecimal>::updateInternal<facebook::velox::UnscaledLongDecimal>' requested here updateInternal<TAccumulator>(groups, rows, args, mayPushdown); ^ velox/functions/prestosql/aggregates/SumAggregate.h:31:12: note: in instantiation of member function 'facebook::velox::aggregate::SumAggregate<facebook::velox::UnscaledShortDecimal, facebook::velox::UnscaledLongDecimal, facebook::velox::UnscaledLongDecimal>::addRawInput' requested here explicit SumAggregate(TypePtr resultType) : BaseAggregate(resultType) {} ^ third-party-buck/platform010/build/libgcc/include/c++/trunk/bits/unique_ptr.h:962:34: note: in instantiation of member function 'facebook::velox::aggregate::SumAggregate<facebook::velox::UnscaledShortDecimal, facebook::velox::UnscaledLongDecimal, facebook::velox::UnscaledLongDecimal>::SumAggregate' requested here { return unique_ptr<_Tp>(new _Tp(std::forward<_Args>(__args)...)); } ^ velox/functions/prestosql/aggregates/SumAggregate.h:290:25: note: in instantiation of function template specialization 'std::make_unique<facebook::velox::aggregate::SumAggregate<facebook::velox::UnscaledShortDecimal, facebook::velox::UnscaledLongDecimal, facebook::velox::UnscaledLongDecimal>, const std::shared_ptr<const facebook::velox::Type> &>' requested here return std::make_unique< ^ velox/functions/prestosql/aggregates/SumAggregate.cpp:22:5: note: in instantiation of function template specialization 'facebook::velox::aggregate::registerSumAggregate<SumAggregate>' requested here registerSumAggregate<SumAggregate>(kSum); ^ velox/common/base/CheckedArithmetic.h:29:3: note: candidate function template not viable: no known conversion from 'facebook::velox::UnscaledShortDecimal' to 'const facebook::velox::UnscaledLongDecimal' for 2nd argument T checkedPlus(const T& a, const T& b) { ^ In file included from velox/functions/prestosql/aggregates/SumAggregate.cpp:16: In file included from velox/functions/prestosql/aggregates/SumAggregate.h:20: velox/functions/prestosql/aggregates/SimpleNumericAggregate.h:102:11: error: no matching member function for call to 'updateNonNullValue' updateNonNullValue<tableHasNulls, TData>( ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ velox/functions/prestosql/aggregates/SumAggregate.h:130:31: note: in instantiation of function template specialization 'facebook::velox::aggregate::SimpleNumericAggregate<facebook::velox::UnscaledShortDecimal, facebook::velox::UnscaledLongDecimal, facebook::velox::UnscaledLongDecimal>::updateGroups<true, facebook::velox::UnscaledLongDecimal, void (*)(facebook::velox::UnscaledLongDecimal &, facebook::velox::UnscaledLongDecimal)>' requested here BaseAggregate::template updateGroups<true, TData>( ^ velox/functions/prestosql/aggregates/SumAggregate.h:71:5: note: in instantiation of function template specialization 'facebook::velox::aggregate::SumAggregate<facebook::velox::UnscaledShortDecimal, facebook::velox::UnscaledLongDecimal, facebook::velox::UnscaledLongDecimal>::updateInternal<facebook::velox::UnscaledLongDecimal>' requested here updateInternal<TAccumulator>(groups, rows, args, mayPushdown); ^ velox/functions/prestosql/aggregates/SumAggregate.h:31:12: note: in instantiation of member function 'facebook::velox::aggregate::SumAggregate<facebook::velox::UnscaledShortDecimal, facebook::velox::UnscaledLongDecimal, facebook::velox::UnscaledLongDecimal>::addRawInput' requested here explicit SumAggregate(TypePtr resultType) : BaseAggregate(resultType) {} ^ third-party-buck/platform010/build/libgcc/include/c++/trunk/bits/unique_ptr.h:962:34: note: in instantiation of member function 'facebook::velox::aggregate::SumAggregate<facebook::velox::UnscaledShortDecimal, facebook::velox::UnscaledLongDecimal, facebook::velox::UnscaledLongDecimal>::SumAggregate' requested here { return unique_ptr<_Tp>(new _Tp(std::forward<_Args>(__args)...)); } ^ velox/functions/prestosql/aggregates/SumAggregate.h:290:25: note: in instantiation of function template specialization 'std::make_unique<facebook::velox::aggregate::SumAggregate<facebook::velox::UnscaledShortDecimal, facebook::velox::UnscaledLongDecimal, facebook::velox::UnscaledLongDecimal>, const std::shared_ptr<const facebook::velox::Type> &>' requested here return std::make_unique< ^ velox/functions/prestosql/aggregates/SumAggregate.cpp:22:5: note: in instantiation of function template specialization 'facebook::velox::aggregate::registerSumAggregate<SumAggregate>' requested here registerSumAggregate<SumAggregate>(kSum); ^ velox/functions/prestosql/aggregates/SimpleNumericAggregate.h:213:3: note: candidate function template not viable: no known conversion from 'facebook::velox::UnscaledShortDecimal' to 'facebook::velox::UnscaledLongDecimal' for 2nd argument updateNonNullValue(char* group, TDataType value, Update updateValue) { ^ velox/functions/prestosql/aggregates/SimpleNumericAggregate.h:111:9: error: no matching member function for call to 'updateNonNullValue' updateNonNullValue<tableHasNulls, TData>( ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ velox/functions/prestosql/aggregates/SimpleNumericAggregate.h:213:3: note: candidate function template not viable: no known conversion from 'facebook::velox::UnscaledShortDecimal' to 'facebook::velox::UnscaledLongDecimal' for 2nd argument updateNonNullValue(char* group, TDataType value, Update updateValue) { ^ velox/functions/prestosql/aggregates/SimpleNumericAggregate.h:117:9: error: no matching member function for call to 'updateNonNullValue' updateNonNullValue<tableHasNulls, TData>( ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ velox/functions/prestosql/aggregates/SimpleNumericAggregate.h:213:3: note: candidate function template not viable: no known conversion from 'const facebook::velox::UnscaledShortDecimal' to 'facebook::velox::UnscaledLongDecimal' for 2nd argument updateNonNullValue(char* group, TDataType value, Update updateValue) { ^ velox/functions/prestosql/aggregates/SimpleNumericAggregate.h:122:9: error: no matching member function for call to 'updateNonNullValue' updateNonNullValue<tableHasNulls, TData>( ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ velox/functions/prestosql/aggregates/SimpleNumericAggregate.h:213:3: note: candidate function template not viable: no known conversion from 'facebook::velox::UnscaledShortDecimal' to 'facebook::velox::UnscaledLongDecimal' for 2nd argument updateNonNullValue(char* group, TDataType value, Update updateValue) { ^ In file included from velox/functions/prestosql/aggregates/SumAggregate.cpp:16: In file included from velox/functions/prestosql/aggregates/SumAggregate.h:20: In file included from velox/functions/prestosql/aggregates/SimpleNumericAggregate.h:19: velox/exec/AggregationHook.h:172:9: error: no viable conversion from 'const facebook::velox::UnscaledShortDecimal' to 'facebook::velox::UnscaledLongDecimal' *reinterpret_cast<const TValue*>(value)); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ velox/exec/AggregationHook.h:151:7: note: in instantiation of member function 'facebook::velox::aggregate::SimpleCallableHook<facebook::velox::UnscaledShortDecimal, facebook::velox::UnscaledLongDecimal, void (*)(facebook::velox::UnscaledLongDecimal &, facebook::velox::UnscaledLongDecimal)>::addValue' requested here class SimpleCallableHook final : public AggregationHook { ^ velox/functions/prestosql/aggregates/SumAggregate.h:130:31: note: in instantiation of function template specialization 'facebook::velox::aggregate::SimpleNumericAggregate<facebook::velox::UnscaledShortDecimal, facebook::velox::UnscaledLongDecimal, facebook::velox::UnscaledLongDecimal>::updateGroups<true, facebook::velox::UnscaledLongDecimal, void (*)(facebook::velox::UnscaledLongDecimal &, facebook::velox::UnscaledLongDecimal)>' requested here BaseAggregate::template updateGroups<true, TData>( ^ velox/functions/prestosql/aggregates/SumAggregate.h:71:5: note: in instantiation of function template specialization 'facebook::velox::aggregate::SumAggregate<facebook::velox::UnscaledShortDecimal, facebook::velox::UnscaledLongDecimal, facebook::velox::UnscaledLongDecimal>::updateInternal<facebook::velox::UnscaledLongDecimal>' requested here updateInternal<TAccumulator>(groups, rows, args, mayPushdown); ^ velox/functions/prestosql/aggregates/SumAggregate.h:31:12: note: in instantiation of member function 'facebook::velox::aggregate::SumAggregate<facebook::velox::UnscaledShortDecimal, facebook::velox::UnscaledLongDecimal, facebook::velox::UnscaledLongDecimal>::addRawInput' requested here explicit SumAggregate(TypePtr resultType) : BaseAggregate(resultType) {} ^ third-party-buck/platform010/build/libgcc/include/c++/trunk/bits/unique_ptr.h:962:34: note: in instantiation of member function 'facebook::velox::aggregate::SumAggregate<facebook::velox::UnscaledShortDecimal, facebook::velox::UnscaledLongDecimal, facebook::velox::UnscaledLongDecimal>::SumAggregate' requested here { return unique_ptr<_Tp>(new _Tp(std::forward<_Args>(__args)...)); } ^ velox/functions/prestosql/aggregates/SumAggregate.h:290:25: note: in instantiation of function template specialization 'std::make_unique<facebook::velox::aggregate::SumAggregate<facebook::velox::UnscaledShortDecimal, facebook::velox::UnscaledLongDecimal, facebook::velox::UnscaledLongDecimal>, const std::shared_ptr<const facebook::velox::Type> &>' requested here return std::make_unique< ^ velox/functions/prestosql/aggregates/SumAggregate.cpp:22:5: note: in instantiation of function template specialization 'facebook::velox::aggregate::registerSumAggregate<SumAggregate>' requested here registerSumAggregate<SumAggregate>(kSum); ^ velox/type/UnscaledLongDecimal.h:35:8: note: candidate constructor (the implicit copy constructor) not viable: no known conversion from 'const facebook::velox::UnscaledShortDecimal' to 'const facebook::velox::UnscaledLongDecimal &' for 1st argument struct UnscaledLongDecimal { ^ velox/type/UnscaledLongDecimal.h:35:8: note: candidate constructor (the implicit move constructor) not viable: no known conversion from 'const facebook::velox::UnscaledShortDecimal' to 'facebook::velox::UnscaledLongDecimal &&' for 1st argument velox/type/UnscaledLongDecimal.h:40:22: note: explicit constructor is not a candidate constexpr explicit UnscaledLongDecimal(int128_t value) ^ velox/type/UnscaledLongDecimal.h:43:22: note: explicit constructor is not a candidate constexpr explicit UnscaledLongDecimal(int64_t value) ^ note: remaining 2 candidates omitted; pass -fshow-overloads=all to show them In file included from velox/functions/prestosql/aggregates/SumAggregate.cpp:16: In file included from velox/functions/prestosql/aggregates/SumAggregate.h:20: velox/functions/prestosql/aggregates/SimpleNumericAggregate.h:102:11: error: no matching member function for call to 'updateNonNullValue' updateNonNullValue<tableHasNulls, TData>( ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ velox/functions/prestosql/aggregates/SumAggregate.h:133:31: note: in instantiation of function template specialization 'facebook::velox::aggregate::SimpleNumericAggregate<facebook::velox::UnscaledShortDecimal, facebook::velox::UnscaledLongDecimal, facebook::velox::UnscaledLongDecimal>::updateGroups<false, facebook::velox::UnscaledLongDecimal, void (*)(facebook::velox::UnscaledLongDecimal &, facebook::velox::UnscaledLongDecimal)>' requested here BaseAggregate::template updateGroups<false, TData>( ^ velox/functions/prestosql/aggregates/SumAggregate.h:71:5: note: in instantiation of function template specialization 'facebook::velox::aggregate::SumAggregate<facebook::velox::UnscaledShortDecimal, facebook::velox::UnscaledLongDecimal, facebook::velox::UnscaledLongDecimal>::updateInternal<facebook::velox::UnscaledLongDecimal>' requested here updateInternal<TAccumulator>(groups, rows, args, mayPushdown); ^ velox/functions/prestosql/aggregates/SumAggregate.h:31:12: note: in instantiation of member function 'facebook::velox::aggregate::SumAggregate<facebook::velox::UnscaledShortDecimal, facebook::velox::UnscaledLongDecimal, facebook::velox::UnscaledLongDecimal>::addRawInput' requested here explicit SumAggregate(TypePtr resultType) : BaseAggregate(resultType) {} ^ third-party-buck/platform010/build/libgcc/include/c++/trunk/bits/unique_ptr.h:962:34: note: in instantiation of member function 'facebook::velox::aggregate::SumAggregate<facebook::velox::UnscaledShortDecimal, facebook::velox::UnscaledLongDecimal, facebook::velox::UnscaledLongDecimal>::SumAggregate' requested here { return unique_ptr<_Tp>(new _Tp(std::forward<_Args>(__args)...)); } ^ velox/functions/prestosql/aggregates/SumAggregate.h:290:25: note: in instantiation of function template specialization 'std::make_unique<facebook::velox::aggregate::SumAggregate<facebook::velox::UnscaledShortDecimal, facebook::velox::UnscaledLongDecimal, facebook::velox::UnscaledLongDecimal>, const std::shared_ptr<const facebook::velox::Type> &>' requested here return std::make_unique< ^ velox/functions/prestosql/aggregates/SumAggregate.cpp:22:5: note: in instantiation of function template specialization 'facebook::velox::aggregate::registerSumAggregate<SumAggregate>' requested here registerSumAggregate<SumAggregate>(kSum); ^ velox/functions/prestosql/aggregates/SimpleNumericAggregate.h:213:3: note: candidate function template not viable: no known conversion from 'facebook::velox::UnscaledShortDecimal' to 'facebook::velox::UnscaledLongDecimal' for 2nd argument updateNonNullValue(char* group, TDataType value, Update updateValue) { ^ velox/functions/prestosql/aggregates/SimpleNumericAggregate.h:111:9: error: no matching member function for call to 'updateNonNullValue' updateNonNullValue<tableHasNulls, TData>( ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ velox/functions/prestosql/aggregates/SimpleNumericAggregate.h:213:3: note: candidate function template not viable: no known conversion from 'facebook::velox::UnscaledShortDecimal' to 'facebook::velox::UnscaledLongDecimal' for 2nd argument updateNonNullValue(char* group, TDataType value, Update updateValue) { ^ velox/functions/prestosql/aggregates/SimpleNumericAggregate.h:117:9: error: no matching member function for call to 'updateNonNullValue' updateNonNullValue<tableHasNulls, TData>( ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ velox/functions/prestosql/aggregates/SimpleNumericAggregate.h:213:3: note: candidate function template not viable: no known conversion from 'const facebook::velox::UnscaledShortDecimal' to 'facebook::velox::UnscaledLongDecimal' for 2nd argument updateNonNullValue(char* group, TDataType value, Update updateValue) { ^ velox/functions/prestosql/aggregates/SimpleNumericAggregate.h:122:9: error: no matching member function for call to 'updateNonNullValue' updateNonNullValue<tableHasNulls, TData>( ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ velox/functions/prestosql/aggregates/SimpleNumericAggregate.h:213:3: note: candidate function template not viable: no known conversion from 'facebook::velox::UnscaledShortDecimal' to 'facebook::velox::UnscaledLongDecimal' for 2nd argument updateNonNullValue(char* group, TDataType value, Update updateValue) { ^ In file included from velox/functions/prestosql/aggregates/SumAggregate.cpp:16: velox/functions/prestosql/aggregates/SumAggregate.h:87:29: error: no matching member function for call to 'updateOneGroup' BaseAggregate::template updateOneGroup<TAccumulator>( ~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~ velox/functions/prestosql/aggregates/SumAggregate.h:31:12: note: in instantiation of member function 'facebook::velox::aggregate::SumAggregate<facebook::velox::UnscaledShortDecimal, facebook::velox::UnscaledLongDecimal, facebook::velox::UnscaledLongDecimal>::addSingleGroupRawInput' requested here explicit SumAggregate(TypePtr resultType) : BaseAggregate(resultType) {} ^ third-party-buck/platform010/build/libgcc/include/c++/trunk/bits/unique_ptr.h:962:34: note: in instantiation of member function 'facebook::velox::aggregate::SumAggregate<facebook::velox::UnscaledShortDecimal, facebook::velox::UnscaledLongDecimal, facebook::velox::UnscaledLongDecimal>::SumAggregate' requested here { return unique_ptr<_Tp>(new _Tp(std::forward<_Args>(__args)...)); } ^ velox/functions/prestosql/aggregates/SumAggregate.h:290:25: note: in instantiation of function template specialization 'std::make_unique<facebook::velox::aggregate::SumAggregate<facebook::velox::UnscaledShortDecimal, facebook::velox::UnscaledLongDecimal, facebook::velox::UnscaledLongDecimal>, const std::shared_ptr<const facebook::velox::Type> &>' requested here return std::make_unique< ^ velox/functions/prestosql/aggregates/SumAggregate.cpp:22:5: note: in instantiation of function template specialization 'facebook::velox::aggregate::registerSumAggregate<SumAggregate>' requested here registerSumAggregate<SumAggregate>(kSum); ^ velox/functions/prestosql/aggregates/SimpleNumericAggregate.h:134:8: note: candidate function template not viable: no known conversion from 'facebook::velox::UnscaledShortDecimal' to 'facebook::velox::UnscaledLongDecimal' for 7th argument void updateOneGroup( ^ In file included from velox/functions/prestosql/aggregates/SumAggregate.cpp:16: velox/functions/prestosql/aggregates/SumAggregate.h:102:29: error: no matching member function for call to 'updateOneGroup' BaseAggregate::template updateOneGroup<ResultType>( ~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~ velox/functions/prestosql/aggregates/SumAggregate.h:31:12: note: in instantiation of member function 'facebook::velox::aggregate::SumAggregate<facebook::velox::UnscaledShortDecimal, facebook::velox::UnscaledLongDecimal, facebook::velox::UnscaledLongDecimal>::addSingleGroupIntermediateResults' requested here explicit SumAggregate(TypePtr resultType) : BaseAggregate(resultType) {} ^ third-party-buck/platform010/build/libgcc/include/c++/trunk/bits/unique_ptr.h:962:34: note: in instantiation of member function 'facebook::velox::aggregate::SumAggregate<facebook::velox::UnscaledShortDecimal, facebook::velox::UnscaledLongDecimal, facebook::velox::UnscaledLongDecimal>::SumAggregate' requested here { return unique_ptr<_Tp>(new _Tp(std::forward<_Args>(__args)...)); } ^ velox/functions/prestosql/aggregates/SumAggregate.h:290:25: note: in instantiation of function template specialization 'std::make_unique<facebook::velox::aggregate::SumAggregate<facebook::velox::UnscaledShortDecimal, facebook::velox::UnscaledLongDecimal, facebook::velox::UnscaledLongDecimal>, const std::shared_ptr<const facebook::velox::Type> &>' requested here return std::make_unique< ^ velox/functions/prestosql/aggregates/SumAggregate.cpp:22:5: note: in instantiation of function template specialization 'facebook::velox::aggregate::registerSumAggregate<SumAggregate>' requested here registerSumAggregate<SumAggregate>(kSum); ^ velox/functions/prestosql/aggregates/SimpleNumericAggregate.h:134:8: note: candidate function template not viable: no known conversion from 'facebook::velox::UnscaledShortDecimal' to 'facebook::velox::UnscaledLongDecimal' for 7th argument void updateOneGroup( ^ 12 errors generated. Cannot execute a rule out of process. On RE worker. Thread: Thread[main,5,main] Command failed with exit code 1. command: [/re_cwd/fbcode/buck-out/dev/gen/aab7ed39/tools/build/buck/wrappers/fbcc__/fbcc.sh, --cc=/re_cwd/fbcode/third-party-buck/platform010/build/llvm-[fb/bin/clang](https://l.workplace.com/l.php?u=https%3A%2F%2Fwww.fburl.com%2Fbin%2Fclang&h=AT0AMGrb67nKh7cGCCjxOxPtD_GISxoDBqB6bMCXiN880cpgTFp0xgOzyiWAg9WeKtKAQVAy4Pw9JmeIai_Y_mSSKrbyxsXcCigh1MiVueoF4GADSLnNVsUDLNtrtVNrE4bzBhmL32HokiBWJLYRQdf_qIUv7p7R4UPGBA)++, --target=x86_64-redhat-linux-gnu, -nostdinc, -resource-dir, /re_cwd/fbcode/third-party-buck/platform010/build/llvm-[fb/lib/clang/stable](https://l.workplace.com/l.php?u=https%3A%2F%2Fwww.fburl.com%2Flib%2Fclang%2Fstable&h=AT0AMGrb67nKh7cGCCjxOxPtD_GISxoDBqB6bMCXiN880cpgTFp0xgOzyiWAg9WeKtKAQVAy4Pw9JmeIai_Y_mSSKrbyxsXcCigh1MiVueoF4GADSLnNVsUDLNtrtVNrE4bzBhmL32HokiBWJLYRQdf_qIUv7p7R4UPGBA), -idirafter, /re_cwd/fbco... <truncated> ... stderr: In file included from velox/functions/prestosql/aggregates/SumAggregate.cpp:16: In file included from velox/functions/prestosql/aggregates/SumAggregate.h:20: In file included from velox/functions/prestosql/aggregates/SimpleNumericAggregate.h:19: velox/exec/AggregationHook.h:106:14: error: no matching function for call to 'checkedPlus' result = checkedPlus<TOutput>(result, value); ^~~~~~~~~~~~~~~~~~~~ velox/exec/AggregationHook.h:144:5: note: in instantiation of function template specialization 'facebook::velox::aggregate::(anonymous namespace)::updateSingleValue<facebook::velox::UnscaledLongDecimal, facebook::velox::UnscaledShortDecimal>' requested here updateSingleValue( ^ velox/exec/AggregationHook.h:112:7: note: in instantiation of member function 'facebook::velox::aggregate::SumHook<facebook::velox::UnscaledShortDecimal, facebook::velox::UnscaledLongDecimal>::addValue' requested here class SumHook final : public AggregationHook { ^ velox/functions/prestosql/aggregates/SumAggregate.h:124:31: note: in instantiation of function template specialization 'facebook::velox::aggregate::SimpleNumericAggregate<facebook::velox::UnscaledShortDecimal, facebook::velox::UnscaledLongDecimal, facebook::velox::UnscaledLongDecimal>::pushdown<facebook::velox::aggregate::SumHook<facebook::velox::UnscaledShortDecimal, facebook::velox::UnscaledLongDecimal>>' requested here BaseAggregate::template pushdown<SumHook<TInput, TData>>( ^ velox/functions/prestosql/aggregates/SumAggregate.h:71:5: note: in instantiation of function template specialization 'facebook::velox::aggregate::SumAggregate<facebook::velox::UnscaledShortDecimal, facebook::velox::UnscaledLongDecimal, facebook::velox::UnscaledLongDecimal>::updateInternal<facebook::velox::UnscaledLongDecimal>' requested here updateInternal<TAccumulator>(groups, rows, args, mayPushdown); ^ velox/functions/prestosql/aggregates/SumAggregate.h:31:12: note: in instantiation of member function 'facebook::velox::aggregate::SumAggregate<facebook::velox::UnscaledShortDecimal, facebook::velox::UnscaledLongDecimal, facebook::velox::UnscaledLongDecimal>::addRawInput' requested here explicit SumAggregate(TypePtr resultType) : BaseAggregate(resultType) {} ^ third-party-buck/platform010/build/libgcc/include/c++/trunk/bits/unique_ptr.h:962:34: note: in instantiation of member function 'facebook::velox::aggregate::SumAggregate<facebook::velox::UnscaledShortDecimal, facebook::velox::UnscaledLongDecimal, facebook::velox::UnscaledLongDecimal>::SumAggregate' requested here { return unique_ptr<_Tp>(new _Tp(std::forward<_Args>(__args)...)); } ^ velox/functions/prestosql/aggregates/SumAggregate.h:290:25: note: in instantiation of function template specialization 'std::make_unique<facebook::velox::aggregate::SumAggregate<facebook::velox::UnscaledShortDecimal, facebook::velox::UnscaledLongDecimal, facebook::velox::UnscaledLongDecimal>, const std::shared_ptr<const facebook::velox::Type> &>' requested here return std::make_unique< ^ velox/functions/prestosql/aggregates/SumAggregate.cpp:22:5: note: in instantiation of function template specialization 'facebook::velox::aggregate::registerSumAggregate<SumAggregate>' requested here registerSumAggregate<SumAggregate>(kSum); ^ velox/common/base/CheckedArithmetic.h:29:3: note: candidate function template not viable: no known conversion from 'facebook::velox::UnscaledShortDecimal' to 'const facebook::velox::UnscaledLongDecimal' for 2nd argument T checkedPlus(const T& a, const T& b) { ^ In file included from velox/functions/prestosql/aggregates/SumAggregate.cpp:16: In file included from velox/functions/prestosql/aggregates/SumAggregate.h:20: velox/functions/prestosql/aggregates/SimpleNumericAggregate.h:102:11: error: no matching member function for call to 'updateNonNullValue' updateNonNullValue<tableHasNulls, TData>( ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ velox/functions/prestosql/aggregates/SumAggregate.h:130:31: note: in instantiation of function template specialization 'facebook::velox::aggregate::SimpleNumericAggregate<facebook::velox::UnscaledShortDecimal, facebook::velox::UnscaledLongDecimal, facebook::velox::UnscaledLongDecimal>::updateGroups<true, facebook::velox::UnscaledLongDecimal, void (*)(facebook::velox::UnscaledLongDecimal &, facebook::velox::UnscaledLongDecimal)>' requested here BaseAggregate::template updateGroups<true, TData>( ^ velox/functions/prestosql/aggregates/SumAggregate.h:71:5: note: in instantiation of function template specialization 'facebook::velox::aggregate::SumAggregate<facebook::velox::UnscaledShortDecimal, facebook::velox::UnscaledLongDecimal, facebook::velox::UnscaledLongDecimal>::updateInternal<facebook::velox::UnscaledLongDecimal>' requested here updateInternal<TAccumulator>(groups, rows, args, mayPushdown); ^ velox/functions/prestosql/aggregates/SumAggregate.h:31:12: note: in instantiation of member function 'facebook::velox::aggregate::SumAggregate<facebook::velox::UnscaledShortDecimal, facebook::velox::UnscaledLongDecimal, facebook::velox::UnscaledLongDecimal>::addRawInput' requested here explicit SumAggregate(TypePtr resultType) : BaseAggregate(resultType) {} ^ third-party-buck/platform010/build/libgcc/include/c++/trunk/bits/unique_ptr.h:962:34: note: in instantiation of member function 'facebook::velox::aggregate::SumAggregate<facebook::velox::UnscaledShortDecimal, facebook::velox::UnscaledLongDecimal, facebook::velox::UnscaledLongDecimal>::SumAggregate' requested here { return unique_ptr<_Tp>(new _Tp(std::forward<_Args>(__args)...)); } ^ velox/functions/prestosql/aggregates/SumAggregate.h:290:25: note: in instantiation of function template specialization 'std::make_unique<facebook::velox::aggregate::SumAggregate<facebook::velox::UnscaledShortDecimal, facebook::velox::UnscaledLongDecimal, facebook::velox::UnscaledLongDecimal>, const std::shared_ptr<const facebook::velox::Type> &>' requested here return std::make_unique< ^ velox/functions/prestosql/aggregates/SumAggregate.cpp:22:5: note: in instantiation of function template specialization 'facebook::velox::aggregate::registerSumAggregate<SumAggregate>' requested here registerSumAggregate<SumAggregate>(kSum); ^ velox/functions/prestosql/aggregates/SimpleNumericAggregate.h:213:3: note: candidate function template not viable: no known conversion from 'facebook::velox::UnscaledShortDecimal' to 'facebook::velox::UnscaledLongDecimal' for 2nd argument updateNonNullValue(char* group, TDataType value, Update updateValue) { ^ velox/functions/prestosql/aggregates/SimpleNumericAggregate.h:111:9: error: no matching member function for call to 'updateNonNullValue' updateNonNullValue<tableHasNulls, TData>( ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ velox/functions/prestosql/aggregates/SimpleNumericAggregate.h:213:3: note: candidate function template not viable: no known conversion from 'facebook::velox::UnscaledShortDecimal' to 'facebook::velox::UnscaledLongDecimal' for 2nd argument updateNonNullValue(char* group, TDataType value, Update updateValue) { ^ velox/functions/prestosql/aggregates/SimpleNumericAggregate.h:117:9: error: no matching member function for call to 'updateNonNullValue' updateNonNullValue<tableHasNulls, TData>( ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ velox/functions/prestosql/aggregates/SimpleNumericAggregate.h:213:3: note: candidate function template not viable: no known conversion from 'const facebook::velox::UnscaledShortDecimal' to 'facebook::velox::UnscaledLongDecimal' for 2nd argument updateNonNullValue(char* group, TDataType value, Update updateValue) { ^ velox/functions/prestosql/aggregates/SimpleNumericAggregate.h:122:9: error: no matching member function for call to 'updateNonNullValue' updateNonNullValue<tableHasNulls, TData>( ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ velox/functions/prestosql/aggregates/SimpleNumericAggregate.h:213:3: note: candidate function template not viable: no known conversion from 'facebook::velox::UnscaledShortDecimal' to 'facebook::velox::UnscaledLongDecimal' for 2nd argument updateNonNullValue(char* group, TDataType value, Update updateValue) { ^ In file included from velox/functions/prestosql/aggregates/SumAggregate.cpp:16: In file included from velox/functions/prestosql/aggregates/SumAggregate.h:20: In file included from velox/functions/prestosql/aggregates/SimpleNumericAggregate.h:19: velox/exec/AggregationHook.h:172:9: error: no viable conversion from 'const facebook::velox::UnscaledShortDecimal' to 'facebook::velox::UnscaledLongDecimal' *reinterpret_cast<const TValue*>(value)); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ velox/exec/AggregationHook.h:151:7: note: in instantiation of member function 'facebook::velox::aggregate::SimpleCallableHook<facebook::velox::UnscaledShortDecimal, facebook::velox::UnscaledLongDecimal, void (*)(facebook::velox::UnscaledLongDecimal &, facebook::velox::UnscaledLongDecimal)>::addValue' requested here class SimpleCallableHook final : public AggregationHook { ^ velox/functions/prestosql/aggregates/SumAggregate.h:130:31: note: in instantiation of function template specialization 'facebook::velox::aggregate::SimpleNumericAggregate<facebook::velox::UnscaledShortDecimal, facebook::velox::UnscaledLongDecimal, facebook::velox::UnscaledLongDecimal>::updateGroups<true, facebook::velox::UnscaledLongDecimal, void (*)(facebook::velox::UnscaledLongDecimal &, facebook::velox::UnscaledLongDecimal)>' requested here BaseAggregate::template updateGroups<true, TData>( ^ velox/functions/prestosql/aggregates/SumAggregate.h:71:5: note: in instantiation of function template specialization 'facebook::velox::aggregate::SumAggregate<facebook::velox::UnscaledShortDecimal, facebook::velox::UnscaledLongDecimal, facebook::velox::UnscaledLongDecimal>::updateInternal<facebook::velox::UnscaledLongDecimal>' requested here updateInternal<TAccumulator>(groups, rows, args, mayPushdown); ^ velox/functions/prestosql/aggregates/SumAggregate.h:31:12: note: in instantiation of member function 'facebook::velox::aggregate::SumAggregate<facebook::velox::UnscaledShortDecimal, facebook::velox::UnscaledLongDecimal, facebook::velox::UnscaledLongDecimal>::addRawInput' requested here explicit SumAggregate(TypePtr resultType) : BaseAggregate(resultType) {} ^ third-party-buck/platform010/build/libgcc/include/c++/trunk/bits/unique_ptr.h:962:34: note: in instantiation of member function 'facebook::velox::aggregate::SumAggregate<facebook::velox::UnscaledShortDecimal, facebook::velox::UnscaledLongDecimal, facebook::velox::UnscaledLongDecimal>::SumAggregate' requested here { return unique_ptr<_Tp>(new _Tp(std::forward<_Args>(__args)...)); } ^ velox/functions/prestosql/aggregates/SumAggregate.h:290:25: note: in instantiation of function template specialization 'std::make_unique<facebook::velox::aggregate::SumAggregate<facebook::velox::UnscaledShortDecimal, facebook::velox::UnscaledLongDecimal, facebook::velox::UnscaledLongDecimal>, const std::shared_ptr<const facebook::velox::Type> &>' requested here return std::make_unique< ^ velox/functions/prestosql/aggregates/SumAggregate.cpp:22:5: note: in instantiation of function template specialization 'facebook::velox::aggregate::registerSumAggregate<SumAggregate>' requested here registerSumAggregate<SumAggregate>(kSum); ^ velox/type/UnscaledLongDecimal.h:35:8: note: candidate constructor (the implicit copy constructor) not viable: no known conversion from 'const facebook::velox::UnscaledShortDecimal' to 'const facebook::velox::UnscaledLongDecimal &' for 1st argument struct UnscaledLongDecimal { ^ velox/type/UnscaledLongDecimal.h:35:8: note: candidate constructor (the implicit move constructor) not viable: no known conversion from 'const facebook::velox::UnscaledShortDecimal' to 'facebook::velox::UnscaledLongDecimal &&' for 1st argument velox/type/UnscaledLongDecimal.h:40:22: note: explicit constructor is not a candidate constexpr explicit UnscaledLongDecimal(int128_t value) ^ velox/type/UnscaledLongDecimal.h:43:22: note: explicit constructor is not a candidate constexpr explicit UnscaledLongDecimal(int64_t value) ^ note: remaining 2 candidates omitted; pass -fshow-overloads=all to show them In file included from velox/functions/prestosql/aggregates/SumAggregate.cpp:16: In file included from velox/functions/prestosql/aggregates/SumAggregate.h:20: velox/functions/prestosql/aggregates/SimpleNumericAggregate.h:102:11: error: no matching member function for call to 'updateNonNullValue' updateNonNullValue<tableHasNulls, TData>( ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ velox/functions/prestosql/aggregates/SumAggregate.h:133:31: note: in instantiation of function template specialization 'facebook::velox::aggregate::SimpleNumericAggregate<facebook::velox::UnscaledShortDecimal, facebook::velox::UnscaledLongDecimal, facebook::velox::UnscaledLongDecimal>::updateGroups<false, facebook::velox::UnscaledLongDecimal, void (*)(facebook::velox::UnscaledLongDecimal &, facebook::velox::UnscaledLongDecimal)>' requested here BaseAggregate::template updateGroups<false, TData>( ^ velox/functions/prestosql/aggregates/SumAggregate.h:71:5: note: in instantiation of function template specialization 'facebook::velox::aggregate::SumAggregate<facebook::velox::UnscaledShortDecimal, facebook::velox::UnscaledLongDecimal, facebook::velox::UnscaledLongDecimal>::updateInternal<facebook::velox::UnscaledLongDecimal>' requested here updateInternal<TAccumulator>(groups, rows, args, mayPushdown); ^ velox/functions/prestosql/aggregates/SumAggregate.h:31:12: note: in instantiation of member function 'facebook::velox::aggregate::SumAggregate<facebook::velox::UnscaledShortDecimal, facebook::velox::UnscaledLongDecimal, facebook::velox::UnscaledLongDecimal>::addRawInput' requested here explicit SumAggregate(TypePtr resultType) : BaseAggregate(resultType) {} ^ third-party-buck/platform010/build/libgcc/include/c++/trunk/bits/unique_ptr.h:962:34: note: in instantiation of member function 'facebook::velox::aggregate::SumAggregate<facebook::velox::UnscaledShortDecimal, facebook::velox::UnscaledLongDecimal, facebook::velox::UnscaledLongDecimal>::SumAggregate' requested here { return unique_ptr<_Tp>(new _Tp(std::forward<_Args>(__args)...)); } ^ velox/functions/prestosql/aggregates/SumAggregate.h:290:25: note: in instantiation of function template specialization 'std::make_unique<facebook::velox::aggregate::SumAggregate<facebook::velox::UnscaledShortDecimal, facebook::velox::UnscaledLongDecimal, facebook::velox::UnscaledLongDecimal>, const std::shared_ptr<const facebook::velox::Type> &>' requested here return std::make_unique< ^ velox/functions/prestosql/aggregates/SumAggregate.cpp:22:5: note: in instantiation of function template specialization 'facebook::velox::aggregate::registerSumAggregate<SumAggregate>' requested here registerSumAggregate<SumAggregate>(kSum); ^ velox/functions/prestosql/aggregates/SimpleNumericAggregate.h:213:3: note: candidate function template not viable: no known conversion from 'facebook::velox::UnscaledShortDecimal' to 'facebook::velox::UnscaledLongDecimal' for 2nd argument updateNonNullValue(char* group, TDataType value, Update updateValue) { ^ velox/functions/prestosql/aggregates/SimpleNumericAggregate.h:111:9: error: no matching member function for call to 'updateNonNullValue' updateNonNullValue<tableHasNulls, TData>( ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ velox/functions/prestosql/aggregates/SimpleNumericAggregate.h:213:3: note: candidate function template not viable: no known conversion from 'facebook::velox::UnscaledShortDecimal' to 'facebook::velox::UnscaledLongDecimal' for 2nd argument updateNonNullValue(char* group, TDataType value, Update updateValue) { ^ velox/functions/prestosql/aggregates/SimpleNumericAggregate.h:117:9: error: no matching member function for call to 'updateNonNullValue' updateNonNullValue<tableHasNulls, TData>( ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ velox/functions/prestosql/aggregates/SimpleNumericAggregate.h:213:3: note: candidate function template not viable: no known conversion from 'const facebook::velox::UnscaledShortDecimal' to 'facebook::velox::UnscaledLongDecimal' for 2nd argument updateNonNullValue(char* group, TDataType value, Update updateValue) { ^ velox/functions/prestosql/aggregates/SimpleNumericAggregate.h:122:9: error: no matching member function for call to 'updateNonNullValue' updateNonNullValue<tableHasNulls, TData>( ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ velox/functions/prestosql/aggregates/SimpleNumericAggregate.h:213:3: note: candidate function template not viable: no known conversion from 'facebook::velox::UnscaledShortDecimal' to 'facebook::velox::UnscaledLongDecimal' for 2nd argument updateNonNullValue(char* group, TDataType value, Update updateValue) { ^ In file included from velox/functions/prestosql/aggregates/SumAggregate.cpp:16: velox/functions/prestosql/aggregates/SumAggregate.h:87:29: error: no matching member function for call to 'updateOneGroup' BaseAggregate::template updateOneGroup<TAccumulator>( ~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~ velox/functions/prestosql/aggregates/SumAggregate.h:31:12: note: in instantiation of member function 'facebook::velox::aggregate::SumAggregate<facebook::velox::UnscaledShortDecimal, facebook::velox::UnscaledLongDecimal, facebook::velox::UnscaledLongDecimal>::addSingleGroupRawInput' requested here explicit SumAggregate(TypePtr resultType) : BaseAggregate(resultType) {} ^ third-party-buck/platform010/build/libgcc/include/c++/trunk/bits/unique_ptr.h:962:34: note: in instantiation of member function 'facebook::velox::aggregate::SumAggregate<facebook::velox::UnscaledShortDecimal, facebook::velox::UnscaledLongDecimal, facebook::velox::UnscaledLongDecimal>::SumAggregate' requested here { return unique_ptr<_Tp>(new _Tp(std::forward<_Args>(__args)...)); } ^ velox/functions/prestosql/aggregates/SumAggregate.h:290:25: note: in instantiation of function template specialization 'std::make_unique<facebook::velox::aggregate::SumAggregate<facebook::velox::UnscaledShortDecimal, facebook::velox::UnscaledLongDecimal, facebook::velox::UnscaledLongDecimal>, const std::shared_ptr<const facebook::velox::Type> &>' requested here return std::make_unique< ^ velox/functions/prestosql/aggregates/SumAggregate.cpp:22:5: note: in instantiation of function template specialization 'facebook::velox::aggregate::registerSumAggregate<SumAggregate>' requested here registerSumAggregate<SumAggregate>(kSum); ^ velox/functions/prestosql/aggregates/SimpleNumericAggregate.h:134:8: note: candidate function template not viable: no known conversion from 'facebook::velox::UnscaledShortDecimal' to 'facebook::velox::UnscaledLongDecimal' for 7th argument void updateOneGroup( ^ In file included from velox/functions/prestosql/aggregates/SumAggregate.cpp:16: velox/functions/prestosql/aggregates/SumAggregate.h:102:29: error: no matching member function for call to 'updateOneGroup' BaseAggregate::template updateOneGroup<ResultType>( ~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~ velox/functions/prestosql/aggregates/SumAggregate.h:31:12: note: in instantiation of member function 'facebook::velox::aggregate::SumAggregate<facebook::velox::UnscaledShortDecimal, facebook::velox::UnscaledLongDecimal, facebook::velox::UnscaledLongDecimal>::addSingleGroupIntermediateResults' requested here explicit SumAggregate(TypePtr resultType) : BaseAggregate(resultType) {} ^ third-party-buck/platform010/build/libgcc/include/c++/trunk/bits/unique_ptr.h:962:34: note: in instantiation of member function 'facebook::velox::aggregate::SumAggregate<facebook::velox::UnscaledShortDecimal, facebook::velox::UnscaledLongDecimal, facebook::velox::UnscaledLongDecimal>::SumAggregate' requested here { return unique_ptr<_Tp>(new _Tp(std::forward<_Args>(__args)...)); } ^ velox/functions/prestosql/aggregates/SumAggregate.h:290:25: note: in instantiation of function template specialization 'std::make_unique<facebook::velox::aggregate::SumAggregate<facebook::velox::UnscaledShortDecimal, facebook::velox::UnscaledLongDecimal, facebook::velox::UnscaledLongDecimal>, const std::shared_ptr<const facebook::velox::Type> &>' requested here return std::make_unique< ^ velox/functions/prestosql/aggregates/SumAggregate.cpp:22:5: note: in instantiation of function template specialization 'facebook::velox::aggregate::registerSumAggregate<SumAggregate>' requested here registerSumAggregate<SumAggregate>(kSum); ^ velox/functions/prestosql/aggregates/SimpleNumericAggregate.h:134:8: note: candidate function template not viable: no known conversion from 'facebook::velox::UnscaledShortDecimal' to 'facebook::velox::UnscaledLongDecimal' for 7th argument void updateOneGroup( ^ 12 errors generated. When running <c++ preprocess_and_compile>. When running <remote_execution>. When building rule //velox/functions/prestosql/aggregates:aggregates#compile-pic-SumAggregate.cpp.o49bb68fa,platform010-clang (ovr_config//platform/linux:x86_64-fbcode).

xiaoxmeng · 2022-08-24T16:22:39Z

velox/type/tests/DecimalTest.cpp

+TEST(DecimalTest, overloads) {
+  ASSERT_EQ(UnscaledShortDecimal(3), UnscaledShortDecimal(10) / 3);
+  ASSERT_EQ(UnscaledLongDecimal(33), UnscaledLongDecimal(100) / 3);
+  ASSERT_EQ(UnscaledLongDecimal(300), UnscaledLongDecimal(100) * 3);


operator doesn't work in fb internal tests. We can disable this and I can help on turn on this later.

Can I use the mul https://github.com/facebookincubator/velox/blob/main/velox/type/DecimalUtil.h#L33 ?

I also want to discuss this issue further here #2388

xiaoxmeng · 2022-08-24T16:23:58Z

velox/functions/prestosql/aggregates/tests/SumTest.cpp

@@ -461,5 +487,14 @@ TEST_F(SumTest, floatAggregateOverflow) {
  testAggregateOverflow<float, float, double>();
  testAggregateOverflow<double, double>();
 }
+
+TEST_F(SumTest, decimalAggregateOverflow) {


Please skip this as it doesn't pass in fb internal test. I can help to turn on this later.

velox/exec/AggregationHook.h

facebook-github-bot · 2022-08-25T22:40:56Z

@xiaoxmeng has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

xiaoxmeng

Some nits found by FB internal lint.

xiaoxmeng · 2022-08-29T16:39:42Z

velox/exec/Aggregate.h

+  /// Some types such as int128_t require aligned access.
+  virtual int32_t accumulatorAlignmentSize() const {
+    return 1;
+  };


Remove the trailing semicolon.

xiaoxmeng · 2022-08-29T16:40:45Z

velox/type/UnscaledLongDecimal.h

+
+  constexpr explicit UnscaledLongDecimal(int value) : unscaledValue_(value) {}
+
+  UnscaledLongDecimal(UnscaledShortDecimal value)


Add explicit.

facebook-github-bot · 2022-08-29T16:42:32Z

@xiaoxmeng has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2022-08-29T19:31:20Z

@xiaoxmeng has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2022-08-29T20:10:09Z

@xiaoxmeng has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 1, 2022

majetideepak changed the title ~~Add support for Decimal Type sum aggregation~~ Add support for Decimal Type Sum Aggregation Aug 1, 2022

majetideepak marked this pull request as draft August 1, 2022 14:52

majetideepak commented Aug 1, 2022

View reviewed changes

velox/exec/GroupingSet.cpp Outdated Show resolved Hide resolved

majetideepak marked this pull request as ready for review August 1, 2022 15:37

majetideepak requested review from mbasmanova and karteekmurthys August 1, 2022 15:57

majetideepak force-pushed the decimal-sum-agg branch from 4841bbd to 05301ee Compare August 1, 2022 22:51

mbasmanova reviewed Aug 2, 2022

View reviewed changes

majetideepak mentioned this pull request Aug 2, 2022

Check for overflow in sum aggregate function #2010

Closed

majetideepak force-pushed the decimal-sum-agg branch 4 times, most recently from 1262bbf to 977fca5 Compare August 4, 2022 22:40

majetideepak requested a review from mbasmanova August 4, 2022 23:21

majetideepak commented Aug 5, 2022

View reviewed changes

majetideepak marked this pull request as draft August 5, 2022 11:10

karteekmurthys reviewed Aug 8, 2022

View reviewed changes

velox/functions/prestosql/aggregates/tests/SumTest.cpp Show resolved Hide resolved

majetideepak mentioned this pull request Aug 11, 2022

Fix sum aggregate function #2225

Closed

majetideepak force-pushed the decimal-sum-agg branch from 977fca5 to 180e234 Compare August 17, 2022 22:43

majetideepak marked this pull request as ready for review August 17, 2022 22:48

majetideepak force-pushed the decimal-sum-agg branch 2 times, most recently from 98e511a to ee68e05 Compare August 18, 2022 12:42

majetideepak force-pushed the decimal-sum-agg branch from ee68e05 to 2d914d0 Compare August 22, 2022 00:53

majetideepak force-pushed the decimal-sum-agg branch from 2d914d0 to 6e23d96 Compare August 22, 2022 03:51

xiaoxmeng approved these changes Aug 22, 2022

View reviewed changes

mbasmanova approved these changes Aug 23, 2022

View reviewed changes

majetideepak force-pushed the decimal-sum-agg branch from b5260d9 to c56bde0 Compare August 23, 2022 11:58

majetideepak force-pushed the decimal-sum-agg branch from c56bde0 to 0cf5a75 Compare August 23, 2022 15:41

xiaoxmeng reviewed Aug 24, 2022

View reviewed changes

majetideepak force-pushed the decimal-sum-agg branch 3 times, most recently from 5c003ef to f867628 Compare August 25, 2022 22:00

xiaoxmeng reviewed Aug 25, 2022

View reviewed changes

velox/exec/AggregationHook.h Outdated Show resolved Hide resolved

majetideepak added 7 commits August 28, 2022 23:30

Add support for Decimal Type Sum Aggreggation

c098fbe

Address review comments

46894aa

address review comments

6104d3a

improve inferDecimalType to support re-used variables

3ef4e00

address review comments

5123b72

Address review comments

5bc4f2c

Fix SumTest overload

c634fed

majetideepak force-pushed the decimal-sum-agg branch from f867628 to c634fed Compare August 29, 2022 03:31

xiaoxmeng reviewed Aug 29, 2022

View reviewed changes

Address review comments

2d95838

facebook-github-bot closed this in bc0fc81 Aug 29, 2022

majetideepak deleted the decimal-sum-agg branch September 22, 2022 19:18

	template <
	typename TData = TResult,
	typename UpdateSingle,
	typename UpdateDuplicate>
	void updateOneGroup(
	char* group,
	const SelectivityVector& rows,
	const VectorPtr& arg,
	UpdateSingle updateSingleValue,
	UpdateDuplicate updateDuplicateValues,
	bool /mayPushdown/,
	TData initialValue) {
	DecodedVector decoded(*arg, rows);

	// Do row by row if not all rows are selected.
	if (decoded.isConstantMapping()) {
	if (!decoded.isNullAt(0)) {
	updateDuplicateValues(
	initialValue, decoded.valueAt<TInput>(0), rows.countSelected());
	updateNonNullValue<true, TData>(group, initialValue, updateSingleValue);
	}
	} else if (decoded.mayHaveNulls()) {


		constexpr explicit UnscaledLongDecimal(int value) : unscaledValue_(value) {}

		UnscaledLongDecimal(UnscaledShortDecimal value)

Add support for Decimal Type Sum Aggregation #2163

Add support for Decimal Type Sum Aggregation #2163

Conversation

majetideepak commented Aug 1, 2022 • edited Loading

mbasmanova left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

majetideepak Aug 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Aug 18, 2022

netlify bot commented Aug 22, 2022 • edited Loading

✅ Deploy Preview for meta-velox canceled.

xiaoxmeng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Aug 22, 2022

mbasmanova left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

majetideepak commented Aug 23, 2022

mbasmanova commented Aug 23, 2022

facebook-github-bot commented Aug 23, 2022

xiaoxmeng commented Aug 23, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Aug 25, 2022

xiaoxmeng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Aug 29, 2022

facebook-github-bot commented Aug 29, 2022

facebook-github-bot commented Aug 29, 2022

majetideepak commented Aug 1, 2022 •

edited

Loading

majetideepak Aug 2, 2022 •

edited

Loading

netlify bot commented Aug 22, 2022 •

edited

Loading