From efc04b29a6930c4584151597f586c96f224d49df Mon Sep 17 00:00:00 2001
From: "Mikhail f. Shiryaev" <mr.felixoid@gmail.com>
Date: Sat, 6 Mar 2021 13:28:21 +0100
Subject: [PATCH] Add check for growing age AND precision in rollup config

---
 .../MergeTree/registerStorageMergeTree.cpp    | 24 +++++++++++++++++--
 .../configs/graphite_rollup.xml               | 21 ++++++++++++++++
 .../test_graphite_merge_tree/test.py          | 18 ++++++++++++++
 3 files changed, 61 insertions(+), 2 deletions(-)

diff --git a/src/Storages/MergeTree/registerStorageMergeTree.cpp b/src/Storages/MergeTree/registerStorageMergeTree.cpp
index d48f85f3e4d..31687abf2f3 100644
--- a/src/Storages/MergeTree/registerStorageMergeTree.cpp
+++ b/src/Storages/MergeTree/registerStorageMergeTree.cpp
@@ -60,6 +60,27 @@ static Names extractColumnNames(const ASTPtr & node)
     }
 }
 
+/** Is used to order Graphite::Retentions by age and precision descending.
+  * Throws exception if not both age and precision are less or greater then another.
+  */
+static bool compareRetentions(const Graphite::Retention & a, const Graphite::Retention & b)
+{
+    if (a.age > b.age && a.precision > b.precision)
+    {
+        return true;
+    }
+    else if (a.age < b.age && a.precision < b.precision)
+    {
+        return false;
+    }
+    String error_msg = "age and precision should only grow up: "
+        + std::to_string(a.age) + ":" + std::to_string(a.precision) + " vs "
+        + std::to_string(b.age) + ":" + std::to_string(b.precision);
+    throw Exception(
+        error_msg,
+        ErrorCodes::BAD_ARGUMENTS);
+}
+
 /** Read the settings for Graphite rollup from config.
   * Example
   *
@@ -157,8 +178,7 @@ appendGraphitePattern(const Poco::Util::AbstractConfiguration & config, const St
 
     /// retention should be in descending order of age.
     if (pattern.type & pattern.TypeRetention) /// TypeRetention or TypeAll
-        std::sort(pattern.retentions.begin(), pattern.retentions.end(),
-            [] (const Graphite::Retention & a, const Graphite::Retention & b) { return a.age > b.age; });
+        std::sort(pattern.retentions.begin(), pattern.retentions.end(), compareRetentions);
 
     patterns.emplace_back(pattern);
 }
diff --git a/tests/integration/test_graphite_merge_tree/configs/graphite_rollup.xml b/tests/integration/test_graphite_merge_tree/configs/graphite_rollup.xml
index 6d1907f3da7..f5206c8827a 100644
--- a/tests/integration/test_graphite_merge_tree/configs/graphite_rollup.xml
+++ b/tests/integration/test_graphite_merge_tree/configs/graphite_rollup.xml
@@ -94,4 +94,25 @@
             </retention>
         </default>
     </graphite_rollup_broken>
+    <graphite_rollup_wrong_age_precision>
+        <path_column_name>metric</path_column_name>
+        <time_column_name>timestamp</time_column_name>
+        <value_column_name>value</value_column_name>
+        <version_column_name>updated</version_column_name>
+        <default>
+            <function>avg</function>
+            <retention>
+                <age>0</age>
+                <precision>60</precision>
+            </retention>
+            <retention>
+                <age>36000</age>
+                <precision>600</precision>
+            </retention>
+            <retention>
+                <age>72000</age>
+                <precision>300</precision>
+            </retention>
+        </default>
+    </graphite_rollup_wrong_age_precision>
 </yandex>
diff --git a/tests/integration/test_graphite_merge_tree/test.py b/tests/integration/test_graphite_merge_tree/test.py
index ee8a18f1ca5..7628211551d 100644
--- a/tests/integration/test_graphite_merge_tree/test.py
+++ b/tests/integration/test_graphite_merge_tree/test.py
@@ -3,6 +3,7 @@ import os.path as p
 import time
 
 import pytest
+from helpers.client import QueryRuntimeException
 from helpers.cluster import ClickHouseCluster
 from helpers.test_tools import TSV
 
@@ -442,3 +443,20 @@ SELECT * FROM test.graphite;
 ''')
 
     assert TSV(result) == TSV(expected)
+
+
+def test_wrong_rollup_config(graphite_table):
+    with pytest.raises(QueryRuntimeException) as exc:
+        q('''
+CREATE TABLE test.graphite_not_created
+    (metric String, value Float64, timestamp UInt32, date Date, updated UInt32)
+    ENGINE = GraphiteMergeTree('graphite_rollup_wrong_age_precision')
+    PARTITION BY toYYYYMM(date)
+    ORDER BY (metric, timestamp)
+    SETTINGS index_granularity=1;
+        ''')
+
+    # The order of retentions is not guaranteed
+    assert ("age and precision should only grow up: " in str(exc.value))
+    assert ("36000:600" in str(exc.value))
+    assert ("72000:300" in str(exc.value))