From 0188538b337715ce810d89fb63b22ce3ea33e3e7 Mon Sep 17 00:00:00 2001
From: Mikhail Surin <surinmike@gmail.com>
Date: Fri, 22 Jun 2018 17:56:53 +0300
Subject: [PATCH] fix review

---
 .../AggregateFunctionHistogram.cpp            | 33 ++++++++++++-------
 .../AggregateFunctionHistogram.h              | 10 +++---
 2 files changed, 26 insertions(+), 17 deletions(-)

diff --git a/dbms/src/AggregateFunctions/AggregateFunctionHistogram.cpp b/dbms/src/AggregateFunctions/AggregateFunctionHistogram.cpp
index 16d120fc68f..c60359ffc9a 100644
--- a/dbms/src/AggregateFunctions/AggregateFunctionHistogram.cpp
+++ b/dbms/src/AggregateFunctions/AggregateFunctionHistogram.cpp
@@ -7,27 +7,36 @@
 
 namespace DB {
 
+namespace ErrorCodes
+{
+    extern const int NUMBER_OF_ARGUMENTS_DOESNT_MATCH;
+    extern const int ILLEGAL_TYPE_OF_ARGUMENT;
+    extern const int BAD_ARGUMENTS;
+}
+
+namespace {
+
 AggregateFunctionPtr createAggregateFunctionHistogram(const std::string & name, const DataTypes & arguments, const Array & params)
 {
     if (params.size() != 1)
-    {
-        throw Exception("Function " + name + " requires only bins count");
-    }
+        throw Exception("Function " + name + " requires bins count", ErrorCodes::NUMBER_OF_ARGUMENTS_DOESNT_MATCH);
+
+    UInt32 bins_count = applyVisitor(FieldVisitorConvertToNumber<UInt32>(), params[0]);
+
+    if (bins_count == 0)
+        throw Exception("Bin count should be positive", ErrorCodes::BAD_ARGUMENTS);
+
     assertUnary(name, arguments);
-
-    UInt32 bins_count;
-
-#define READ(VAL, PARAM) \
-    VAL = applyVisitor(FieldVisitorConvertToNumber<decltype(VAL)>(), PARAM);
-
-    READ(bins_count, params[0]);
-#undef READ
-
     AggregateFunctionPtr res(createWithNumericType<AggregateFunctionHistogram>(*arguments[0], bins_count));
 
+    if (!res)
+        throw Exception("Illegal type " + arguments[0]->getName() + " of argument for aggregate function " + name, ErrorCodes::ILLEGAL_TYPE_OF_ARGUMENT);
+
     return res;
 }
 
+}
+
 void registerAggregateFunctionHistogram(AggregateFunctionFactory & factory)
 {
     factory.registerFunction("histogram", createAggregateFunctionHistogram);
diff --git a/dbms/src/AggregateFunctions/AggregateFunctionHistogram.h b/dbms/src/AggregateFunctions/AggregateFunctionHistogram.h
index 7b804ff0453..a84fa4f0663 100644
--- a/dbms/src/AggregateFunctions/AggregateFunctionHistogram.h
+++ b/dbms/src/AggregateFunctions/AggregateFunctionHistogram.h
@@ -86,11 +86,11 @@ private:
 
         // Maintain doubly-linked list of "active" points
         // and store neighbour pairs in priority queue by distance
-        AutoArray<int> previous(size + 1);
-        AutoArray<int> next(size + 1);
+        AutoArray<UInt32> previous(size + 1);
+        AutoArray<UInt32> next(size + 1);
         AutoArray<bool> active(size + 1, true);
         active[size] = false;
-        auto delete_node = [&](int i)
+        auto delete_node = [&](UInt32 i)
         {
             previous[next[i]] = previous[i];
             next[previous[i]] = next[i];
@@ -104,13 +104,13 @@ private:
         next[size] = 0;
         previous[0] = size;
 
-        using QueueItem = std::pair<Mean, int>;
+        using QueueItem = std::pair<Mean, UInt32>;
 
         std::vector<QueueItem> storage;
         storage.reserve(2 * size - max_bins);
         std::priority_queue<QueueItem, std::vector<QueueItem>, std::greater<QueueItem>> queue;
 
-        auto quality = [&](int i) { return points[next[i]].mean - points[i].mean; };
+        auto quality = [&](UInt32 i) { return points[next[i]].mean - points[i].mean; };
         for (size_t i = 0; i + 1 < size; i++)
             queue.push({quality(i), i});