Merge pull request ClickHouse#34243 from azat/use_skip_indexes_if_final

Disable data skipping indexes by default for queries with FINAL

src/Core/Settings.h

@@ -170,6 +170,7 @@ class IColumn;
     M(Bool, force_index_by_date, false, "Throw an exception if there is a partition key in a table, and it is not used.", 0) \
     M(Bool, force_primary_key, false, "Throw an exception if there is primary key in a table, and it is not used.", 0) \
     M(Bool, use_skip_indexes, true, "Use data skipping indexes during query execution.", 0) \
+    M(Bool, use_skip_indexes_if_final, false, "If query has FINAL, then skipping data based on indexes may produce incorrect result, hence disabled by default.", 0) \
     M(String, force_data_skipping_indices, "", "Comma separated list of strings or literals with the name of the data skipping indices that should be used during query execution, otherwise an exception will be thrown.", 0) \
     \
     M(Float, max_streams_to_max_threads_ratio, 1, "Allows you to use more sources than the number of threads - to more evenly distribute work across threads. It is assumed that this is a temporary solution, since it will be possible in the future to make the number of sources equal to the number of threads, but for each source to dynamically select available work for itself.", 0) \

src/Processors/QueryPlan/ReadFromMergeTree.cpp

@@ -912,6 +912,11 @@ MergeTreeDataSelectAnalysisResultPtr ReadFromMergeTree::selectRangesToRead(
         parts_before_pk = parts.size();
 
         auto reader_settings = getMergeTreeReaderSettings(context);
+
+        bool use_skip_indexes = context->getSettings().use_skip_indexes;
+        if (select.final() && !context->getSettings().use_skip_indexes_if_final)
+            use_skip_indexes = false;
+
         result.parts_with_ranges = MergeTreeDataSelectExecutor::filterPartsByPrimaryKeyAndSkipIndexes(
             std::move(parts),
             metadata_snapshot,
@@ -922,7 +927,7 @@ MergeTreeDataSelectAnalysisResultPtr ReadFromMergeTree::selectRangesToRead(
             log,
             num_streams,
             result.index_stats,
-            context->getSettings().use_skip_indexes);
+            use_skip_indexes);
     }
     catch (...)
     {

tests/queries/0_stateless/02200_use_skip_indexes.reference

@@ -0,0 +1,4 @@
+-- { echoOn }
+SELECT * FROM data_02200 WHERE value = 1 SETTINGS use_skip_indexes=1, max_rows_to_read=1;
+1	1
+SELECT * FROM data_02200 WHERE value = 1 SETTINGS use_skip_indexes=0, max_rows_to_read=1; -- { serverError TOO_MANY_ROWS }

tests/queries/0_stateless/02200_use_skip_indexes.sql

@@ -0,0 +1,14 @@
+CREATE TABLE data_02200 (
+    key Int,
+    value Int,
+    INDEX idx value TYPE minmax GRANULARITY 1
+)
+Engine=MergeTree()
+ORDER BY key
+PARTITION BY key;
+
+INSERT INTO data_02200 SELECT number, number FROM numbers(10);
+
+-- { echoOn }
+SELECT * FROM data_02200 WHERE value = 1 SETTINGS use_skip_indexes=1, max_rows_to_read=1;
+SELECT * FROM data_02200 WHERE value = 1 SETTINGS use_skip_indexes=0, max_rows_to_read=1; -- { serverError TOO_MANY_ROWS }

tests/queries/0_stateless/02201_use_skip_indexes_if_final.reference

@@ -0,0 +1,6 @@
+-- { echoOn }
+SELECT * FROM data_02201 FINAL WHERE value = 1 SETTINGS use_skip_indexes=0, use_skip_indexes_if_final=0, max_rows_to_read=1; -- { serverError TOO_MANY_ROWS }
+SELECT * FROM data_02201 FINAL WHERE value = 1 SETTINGS use_skip_indexes=1, use_skip_indexes_if_final=0, max_rows_to_read=1; -- { serverError TOO_MANY_ROWS }
+SELECT * FROM data_02201 FINAL WHERE value = 1 SETTINGS use_skip_indexes=0, use_skip_indexes_if_final=1, max_rows_to_read=1; -- { serverError TOO_MANY_ROWS }
+SELECT * FROM data_02201 FINAL WHERE value = 1 SETTINGS use_skip_indexes=1, use_skip_indexes_if_final=1, max_rows_to_read=1;
+1	1

tests/queries/0_stateless/02201_use_skip_indexes_if_final.sql

@@ -0,0 +1,16 @@
+CREATE TABLE data_02201 (
+    key Int,
+    value Int,
+    INDEX idx value TYPE minmax GRANULARITY 1
+)
+Engine=AggregatingMergeTree()
+ORDER BY key
+PARTITION BY key;
+
+INSERT INTO data_02201 SELECT number, number FROM numbers(10);
+
+-- { echoOn }
+SELECT * FROM data_02201 FINAL WHERE value = 1 SETTINGS use_skip_indexes=0, use_skip_indexes_if_final=0, max_rows_to_read=1; -- { serverError TOO_MANY_ROWS }
+SELECT * FROM data_02201 FINAL WHERE value = 1 SETTINGS use_skip_indexes=1, use_skip_indexes_if_final=0, max_rows_to_read=1; -- { serverError TOO_MANY_ROWS }
+SELECT * FROM data_02201 FINAL WHERE value = 1 SETTINGS use_skip_indexes=0, use_skip_indexes_if_final=1, max_rows_to_read=1; -- { serverError TOO_MANY_ROWS }
+SELECT * FROM data_02201 FINAL WHERE value = 1 SETTINGS use_skip_indexes=1, use_skip_indexes_if_final=1, max_rows_to_read=1;

tests/queries/0_stateless/02202_use_skip_indexes_if_final.reference

@@ -0,0 +1,6 @@
+-- { echoOn }
+SELECT * FROM data_02201 FINAL WHERE value_max = 1 ORDER BY key, value_max SETTINGS use_skip_indexes=1, use_skip_indexes_if_final=0;
+0	1
+SELECT * FROM data_02201 FINAL WHERE value_max = 1 ORDER BY key, value_max SETTINGS use_skip_indexes=1, use_skip_indexes_if_final=1;
+0	1
+1	1

tests/queries/0_stateless/02202_use_skip_indexes_if_final.sql

@@ -0,0 +1,19 @@
+-- This tests will show the difference in data with use_skip_indexes_if_final and w/o
+
+CREATE TABLE data_02201 (
+    key Int,
+    value_max SimpleAggregateFunction(max, Int),
+    INDEX idx value_max TYPE minmax GRANULARITY 1
+)
+Engine=AggregatingMergeTree()
+ORDER BY key
+PARTITION BY key;
+
+SYSTEM STOP MERGES data_02201;
+
+INSERT INTO data_02201 SELECT number, number FROM numbers(10);
+INSERT INTO data_02201 SELECT number, number+1 FROM numbers(10);
+
+-- { echoOn }
+SELECT * FROM data_02201 FINAL WHERE value_max = 1 ORDER BY key, value_max SETTINGS use_skip_indexes=1, use_skip_indexes_if_final=0;
+SELECT * FROM data_02201 FINAL WHERE value_max = 1 ORDER BY key, value_max SETTINGS use_skip_indexes=1, use_skip_indexes_if_final=1;