From a73f1b9ebeb6ff885b14e4ce52fab518663e3f6c Mon Sep 17 00:00:00 2001 From: Nikolai Kochetov Date: Tue, 8 Jun 2021 16:51:17 +0300 Subject: [PATCH 1/6] Fix crash in explain syntax and cross join. --- src/Parsers/ASTIdentifier.cpp | 4 ++-- .../0_stateless/01890_cross_join_explain_crash.reference | 0 tests/queries/0_stateless/01890_cross_join_explain_crash.sql | 2 ++ 3 files changed, 4 insertions(+), 2 deletions(-) create mode 100644 tests/queries/0_stateless/01890_cross_join_explain_crash.reference create mode 100644 tests/queries/0_stateless/01890_cross_join_explain_crash.sql diff --git a/src/Parsers/ASTIdentifier.cpp b/src/Parsers/ASTIdentifier.cpp index 54ccc155dbf..e0f3fdc058e 100644 --- a/src/Parsers/ASTIdentifier.cpp +++ b/src/Parsers/ASTIdentifier.cpp @@ -116,7 +116,7 @@ void ASTIdentifier::formatImplWithoutAlias(const FormatSettings & settings, Form if (i != 0) settings.ostr << '.'; - if (name_parts[i].empty()) + if (name_parts[i].empty() && j < children.size()) children[j++]->formatImpl(settings, state, frame); else format_element(name_parts[i]); @@ -125,7 +125,7 @@ void ASTIdentifier::formatImplWithoutAlias(const FormatSettings & settings, Form else { const auto & name = shortName(); - if (name.empty()) + if (name.empty() && !children.empty()) children.front()->formatImpl(settings, state, frame); else format_element(name); diff --git a/tests/queries/0_stateless/01890_cross_join_explain_crash.reference b/tests/queries/0_stateless/01890_cross_join_explain_crash.reference new file mode 100644 index 00000000000..e69de29bb2d diff --git a/tests/queries/0_stateless/01890_cross_join_explain_crash.sql b/tests/queries/0_stateless/01890_cross_join_explain_crash.sql new file mode 100644 index 00000000000..868956c549d --- /dev/null +++ b/tests/queries/0_stateless/01890_cross_join_explain_crash.sql @@ -0,0 +1,2 @@ +SET joined_subquery_requires_alias = 0; +select * FROM (SELECT 1), (SELECT 1), (SELECT 1); -- { serverError 352 } From 194dd91f0ca826861e0cddd4148fade9453ca842 Mon Sep 17 00:00:00 2001 From: Nikolai Kochetov Date: Tue, 8 Jun 2021 16:54:48 +0300 Subject: [PATCH 2/6] Fix crash in explain syntax and cross join. --- src/Parsers/ASTIdentifier.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/Parsers/ASTIdentifier.cpp b/src/Parsers/ASTIdentifier.cpp index e0f3fdc058e..fbf7ede1b17 100644 --- a/src/Parsers/ASTIdentifier.cpp +++ b/src/Parsers/ASTIdentifier.cpp @@ -116,6 +116,9 @@ void ASTIdentifier::formatImplWithoutAlias(const FormatSettings & settings, Form if (i != 0) settings.ostr << '.'; + /// Some AST revriting code, like IdentifierSemantic::setColumnLongName, + /// does not respect children of identifier. + /// Here we also ingore children if they are empty. if (name_parts[i].empty() && j < children.size()) children[j++]->formatImpl(settings, state, frame); else From 5cc005710ac93c08c5d661a0e8fe65028535ed8e Mon Sep 17 00:00:00 2001 From: Nikolai Kochetov Date: Tue, 8 Jun 2021 18:23:42 +0300 Subject: [PATCH 3/6] Update ASTIdentifier.cpp --- src/Parsers/ASTIdentifier.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Parsers/ASTIdentifier.cpp b/src/Parsers/ASTIdentifier.cpp index fbf7ede1b17..5ebf42ad8e3 100644 --- a/src/Parsers/ASTIdentifier.cpp +++ b/src/Parsers/ASTIdentifier.cpp @@ -118,7 +118,7 @@ void ASTIdentifier::formatImplWithoutAlias(const FormatSettings & settings, Form /// Some AST revriting code, like IdentifierSemantic::setColumnLongName, /// does not respect children of identifier. - /// Here we also ingore children if they are empty. + /// Here we also ignore children if they are empty. if (name_parts[i].empty() && j < children.size()) children[j++]->formatImpl(settings, state, frame); else From 736b56ce3337e6b70d3b5ec8ca971a5531172e3f Mon Sep 17 00:00:00 2001 From: alexey-milovidov Date: Tue, 8 Jun 2021 20:39:42 +0300 Subject: [PATCH 4/6] Update ASTIdentifier.cpp --- src/Parsers/ASTIdentifier.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Parsers/ASTIdentifier.cpp b/src/Parsers/ASTIdentifier.cpp index 5ebf42ad8e3..10b36f950ab 100644 --- a/src/Parsers/ASTIdentifier.cpp +++ b/src/Parsers/ASTIdentifier.cpp @@ -116,7 +116,7 @@ void ASTIdentifier::formatImplWithoutAlias(const FormatSettings & settings, Form if (i != 0) settings.ostr << '.'; - /// Some AST revriting code, like IdentifierSemantic::setColumnLongName, + /// Some AST rewriting code, like IdentifierSemantic::setColumnLongName, /// does not respect children of identifier. /// Here we also ignore children if they are empty. if (name_parts[i].empty() && j < children.size()) From c1d233cabc187205ae59b9891aabe1f42bb979a7 Mon Sep 17 00:00:00 2001 From: alexey-milovidov Date: Tue, 8 Jun 2021 20:40:33 +0300 Subject: [PATCH 5/6] Update ASTIdentifier.cpp --- src/Parsers/ASTIdentifier.cpp | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/Parsers/ASTIdentifier.cpp b/src/Parsers/ASTIdentifier.cpp index 10b36f950ab..02430cb40f7 100644 --- a/src/Parsers/ASTIdentifier.cpp +++ b/src/Parsers/ASTIdentifier.cpp @@ -80,7 +80,7 @@ void ASTIdentifier::setShortName(const String & new_name) name_parts = {new_name}; bool special = semantic->special; - //how about keep the semantic info here, such as table + /// How about keep the semantic info here, such as table auto table = semantic->table; *semantic = IdentifierSemanticImpl(); @@ -120,7 +120,10 @@ void ASTIdentifier::formatImplWithoutAlias(const FormatSettings & settings, Form /// does not respect children of identifier. /// Here we also ignore children if they are empty. if (name_parts[i].empty() && j < children.size()) - children[j++]->formatImpl(settings, state, frame); + { + children[j]->formatImpl(settings, state, frame); + ++j; + } else format_element(name_parts[i]); } From 9e0f3e5e9ee140879bfb2010fd52739d0a7c99d2 Mon Sep 17 00:00:00 2001 From: Nikolai Kochetov Date: Wed, 9 Jun 2021 13:13:13 +0300 Subject: [PATCH 6/6] Avoid using empty table name in identifier. --- .../JoinToSubqueryTransformVisitor.cpp | 15 ++++++++++++++- .../01890_cross_join_explain_crash.reference | 3 +++ .../01890_cross_join_explain_crash.sql | 6 ++++++ 3 files changed, 23 insertions(+), 1 deletion(-) diff --git a/src/Interpreters/JoinToSubqueryTransformVisitor.cpp b/src/Interpreters/JoinToSubqueryTransformVisitor.cpp index 3cb8004a29f..65848e4fbb9 100644 --- a/src/Interpreters/JoinToSubqueryTransformVisitor.cpp +++ b/src/Interpreters/JoinToSubqueryTransformVisitor.cpp @@ -91,7 +91,20 @@ public: { if (should_add_column_predicate(column.name)) { - auto identifier = std::make_shared(std::vector{it->first, column.name}); + ASTPtr identifier; + if (it->first.empty()) + /// We want tables from JOIN to have aliases. + /// But it is possible to set joined_subquery_requires_alias = 0, + /// and write a query like `select * FROM (SELECT 1), (SELECT 1), (SELECT 1)`. + /// If so, table name will be empty here. + /// + /// We cannot create compound identifier with empty part (there is an assert). + /// So, try our luck and use only column name. + /// (Rewriting AST for JOIN is not an efficient design). + identifier = std::make_shared(column.name); + else + identifier = std::make_shared(std::vector{it->first, column.name}); + new_select_expression_list->children.emplace_back(std::move(identifier)); } } diff --git a/tests/queries/0_stateless/01890_cross_join_explain_crash.reference b/tests/queries/0_stateless/01890_cross_join_explain_crash.reference index e69de29bb2d..76315843adb 100644 --- a/tests/queries/0_stateless/01890_cross_join_explain_crash.reference +++ b/tests/queries/0_stateless/01890_cross_join_explain_crash.reference @@ -0,0 +1,3 @@ +2 1 1 +1 2 1 +1 1 2 diff --git a/tests/queries/0_stateless/01890_cross_join_explain_crash.sql b/tests/queries/0_stateless/01890_cross_join_explain_crash.sql index 868956c549d..20a1956ea6b 100644 --- a/tests/queries/0_stateless/01890_cross_join_explain_crash.sql +++ b/tests/queries/0_stateless/01890_cross_join_explain_crash.sql @@ -1,2 +1,8 @@ SET joined_subquery_requires_alias = 0; select * FROM (SELECT 1), (SELECT 1), (SELECT 1); -- { serverError 352 } + +-- This queries work by luck. +-- Feel free to remove then if it is the only failed test. +select * from (select 2), (select 1) as a, (select 1) as b; +select * from (select 1) as a, (select 2), (select 1) as b; +select * from (select 1) as a, (select 1) as b, (select 2);