Merge pull request #3391 from 4ertus2/decimal

fix for Decimal128 group by [issue-3378]

dbms/src/Columns/ColumnDecimal.cpp

@@ -117,6 +117,14 @@ MutableColumnPtr ColumnDecimal<T>::cloneResized(size_t size) const
     return std::move(res);
 }
 
+template <typename T>
+void ColumnDecimal<T>::insertData(const char * src, size_t /*length*/)
+{
+    T tmp;
+    memcpy(&tmp, src, sizeof(T));
+    data.emplace_back(tmp);
+}
+
 template <typename T>
 void ColumnDecimal<T>::insertRangeFrom(const IColumn & src, size_t start, size_t length)
 {

dbms/src/Columns/ColumnDecimal.h

@@ -89,7 +89,7 @@ class ColumnDecimal final : public COWPtrHelper<IColumn, ColumnDecimal<T>>
     void reserve(size_t n) override { data.reserve(n); }
 
     void insertFrom(const IColumn & src, size_t n) override { data.push_back(static_cast<const Self &>(src).getData()[n]); }
-    void insertData(const char * pos, size_t /*length*/) override { data.push_back(*reinterpret_cast<const T *>(pos)); }
+    void insertData(const char * pos, size_t /*length*/) override;
     void insertDefault() override { data.push_back(T()); }
     void insert(const Field & x) override { data.push_back(DB::get<typename NearestFieldType<T>::Type>(x)); }
     void insertRangeFrom(const IColumn & src, size_t start, size_t length) override;

dbms/src/Interpreters/Aggregator.cpp

@@ -1132,6 +1132,9 @@ void Aggregator::convertToBlockImpl(
     if (data.empty())
         return;
 
+    if (key_columns.size() != params.keys_size)
+        throw Exception{"Aggregate. Unexpected key columns size.", ErrorCodes::LOGICAL_ERROR};
+
     if (final)
         convertToBlockImplFinal(method, data, key_columns, final_aggregate_columns);
     else
@@ -1151,7 +1154,7 @@ void NO_INLINE Aggregator::convertToBlockImplFinal(
 {
     for (const auto & value : data)
     {
-        method.insertKeyIntoColumns(value, key_columns, params.keys_size, key_sizes);
+        method.insertKeyIntoColumns(value, key_columns, key_sizes);
 
         for (size_t i = 0; i < params.aggregates_size; ++i)
             aggregate_functions[i]->insertResultInto(
@@ -1169,10 +1172,9 @@ void NO_INLINE Aggregator::convertToBlockImplNotFinal(
     MutableColumns & key_columns,
     AggregateColumnsData & aggregate_columns) const
 {
-
     for (auto & value : data)
     {
-        method.insertKeyIntoColumns(value, key_columns, params.keys_size, key_sizes);
+        method.insertKeyIntoColumns(value, key_columns, key_sizes);
 
         /// reserved, so push_back does not throw exceptions
         for (size_t i = 0; i < params.aggregates_size; ++i)

dbms/src/Interpreters/Aggregator.h

@@ -166,7 +166,7 @@ struct AggregationMethodOneNumber
 
     /** Insert the key from the hash table into columns.
       */
-    static void insertKeyIntoColumns(const typename Data::value_type & value, MutableColumns & key_columns, size_t /*keys_size*/, const Sizes & /*key_sizes*/)
+    static void insertKeyIntoColumns(const typename Data::value_type & value, MutableColumns & key_columns, const Sizes & /*key_sizes*/)
     {
         static_cast<ColumnVector<FieldType> *>(key_columns[0].get())->insertData(reinterpret_cast<const char *>(&value.first), sizeof(value.first));
     }
@@ -243,7 +243,7 @@ struct AggregationMethodString
         return StringRef(value.first.data, value.first.size);
     }
 
-    static void insertKeyIntoColumns(const typename Data::value_type & value, MutableColumns & key_columns, size_t, const Sizes &)
+    static void insertKeyIntoColumns(const typename Data::value_type & value, MutableColumns & key_columns, const Sizes &)
     {
         key_columns[0]->insertData(value.first.data, value.first.size);
     }
@@ -312,7 +312,7 @@ struct AggregationMethodFixedString
         return StringRef(value.first.data, value.first.size);
     }
 
-    static void insertKeyIntoColumns(const typename Data::value_type & value, MutableColumns & key_columns, size_t, const Sizes &)
+    static void insertKeyIntoColumns(const typename Data::value_type & value, MutableColumns & key_columns, const Sizes &)
     {
         key_columns[0]->insertData(value.first.data, value.first.size);
     }
@@ -580,7 +580,7 @@ struct AggregationMethodSingleLowCardinalityColumn : public SingleColumnMethod
     static const bool no_consecutive_keys_optimization = true;
     static const bool low_cardinality_optimization = true;
 
-    static void insertKeyIntoColumns(const typename Data::value_type & value, MutableColumns & key_columns, size_t /*keys_size*/, const Sizes & /*key_sizes*/)
+    static void insertKeyIntoColumns(const typename Data::value_type & value, MutableColumns & key_columns, const Sizes & /*key_sizes*/)
     {
         auto ref = Base::getValueRef(value);
         static_cast<ColumnLowCardinality *>(key_columns[0].get())->insertData(ref.data, ref.size);
@@ -783,8 +783,10 @@ struct AggregationMethodKeysFixed
     static const bool no_consecutive_keys_optimization = false;
     static const bool low_cardinality_optimization = false;
 
-    static void insertKeyIntoColumns(const typename Data::value_type & value, MutableColumns & key_columns, size_t keys_size, const Sizes & key_sizes)
+    static void insertKeyIntoColumns(const typename Data::value_type & value, MutableColumns & key_columns, const Sizes & key_sizes)
     {
+        size_t keys_size = key_columns.size();
+
         static constexpr auto bitmap_size = has_nullable_keys ? std::tuple_size<KeysNullMap<Key>>::value : 0;
         /// In any hash key value, column values to be read start just after the bitmap, if it exists.
         size_t pos = bitmap_size;
@@ -891,10 +893,10 @@ struct AggregationMethodSerialized
     static const bool no_consecutive_keys_optimization = true;
     static const bool low_cardinality_optimization = false;
 
-    static void insertKeyIntoColumns(const typename Data::value_type & value, MutableColumns & key_columns, size_t keys_size, const Sizes &)
+    static void insertKeyIntoColumns(const typename Data::value_type & value, MutableColumns & key_columns, const Sizes &)
     {
         auto pos = value.first.data;
-        for (size_t i = 0; i < keys_size; ++i)
+        for (size_t i = 0; i < key_columns.size(); ++i)
             pos = key_columns[i]->deserializeAndInsertFromArena(pos);
     }
 
@@ -1284,10 +1286,10 @@ class Aggregator
         Block intermediate_header;
 
         /// What to count.
-        ColumnNumbers keys;
-        AggregateDescriptions aggregates;
-        size_t keys_size;
-        size_t aggregates_size;
+        const ColumnNumbers keys;
+        const AggregateDescriptions aggregates;
+        const size_t keys_size;
+        const size_t aggregates_size;
 
         /// The settings of approximate calculation of GROUP BY.
         const bool overflow_row;    /// Do we need to put into AggregatedDataVariants::without_key aggregates for keys that are not in max_rows_to_group_by.
@@ -1344,9 +1346,6 @@ class Aggregator
         {
             intermediate_header = intermediate_header_;
         }
-
-        /// Calculate the column numbers in `keys` and `aggregates`.
-        void calculateColumnNumbers(const Block & block);
     };
 
     Aggregator(const Params & params_);

dbms/tests/queries/0_stateless/00737_decimal_group_by.reference

@@ -0,0 +1,11 @@
+1.10
+2.1000
+3.100000000000
+1.20
+2.2000
+3.200000000000
+1.30
+2.3000
+3.300000000000
+1	1.000000000000000000	10.000000000000000000
+1	1.000000000000000000	10.000000000000000000

dbms/tests/queries/0_stateless/00737_decimal_group_by.sql

@@ -0,0 +1,26 @@
+select toDecimal32(1.1, 2) as x group by x;
+select toDecimal64(2.1, 4) as x group by x;
+select toDecimal128(3.1, 12) as x group by x;
+
+select materialize(toDecimal32(1.2, 2)) as x group by x;
+select materialize(toDecimal64(2.2, 4)) as x group by x;
+select materialize(toDecimal128(3.2, 12)) as x group by x;
+
+select x from (select toDecimal32(1.3, 2) x) group by x;
+select x from (select toDecimal64(2.3, 4) x) group by x;
+select x from (select toDecimal128(3.3, 12) x) group by x;
+
+DROP TABLE IF EXISTS test.decimal;
+CREATE TABLE IF NOT EXISTS test.decimal
+(
+    A UInt64,
+    B Decimal128(18),
+    C Decimal128(18)
+) Engine = Memory;
+
+INSERT INTO test.decimal VALUES (1,1,1), (1,1,2), (1,1,3), (1,1,4);
+
+SELECT A, toString(B) AS B_str, toString(SUM(C)) AS c_str FROM test.decimal GROUP BY A, B_str;
+SELECT A, B_str, toString(cc) FROM (SELECT A, toString(B) AS B_str, SUM(C) AS cc FROM test.decimal GROUP BY A, B_str);
+
+DROP TABLE test.decimal;