address review comments

facebookincubator · Aug 25, 2022 · f867628 · f867628
1 parent 3836004
commit f867628
Show file tree

Hide file tree

Showing 4 changed files with 22 additions and 21 deletions.
diff --git a/velox/exec/AggregationHook.h b/velox/exec/AggregationHook.h
@@ -97,8 +97,8 @@ class AggregationHook : public ValueHook {
 };
 
 namespace {
-template <typename TOutput, typename TInput>
-inline void updateSingleValue(TOutput& result, TInput value) {
+template <typename TOutput>
+inline void updateSingleValue(TOutput& result, TOutput value) {
   if constexpr (
       std::is_same_v<TOutput, double> || std::is_same_v<TOutput, float>) {
     result += value;
@@ -143,7 +143,7 @@ class SumHook final : public AggregationHook {
     clearNull(group);
     updateSingleValue(
         *reinterpret_cast<TAggregate*>(group + offset_),
-        *reinterpret_cast<const TValue*>(value));
+        TAggregate(*reinterpret_cast<const TValue*>(value)));
   }
 };
 

diff --git a/velox/functions/prestosql/aggregates/SimpleNumericAggregate.h b/velox/functions/prestosql/aggregates/SimpleNumericAggregate.h
@@ -100,7 +100,7 @@ class SimpleNumericAggregate : public exec::Aggregate {
         auto value = decoded.valueAt<TInput>(0);
         rows.applyToSelected([&](vector_size_t i) {
           updateNonNullValue<tableHasNulls, TData>(
-              groups[i], value, updateSingleValue);
+              groups[i], TData(value), updateSingleValue);
         });
       }
     } else if (decoded.mayHaveNulls()) {
@@ -109,18 +109,18 @@ class SimpleNumericAggregate : public exec::Aggregate {
           return;
         }
         updateNonNullValue<tableHasNulls, TData>(
-            groups[i], decoded.valueAt<TInput>(i), updateSingleValue);
+            groups[i], TData(decoded.valueAt<TInput>(i)), updateSingleValue);
       });
     } else if (decoded.isIdentityMapping() && !std::is_same_v<TInput, bool>) {
       auto data = decoded.data<TInput>();
       rows.applyToSelected([&](vector_size_t i) {
         updateNonNullValue<tableHasNulls, TData>(
-            groups[i], data[i], updateSingleValue);
+            groups[i], TData(data[i]), updateSingleValue);
       });
     } else {
       rows.applyToSelected([&](vector_size_t i) {
         updateNonNullValue<tableHasNulls, TData>(
-            groups[i], decoded.valueAt<TInput>(i), updateSingleValue);
+            groups[i], TData(decoded.valueAt<TInput>(i)), updateSingleValue);
       });
     }
   }
@@ -154,17 +154,18 @@ class SimpleNumericAggregate : public exec::Aggregate {
           return;
         }
         updateNonNullValue<true, TData>(
-            group, decoded.valueAt<TInput>(i), updateSingleValue);
+            group, TData(decoded.valueAt<TInput>(i)), updateSingleValue);
       });
     } else if (decoded.isIdentityMapping() && !std::is_same_v<TInput, bool>) {
       auto data = decoded.data<TInput>();
       rows.applyToSelected([&](vector_size_t i) {
-        updateNonNullValue<true, TData>(group, data[i], updateSingleValue);
+        updateNonNullValue<true, TData>(
+            group, TData(data[i]), updateSingleValue);
       });
     } else {
       rows.applyToSelected([&](vector_size_t i) {
         updateNonNullValue<true, TData>(
-            group, decoded.valueAt<TInput>(i), updateSingleValue);
+            group, TData(decoded.valueAt<TInput>(i)), updateSingleValue);
       });
     }
   }

diff --git a/velox/type/DecimalUtil.h b/velox/type/DecimalUtil.h
@@ -24,16 +24,6 @@
 
 namespace facebook::velox {
 
-#if defined(__has_feature)
-#if __has_feature(__address_sanitizer__)
-__attribute__((__no_sanitize__("signed-integer-overflow")))
-#endif
-#endif
-inline int128_t
-mul(int128_t x, const int128_t y) {
-  return x * y;
-}
-
 /// A static class that holds helper functions for DECIMAL type.
 class DecimalUtil {
  public:

diff --git a/velox/type/UnscaledLongDecimal.h b/velox/type/UnscaledLongDecimal.h
@@ -32,6 +32,16 @@ constexpr int128_t buildInt128(uint64_t hi, uint64_t lo) {
   return (static_cast<__uint128_t>(hi) << 64) | lo;
 }
 
+#if defined(__has_feature)
+#if __has_feature(__address_sanitizer__)
+__attribute__((__no_sanitize__("signed-integer-overflow")))
+#endif
+#endif
+inline int128_t
+mul(int128_t x, const int128_t y) {
+  return x * y;
+}
+
 struct UnscaledLongDecimal {
  public:
   // Default required for creating vector with NULL values.
@@ -94,7 +104,7 @@ static inline UnscaledLongDecimal operator/(
 static inline UnscaledLongDecimal operator*(
     const UnscaledLongDecimal& a,
     int b) {
-  return UnscaledLongDecimal(a.unscaledValue() * b);
+  return UnscaledLongDecimal(mul(a.unscaledValue(), b));
 }
 } // namespace facebook::velox