From 6310e490320788cc1471f01f66568155946c15f9 Mon Sep 17 00:00:00 2001 From: Azat Khuzhin Date: Mon, 6 Jul 2020 22:34:00 +0300 Subject: [PATCH 1/3] Fix dictGet with bad arguments during GROUP BY injective functions elimination --- src/Interpreters/SyntaxAnalyzer.cpp | 14 +++++++++++--- ...ive_elimination_dictGet_BAD_ARGUMENTS.reference | 0 ...injective_elimination_dictGet_BAD_ARGUMENTS.sql | 1 + 3 files changed, 12 insertions(+), 3 deletions(-) create mode 100644 tests/queries/0_stateless/01375_GROUP_BY_injective_elimination_dictGet_BAD_ARGUMENTS.reference create mode 100644 tests/queries/0_stateless/01375_GROUP_BY_injective_elimination_dictGet_BAD_ARGUMENTS.sql diff --git a/src/Interpreters/SyntaxAnalyzer.cpp b/src/Interpreters/SyntaxAnalyzer.cpp index 9bc7ae055d2..e90b6da3dba 100644 --- a/src/Interpreters/SyntaxAnalyzer.cpp +++ b/src/Interpreters/SyntaxAnalyzer.cpp @@ -327,10 +327,18 @@ void optimizeGroupBy(ASTSelectQuery * select_query, const NameSet & source_colum continue; } - const auto & dict_name = function->arguments->children[0]->as().value.safeGet(); - const auto & dict_ptr = context.getExternalDictionariesLoader().getDictionary(dict_name); - const auto & attr_name = function->arguments->children[1]->as().value.safeGet(); + const auto * dict_name_ast = function->arguments->children[0]->as(); + const auto * attr_name_ast = function->arguments->children[1]->as(); + if (!dict_name_ast || !attr_name_ast) + { + ++i; + continue; + } + const auto & dict_name = dict_name_ast->value.safeGet(); + const auto & attr_name = attr_name_ast->value.safeGet(); + + const auto & dict_ptr = context.getExternalDictionariesLoader().getDictionary(dict_name); if (!dict_ptr->isInjective(attr_name)) { ++i; diff --git a/tests/queries/0_stateless/01375_GROUP_BY_injective_elimination_dictGet_BAD_ARGUMENTS.reference b/tests/queries/0_stateless/01375_GROUP_BY_injective_elimination_dictGet_BAD_ARGUMENTS.reference new file mode 100644 index 00000000000..e69de29bb2d diff --git a/tests/queries/0_stateless/01375_GROUP_BY_injective_elimination_dictGet_BAD_ARGUMENTS.sql b/tests/queries/0_stateless/01375_GROUP_BY_injective_elimination_dictGet_BAD_ARGUMENTS.sql new file mode 100644 index 00000000000..88a2b25c2db --- /dev/null +++ b/tests/queries/0_stateless/01375_GROUP_BY_injective_elimination_dictGet_BAD_ARGUMENTS.sql @@ -0,0 +1 @@ +SELECT dictGetString(concat('default', '.countryId'), 'country', toUInt64(number)) AS country FROM numbers(2) GROUP BY country; -- { serverError 36; } From 128dd4fa8a0ce5c741caaffc7eb18dda2c2e9034 Mon Sep 17 00:00:00 2001 From: Azat Khuzhin Date: Mon, 6 Jul 2020 22:36:16 +0300 Subject: [PATCH 2/3] Fix dictGet arguments check during GROUP BY injective functions elimination This patch changes the place where the dictionary will be loaded (during syntax analysis), but I guess this is fine, it will be loaded anyway. Fixes: #10342 --- src/Interpreters/SyntaxAnalyzer.cpp | 1 + ...BY_injective_elimination_dictGet.reference | 1 + ...GROUP_BY_injective_elimination_dictGet.sql | 31 +++++++++++++++++++ 3 files changed, 33 insertions(+) create mode 100644 tests/queries/0_stateless/01376_GROUP_BY_injective_elimination_dictGet.reference create mode 100644 tests/queries/0_stateless/01376_GROUP_BY_injective_elimination_dictGet.sql diff --git a/src/Interpreters/SyntaxAnalyzer.cpp b/src/Interpreters/SyntaxAnalyzer.cpp index e90b6da3dba..2e270222c37 100644 --- a/src/Interpreters/SyntaxAnalyzer.cpp +++ b/src/Interpreters/SyntaxAnalyzer.cpp @@ -248,6 +248,7 @@ void executeScalarSubqueries(ASTPtr & query, const Context & context, size_t sub const std::unordered_set possibly_injective_function_names { + "dictGet", "dictGetString", "dictGetUInt8", "dictGetUInt16", diff --git a/tests/queries/0_stateless/01376_GROUP_BY_injective_elimination_dictGet.reference b/tests/queries/0_stateless/01376_GROUP_BY_injective_elimination_dictGet.reference new file mode 100644 index 00000000000..9459d4ba2a0 --- /dev/null +++ b/tests/queries/0_stateless/01376_GROUP_BY_injective_elimination_dictGet.reference @@ -0,0 +1 @@ +1.1 diff --git a/tests/queries/0_stateless/01376_GROUP_BY_injective_elimination_dictGet.sql b/tests/queries/0_stateless/01376_GROUP_BY_injective_elimination_dictGet.sql new file mode 100644 index 00000000000..1c7a4d16f05 --- /dev/null +++ b/tests/queries/0_stateless/01376_GROUP_BY_injective_elimination_dictGet.sql @@ -0,0 +1,31 @@ +-- https://github.com/ClickHouse/ClickHouse/issues/11469 +SELECT dictGet('default.countryId', 'country', toUInt64(number)) AS country FROM numbers(2) GROUP BY country; -- { serverError 36; } + + +-- with real dictionary +DROP TABLE IF EXISTS dictdb_01376.table_for_dict; +DROP DICTIONARY IF EXISTS dictdb_01376.dict_exists; +DROP DATABASE IF EXISTS dictdb_01376; + +CREATE DATABASE dictdb_01376 ENGINE = Ordinary; + +CREATE TABLE dictdb_01376.table_for_dict +( + key_column UInt64, + value Float64 +) +ENGINE = Memory(); + +INSERT INTO dictdb_01376.table_for_dict VALUES (1, 1.1); + +CREATE DICTIONARY IF NOT EXISTS dictdb_01376.dict_exists +( + key_column UInt64, + value Float64 DEFAULT 77.77 +) +PRIMARY KEY key_column +SOURCE(CLICKHOUSE(HOST 'localhost' PORT 9000 USER 'default' TABLE 'table_for_dict' DB 'dictdb_01376')) +LIFETIME(1) +LAYOUT(FLAT()); + +SELECT dictGet('dictdb_01376.dict_exists', 'value', toUInt64(1)) as val FROM numbers(2) GROUP BY val; From 6a04de61b65d423f741a2f13336ff778afae73fc Mon Sep 17 00:00:00 2001 From: Azat Khuzhin Date: Wed, 8 Jul 2020 00:26:09 +0300 Subject: [PATCH 3/3] Allow isInjective() with empty block (is function injective with any arguments) Since most of the time function will ignore it anyway, and creating arguments just for checking is function injective or not is overkill --- src/Functions/FunctionsExternalDictionaries.h | 4 ++++ src/Functions/IFunction.h | 4 ++++ 2 files changed, 8 insertions(+) diff --git a/src/Functions/FunctionsExternalDictionaries.h b/src/Functions/FunctionsExternalDictionaries.h index 425dcf8eec0..7f3979f2141 100644 --- a/src/Functions/FunctionsExternalDictionaries.h +++ b/src/Functions/FunctionsExternalDictionaries.h @@ -99,6 +99,10 @@ public: bool isDictGetFunctionInjective(const Block & sample_block) { + /// Assume non-injective by default + if (!sample_block) + return false; + if (sample_block.columns() != 3 && sample_block.columns() != 4) throw Exception{"Function dictGet... takes 3 or 4 arguments", ErrorCodes::NUMBER_OF_ARGUMENTS_DOESNT_MATCH}; diff --git a/src/Functions/IFunction.h b/src/Functions/IFunction.h index b8873ea2671..9bed7b209bf 100644 --- a/src/Functions/IFunction.h +++ b/src/Functions/IFunction.h @@ -133,6 +133,10 @@ public: * But we assume, that it is injective. This could be documented as implementation-specific behaviour. * * sample_block should contain data types of arguments and values of constants, if relevant. + * NOTE: to check is function injective with any arguments, you can pass + * empty block as sample_block (since most of the time function will + * ignore it anyway, and creating arguments just for checking is + * function injective or not is overkill). */ virtual bool isInjective(const Block & /*sample_block*/) const { return false; }