[fix](hist) Fix unstable result of aggregrate function hist (#38608)

* Target Fix unstable result of hist function when involving null value. * Reproduce test result of `regression-test/suites/query_p0/sql_functions/aggregate_functions/test_aggregate_all_functions2.groovy` is unstable, sql `SELECT histogram(k7, 5) FROM baseall` will sometimes acts like the second argument is not passed in. * Root reason We have short-circuit in AggregateFunctionNullVariadicInline, when this row is NULL, the value will not be added by the nested function. Implementation of histogram relies on its add method to get its seconds argument, when we have an all null value block, histogram will not get its seconds arg even if sql is like `select(k7, 5)`, so a max_bucket_num with default value 128 is serialized. When we do merging, and happens to deserialize the above block at last, the max_bucket_num in merge stage will be assigned to 128, and this leads to the wrong result. * Fix by Init value of max_bucket_num is assigned to 0, when we do merging, we will discard this aggregated data if its max_bucket_num is 0.
apache · Aug 5, 2024 · a83bf6c · a83bf6c
1 parent 1a6a3c0
commit a83bf6c
Show file tree

Hide file tree

Showing 3 changed files with 193 additions and 15 deletions.
diff --git a/be/src/vec/aggregate_functions/aggregate_function_histogram.h b/be/src/vec/aggregate_functions/aggregate_function_histogram.h
@@ -28,6 +28,8 @@
 #include <utility>
 #include <vector>
 
+#include "common/exception.h"
+#include "common/status.h"
 #include "vec/aggregate_functions/aggregate_function.h"
 #include "vec/aggregate_functions/aggregate_function_simple_factory.h"
 #include "vec/columns/column.h"
@@ -57,12 +59,10 @@ template <typename T>
 struct AggregateFunctionHistogramData {
     using ColVecType =
             std::conditional_t<IsDecimalNumber<T>, ColumnDecimal<Decimal128V2>, ColumnVector<T>>;
+    const static size_t DEFAULT_BUCKET_NUM = 128;
+    const static size_t BUCKET_NUM_INIT_VALUE = 0;
 
-    void set_parameters(int input_max_num_buckets) {
-        if (input_max_num_buckets > 0) {
-            max_num_buckets = (size_t)input_max_num_buckets;
-        }
-    }
+    void set_parameters(size_t input_max_num_buckets) { max_num_buckets = input_max_num_buckets; }
 
     void reset() { ordered_map.clear(); }
 
@@ -86,6 +86,8 @@ struct AggregateFunctionHistogramData {
     }
 
     void merge(const AggregateFunctionHistogramData& rhs) {
+        // if rhs.max_num_buckets == 0, it means the input block for serialization is all null
+        // we should discard this data, because histogram only fouce on the not-null data
         if (!rhs.max_num_buckets) {
             return;
         }
@@ -104,7 +106,6 @@ struct AggregateFunctionHistogramData {
 
     void write(BufferWritable& buf) const {
         write_binary(max_num_buckets, buf);
-
         size_t element_number = (size_t)ordered_map.size();
         write_binary(element_number, buf);
 
@@ -148,7 +149,13 @@ struct AggregateFunctionHistogramData {
     std::string get(const DataTypePtr& data_type) const {
         std::vector<Bucket<T>> buckets;
         rapidjson::StringBuffer buffer;
-        build_histogram(buckets, ordered_map, max_num_buckets);
+        // NOTE: We need an extral branch for to handle max_num_buckets == 0,
+        // when target column is nullable, and input block is all null,
+        // set_parameters will not be called because of the short-circuit in
+        // AggregateFunctionNullVariadicInline, so max_num_buckets will be 0 in this situation.
+        build_histogram(
+                buckets, ordered_map,
+                max_num_buckets == BUCKET_NUM_INIT_VALUE ? DEFAULT_BUCKET_NUM : max_num_buckets);
         histogram_to_json(buffer, buckets, data_type);
         return std::string(buffer.GetString());
     }
@@ -162,7 +169,7 @@ struct AggregateFunctionHistogramData {
     }
 
 private:
-    size_t max_num_buckets = 128;
+    size_t max_num_buckets = BUCKET_NUM_INIT_VALUE;
     std::map<T, size_t> ordered_map;
 };
 
@@ -186,13 +193,17 @@ class AggregateFunctionHistogram final
 
     void add(AggregateDataPtr __restrict place, const IColumn** columns, ssize_t row_num,
              Arena* arena) const override {
-        if (columns[0]->is_null_at(row_num)) {
-            return;
-        }
-
-        if (has_input_param) {
-            this->data(place).set_parameters(
-                    assert_cast<const ColumnInt32*>(columns[1])->get_element(row_num));
+        if constexpr (has_input_param) {
+            Int32 input_max_num_buckets =
+                    assert_cast<const ColumnInt32*>(columns[1])->get_element(row_num);
+            if (input_max_num_buckets <= 0) {
+                throw doris::Exception(ErrorCode::INVALID_ARGUMENT,
+                                       "Invalid max_num_buckets {}, row_num {}",
+                                       input_max_num_buckets, row_num);
+            }
+            this->data(place).set_parameters(input_max_num_buckets);
+        } else {
+            this->data(place).set_parameters(Data::DEFAULT_BUCKET_NUM);
         }
 
         if constexpr (std::is_same_v<T, std::string>) {

diff --git a/...on-test/data/query_p0/sql_functions/aggregate_functions/test_aggregate_all_functions2.out b/...on-test/data/query_p0/sql_functions/aggregate_functions/test_aggregate_all_functions2.out
@@ -118,3 +118,75 @@
 -- !select_minmax4 --
 243
 
+-- !select_histogram_k0 --
+{"num_buckets":2,"buckets":[{"lower":"0","upper":"0","ndv":1,"count":7,"pre_sum":0},{"lower":"1","upper":"1","ndv":1,"count":8,"pre_sum":7}]}
+
+-- !select_histogram_k1 --
+{"num_buckets":1,"buckets":[{"lower":"1","upper":"15","ndv":15,"count":15,"pre_sum":0}]}
+
+-- !select_histogram_k2 --
+{"num_buckets":2,"buckets":[{"lower":"-32767","upper":"1985","ndv":9,"count":11,"pre_sum":0},{"lower":"1986","upper":"32767","ndv":5,"count":10,"pre_sum":11}]}
+
+-- !select_histogram_k3 --
+{"num_buckets":3,"buckets":[{"lower":"-2147483647","upper":"1001","ndv":3,"count":5,"pre_sum":0},{"lower":"1002","upper":"3021","ndv":2,"count":5,"pre_sum":5},{"lower":"5014","upper":"2147483647","ndv":3,"count":5,"pre_sum":10}]}
+
+-- !select_histogram_k4 --
+{"num_buckets":4,"buckets":[{"lower":"-9223372036854775807","upper":"123456","ndv":4,"count":5,"pre_sum":0},{"lower":"7210457","upper":"11011903","ndv":3,"count":5,"pre_sum":5},{"lower":"11011905","upper":"11011920","ndv":2,"count":3,"pre_sum":10},{"lower":"9223372036854775807","upper":"9223372036854775807","ndv":1,"count":2,"pre_sum":13}]}
+
+-- !select_histogram_k5 --
+{"num_buckets":5,"buckets":[{"lower":"-654.654","upper":"-0.123","ndv":3,"count":3,"pre_sum":0},{"lower":"0.000","upper":"0.666","ndv":2,"count":3,"pre_sum":3},{"lower":"3.141","upper":"123.123","ndv":3,"count":3,"pre_sum":6},{"lower":"243.325","upper":"1243.500","ndv":2,"count":3,"pre_sum":9},{"lower":"24453.325","upper":"604587.000","ndv":3,"count":3,"pre_sum":12}]}
+
+-- !select_histogram_k6 --
+{"num_buckets":2,"buckets":[{"lower":"false","upper":"false","ndv":1,"count":8,"pre_sum":0},{"lower":"true","upper":"true","ndv":1,"count":7,"pre_sum":8}]}
+
+-- !select_histogram_k7 --
+{"num_buckets":7,"buckets":[{"lower":"","upper":"du3lnvl","ndv":3,"count":3,"pre_sum":0},{"lower":"jiw3n4","upper":"lifsno","ndv":2,"count":2,"pre_sum":3},{"lower":"wangjuoo4","upper":"wangjuoo5","ndv":2,"count":3,"pre_sum":5},{"lower":"wangynnsf","upper":"wenlsfnl","ndv":2,"count":3,"pre_sum":8},{"lower":"yanavnd","upper":"yanavnd","ndv":1,"count":1,"pre_sum":11},{"lower":"yanvjldjlll","upper":"yanvjldjlll","ndv":1,"count":1,"pre_sum":12},{"lower":"yunlj8@nk","upper":"yunlj8@nk","ndv":1,"count":2,"pre_sum":13}]}
+
+-- !select_histogram_k10 --
+{"num_buckets":10,"buckets":[{"lower":"1901-12-31","upper":"1901-12-31","ndv":1,"count":1,"pre_sum":0},{"lower":"1988-03-21","upper":"1988-03-21","ndv":1,"count":1,"pre_sum":1},{"lower":"1989-03-21","upper":"1989-03-21","ndv":1,"count":2,"pre_sum":2},{"lower":"1991-08-11","upper":"1991-08-11","ndv":1,"count":2,"pre_sum":4},{"lower":"2012-03-14","upper":"2012-03-14","ndv":1,"count":1,"pre_sum":6},{"lower":"2014-11-11","upper":"2014-11-11","ndv":1,"count":1,"pre_sum":7},{"lower":"2015-01-01","upper":"2015-01-01","ndv":1,"count":1,"pre_sum":8},{"lower":"2015-04-02","upper":"2015-04-02","ndv":1,"count":4,"pre_sum":9},{"lower":"3124-10-10","upper":"3124-10-10","ndv":1,"count":1,"pre_sum":13},{"lower":"9999-12-12","upper":"9999-12-12","ndv":1,"count":1,"pre_sum":14}]}
+
+-- !select_histogram_k11 --
+{"num_buckets":9,"buckets":[{"lower":"1901-01-01 00:00:00","upper":"1901-01-01 00:00:00","ndv":1,"count":1,"pre_sum":0},{"lower":"1989-03-21 13:00:00","upper":"1989-03-21 13:00:00","ndv":1,"count":2,"pre_sum":1},{"lower":"1989-03-21 13:11:00","upper":"1989-03-21 13:11:00","ndv":1,"count":2,"pre_sum":3},{"lower":"2000-01-01 00:00:00","upper":"2000-01-01 00:00:00","ndv":1,"count":1,"pre_sum":5},{"lower":"2013-04-02 15:16:52","upper":"2013-04-02 15:16:52","ndv":1,"count":2,"pre_sum":6},{"lower":"2015-03-13 10:30:00","upper":"2015-03-13 10:30:00","ndv":1,"count":1,"pre_sum":8},{"lower":"2015-03-13 12:36:38","upper":"2015-03-13 12:36:38","ndv":1,"count":2,"pre_sum":9},{"lower":"2015-04-02 00:00:00","upper":"2015-04-02 00:00:00","ndv":1,"count":3,"pre_sum":11},{"lower":"9999-11-11 12:12:00","upper":"9999-11-11 12:12:00","ndv":1,"count":1,"pre_sum":14}]}
+
+-- !select_histogram_k12 --
+{"num_buckets":1,"buckets":[{"lower":"string12345","upper":"string12345","ndv":1,"count":15,"pre_sum":0}]}
+
+-- !select_histogram_k13 --
+{"num_buckets":13,"buckets":[{"lower":"-170141183460469231731687303715884105727","upper":"-20220101","ndv":2,"count":2,"pre_sum":0},{"lower":"-11011903","upper":"-2022","ndv":2,"count":2,"pre_sum":2},{"lower":"0","upper":"0","ndv":1,"count":1,"pre_sum":4},{"lower":"11011903","upper":"11011903","ndv":1,"count":1,"pre_sum":5},{"lower":"20220101","upper":"20220101","ndv":1,"count":1,"pre_sum":6},{"lower":"20220102","upper":"20220102","ndv":1,"count":1,"pre_sum":7},{"lower":"20220104","upper":"20220104","ndv":1,"count":1,"pre_sum":8},{"lower":"701411834604692317","upper":"701411834604692317","ndv":1,"count":1,"pre_sum":9},{"lower":"701411834604692317316873","upper":"701411834604692317316873","ndv":1,"count":1,"pre_sum":10},{"lower":"1701604692317316873037158","upper":"1701604692317316873037158","ndv":1,"count":1,"pre_sum":11},{"lower":"701411834604692317316873037158","upper":"701411834604692317316873037158","ndv":1,"count":1,"pre_sum":12},{"lower":"1701411834604692317316873037158","upper":"1701411834604692317316873037158","ndv":1,"count":1,"pre_sum":13},{"lower":"170141183460469231731687303715884105727","upper":"170141183460469231731687303715884105727","ndv":1,"count":1,"pre_sum":14}]}
+
+-- !select_histogram_k0_all_null --
+{"num_buckets":0,"buckets":[]}
+
+-- !select_histogram_k1_all_null --
+{"num_buckets":0,"buckets":[]}
+
+-- !select_histogram_k2 --
+{"num_buckets":2,"buckets":[{"lower":"10","upper":"12","ndv":3,"count":3,"pre_sum":0},{"lower":"13","upper":"15","ndv":3,"count":3,"pre_sum":3}]}
+
+-- !select_histogram_k3_all_null --
+{"num_buckets":0,"buckets":[]}
+
+-- !select_histogram_k4_all_null --
+{"num_buckets":0,"buckets":[]}
+
+-- !select_histogram_k5_all_null --
+{"num_buckets":0,"buckets":[]}
+
+-- !select_histogram_k6_all_null --
+{"num_buckets":0,"buckets":[]}
+
+-- !select_histogram_k7_all_null --
+{"num_buckets":0,"buckets":[]}
+
+-- !select_histogram_k10_all_null --
+{"num_buckets":0,"buckets":[]}
+
+-- !select_histogram_k11_all_null --
+{"num_buckets":0,"buckets":[]}
+
+-- !select_histogram_k12_all_null --
+{"num_buckets":0,"buckets":[]}
+
+-- !select_histogram_k13_all_null --
+{"num_buckets":0,"buckets":[]}
+
diff --git a/...st/suites/query_p0/sql_functions/aggregate_functions/test_aggregate_all_functions2.groovy b/...st/suites/query_p0/sql_functions/aggregate_functions/test_aggregate_all_functions2.groovy
@@ -113,4 +113,99 @@ suite("test_aggregate_all_functions2") {
     qt_select_minmax2 """ select max_by(datekey,hour) from metric_table; """
     qt_select_minmax3 """ select bitmap_to_string(max_by(device_id,hour)) from metric_table; """
     qt_select_minmax4 """ select bitmap_to_string(min_by(device_id,hour)) from metric_table; """
+
+    sql """
+    INSERT INTO baseall values
+            (NULL, NULL, 10, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL)
+    """
+
+    sql """
+    INSERT INTO baseall values
+            (NULL, NULL, 11, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL)
+    """
+
+    sql """
+    INSERT INTO baseall values
+            (NULL, NULL, 12, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL)
+    """
+
+    sql """
+    INSERT INTO baseall values
+            (NULL, NULL, 13, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL)
+    """
+
+    sql """
+    INSERT INTO baseall values
+            (NULL, NULL, 14, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL)
+    """
+
+    sql """
+    INSERT INTO baseall values
+            (NULL, NULL, 15, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL)
+    """
+
+    qt_select_histogram_k0 """SELECT histogram(k0) FROM baseall"""
+    qt_select_histogram_k1 """SELECT histogram(k1, 1) FROM baseall"""
+    qt_select_histogram_k2 """SELECT histogram(k2, 2) FROM baseall"""
+    qt_select_histogram_k3 """SELECT histogram(k3, 3) FROM baseall"""
+    qt_select_histogram_k4 """SELECT histogram(k4, 4) FROM baseall"""
+    qt_select_histogram_k5 """SELECT histogram(k5, 5) FROM baseall"""
+    qt_select_histogram_k6 """SELECT histogram(k6, 6) FROM baseall"""
+    qt_select_histogram_k7 """SELECT histogram(k7, 7) FROM baseall"""
+    // the test case for double and float is removed, becase the result is not stable since we have
+    // 0 and -0 in column k8, both of them are valid but we can not make both of them stand in out file.
+//     qt_select_histogram_k8 """SELECT histogram(k8, 8) FROM baseall"""
+//     qt_select_histogram_k9 """SELECT histogram(k9, 9) FROM baseall"""
+    qt_select_histogram_k10 """SELECT histogram(k10, 10) FROM baseall"""
+    qt_select_histogram_k11 """SELECT histogram(k11, 11) FROM baseall"""
+    qt_select_histogram_k12 """SELECT histogram(k12, 12) FROM baseall"""
+    qt_select_histogram_k13 """SELECT histogram(k13, 13) FROM baseall"""
+
+    sql """
+    TRUNCATE TABLE baseall;
+    """
+
+    sql """
+    INSERT INTO baseall values
+            (NULL, NULL, 10, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL)
+    """
+
+    sql """
+    INSERT INTO baseall values
+            (NULL, NULL, 11, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL)
+    """
+
+    sql """
+    INSERT INTO baseall values
+            (NULL, NULL, 12, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL)
+    """
+
+    sql """
+    INSERT INTO baseall values
+            (NULL, NULL, 13, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL)
+    """
+
+    sql """
+    INSERT INTO baseall values
+            (NULL, NULL, 14, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL)
+    """
+
+    sql """
+    INSERT INTO baseall values
+            (NULL, NULL, 15, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL)
+    """
+    qt_select_histogram_k0_all_null """SELECT histogram(k0) FROM baseall"""
+    qt_select_histogram_k1_all_null """SELECT histogram(k1, 1) FROM baseall"""
+    qt_select_histogram_k2 """SELECT histogram(k2, 2) FROM baseall"""
+    qt_select_histogram_k3_all_null """SELECT histogram(k3, 3) FROM baseall"""
+    qt_select_histogram_k4_all_null """SELECT histogram(k4, 4) FROM baseall"""
+    qt_select_histogram_k5_all_null """SELECT histogram(k5, 5) FROM baseall"""
+    qt_select_histogram_k6_all_null """SELECT histogram(k6, 6) FROM baseall"""
+    qt_select_histogram_k7_all_null """SELECT histogram(k7, 7) FROM baseall"""
+//     qt_select_histogram_k8_all_null """SELECT histogram(k8, 8) FROM baseall"""
+//     qt_select_histogram_k9_all_null """SELECT histogram(k9, 9) FROM baseall"""
+    qt_select_histogram_k10_all_null """SELECT histogram(k10, 10) FROM baseall"""
+    qt_select_histogram_k11_all_null """SELECT histogram(k11, 11) FROM baseall"""
+    qt_select_histogram_k12_all_null """SELECT histogram(k12, 12) FROM baseall"""
+    qt_select_histogram_k13_all_null """SELECT histogram(k13, 13) FROM baseall"""
 }