From 293d2f06fac5d1daeca5e24b5c0c43910195a307 Mon Sep 17 00:00:00 2001 From: hexiaoting Date: Tue, 20 Oct 2020 15:38:56 +0800 Subject: [PATCH 01/10] Fix: throw error when column transformer use non-exsit column --- src/Parsers/ASTColumnsTransformers.cpp | 9 +++++++++ .../0_stateless/01470_columns_transformers.reference | 6 ------ tests/queries/0_stateless/01470_columns_transformers.sql | 4 ++-- 3 files changed, 11 insertions(+), 8 deletions(-) diff --git a/src/Parsers/ASTColumnsTransformers.cpp b/src/Parsers/ASTColumnsTransformers.cpp index 43d54f07ab8..a204a409926 100644 --- a/src/Parsers/ASTColumnsTransformers.cpp +++ b/src/Parsers/ASTColumnsTransformers.cpp @@ -12,6 +12,7 @@ namespace DB namespace ErrorCodes { extern const int ILLEGAL_TYPE_OF_ARGUMENT; + extern const int NO_SUCH_COLUMN_IN_TABLE; } void IASTColumnsTransformer::transform(const ASTPtr & transformer, ASTs & nodes) @@ -130,6 +131,7 @@ void ASTColumnsReplaceTransformer::transform(ASTs & nodes) const replace_map.emplace(replacement.name, replacement.expr); } + UInt8 replace_column_sucess = 0; for (auto & column : nodes) { if (const auto * id = column->as()) @@ -139,6 +141,7 @@ void ASTColumnsReplaceTransformer::transform(ASTs & nodes) const { column = replace_it->second; column->setAlias(replace_it->first); + ++replace_column_sucess; } } else if (auto * ast_with_alias = dynamic_cast(column.get())) @@ -151,9 +154,15 @@ void ASTColumnsReplaceTransformer::transform(ASTs & nodes) const replaceChildren(new_ast, column, replace_it->first); column = new_ast; column->setAlias(replace_it->first); + ++replace_column_sucess; } } } + + if (replace_column_sucess < replace_map.size()) + throw Exception( + "Expressions in columns transformer REPLACE should use same column name as original column", + ErrorCodes::NO_SUCH_COLUMN_IN_TABLE); } } diff --git a/tests/queries/0_stateless/01470_columns_transformers.reference b/tests/queries/0_stateless/01470_columns_transformers.reference index 2d8a1802289..397499f990f 100644 --- a/tests/queries/0_stateless/01470_columns_transformers.reference +++ b/tests/queries/0_stateless/01470_columns_transformers.reference @@ -8,7 +8,6 @@ 1970-04-11 1970-01-11 1970-11-21 222 18 347 111 11 173.5 -1970-04-11 1970-01-11 1970-11-21 SELECT sum(i), sum(j), @@ -51,11 +50,6 @@ SELECT avg(j + 2 AS j), avg(k) FROM columns_transformers -SELECT - toDate(any(i)), - toDate(any(j)), - toDate(any(k)) -FROM columns_transformers AS a SELECT (i + 1) + 1 AS i, j, diff --git a/tests/queries/0_stateless/01470_columns_transformers.sql b/tests/queries/0_stateless/01470_columns_transformers.sql index f95cee51fb0..755978e82c4 100644 --- a/tests/queries/0_stateless/01470_columns_transformers.sql +++ b/tests/queries/0_stateless/01470_columns_transformers.sql @@ -13,11 +13,12 @@ SELECT columns_transformers.* EXCEPT(j) APPLY(avg) from columns_transformers; -- EXCEPT after APPLY will not match anything SELECT a.* APPLY(toDate) EXCEPT(i, j) APPLY(any) from columns_transformers a; +SELECT * REPLACE(i + 1 AS col) from columns_transformers; -- { serverError 16 } SELECT * REPLACE(i + 1 AS i) APPLY(sum) from columns_transformers; SELECT columns_transformers.* REPLACE(j + 2 AS j, i + 1 AS i) APPLY(avg) from columns_transformers; SELECT columns_transformers.* REPLACE(j + 1 AS j, j + 2 AS j) APPLY(avg) from columns_transformers; -- { serverError 43 } -- REPLACE after APPLY will not match anything -SELECT a.* APPLY(toDate) REPLACE(i + 1 AS i) APPLY(any) from columns_transformers a; +SELECT a.* APPLY(toDate) REPLACE(i + 1 AS i) APPLY(any) from columns_transformers a; -- { serverError 16 } EXPLAIN SYNTAX SELECT * APPLY(sum) from columns_transformers; EXPLAIN SYNTAX SELECT columns_transformers.* APPLY(avg) from columns_transformers; @@ -28,7 +29,6 @@ EXPLAIN SYNTAX SELECT columns_transformers.* EXCEPT(j) APPLY(avg) from columns_t EXPLAIN SYNTAX SELECT a.* APPLY(toDate) EXCEPT(i, j) APPLY(any) from columns_transformers a; EXPLAIN SYNTAX SELECT * REPLACE(i + 1 AS i) APPLY(sum) from columns_transformers; EXPLAIN SYNTAX SELECT columns_transformers.* REPLACE(j + 2 AS j, i + 1 AS i) APPLY(avg) from columns_transformers; -EXPLAIN SYNTAX SELECT a.* APPLY(toDate) REPLACE(i + 1 AS i) APPLY(any) from columns_transformers a; -- Multiple REPLACE in a row EXPLAIN SYNTAX SELECT * REPLACE(i + 1 AS i) REPLACE(i + 1 AS i) from columns_transformers; From 91b1dab75b841035e9b43b20f37205bfa82dc705 Mon Sep 17 00:00:00 2001 From: hexiaoting Date: Wed, 21 Oct 2020 15:54:13 +0800 Subject: [PATCH 02/10] Add EXCEPTSTRICT and REPLACESTRICT column transformers --- src/Parsers/ASTColumnsTransformers.cpp | 52 +++++++++++++++---- src/Parsers/ASTColumnsTransformers.h | 2 + src/Parsers/ExpressionElementParsers.cpp | 28 +++++++++- src/Parsers/ExpressionElementParsers.h | 3 ++ .../01470_columns_transformers.reference | 6 +++ .../01470_columns_transformers.sql | 7 ++- 6 files changed, 85 insertions(+), 13 deletions(-) diff --git a/src/Parsers/ASTColumnsTransformers.cpp b/src/Parsers/ASTColumnsTransformers.cpp index a204a409926..3a9ecba9d3f 100644 --- a/src/Parsers/ASTColumnsTransformers.cpp +++ b/src/Parsers/ASTColumnsTransformers.cpp @@ -46,7 +46,9 @@ void ASTColumnsApplyTransformer::transform(ASTs & nodes) const void ASTColumnsExceptTransformer::formatImpl(const FormatSettings & settings, FormatState & state, FormatStateStacked frame) const { - settings.ostr << (settings.hilite ? hilite_keyword : "") << "EXCEPT" << (settings.hilite ? hilite_none : "") << "("; + settings.ostr << (settings.hilite ? hilite_keyword : ""); + settings.ostr << (is_strict ? "EXCEPTSTRICT" : "EXCEPT"); + settings.ostr << (settings.hilite ? hilite_none : "") << "("; for (ASTs::const_iterator it = children.begin(); it != children.end(); ++it) { @@ -62,23 +64,42 @@ void ASTColumnsExceptTransformer::formatImpl(const FormatSettings & settings, Fo void ASTColumnsExceptTransformer::transform(ASTs & nodes) const { + ASTs unexcepted_columns(children); nodes.erase( std::remove_if( nodes.begin(), nodes.end(), - [this](const ASTPtr & node_child) + [&](const ASTPtr & node_child) { if (const auto * id = node_child->as()) { - for (const auto & except_child : children) + for (size_t i = 0; i < children.size(); i++) { - if (except_child->as().name == id->shortName()) + if (children[i]->as().name == id->shortName()) + { + unexcepted_columns.erase(unexcepted_columns.begin() + i); return true; + } } } return false; }), nodes.end()); + + if (is_strict && !unexcepted_columns.empty()) + { + String unexisted_columns; + for (size_t i = 0; i < unexcepted_columns.size(); ++i) + { + if (i > 0) + unexisted_columns += ", "; + unexisted_columns += unexcepted_columns[i]->as().name; + } + + throw Exception( + "Columns transformer EXPCEPTSTRICT process unexist column : " + unexisted_columns, + ErrorCodes::NO_SUCH_COLUMN_IN_TABLE); + } } void ASTColumnsReplaceTransformer::Replacement::formatImpl( @@ -90,7 +111,9 @@ void ASTColumnsReplaceTransformer::Replacement::formatImpl( void ASTColumnsReplaceTransformer::formatImpl(const FormatSettings & settings, FormatState & state, FormatStateStacked frame) const { - settings.ostr << (settings.hilite ? hilite_keyword : "") << "REPLACE" << (settings.hilite ? hilite_none : "") << "("; + settings.ostr << (settings.hilite ? hilite_keyword : ""); + settings.ostr << (is_strict ? "REPLACESTRICT" : "REPLACE"); + settings.ostr << (settings.hilite ? hilite_none : "") << "("; for (ASTs::const_iterator it = children.begin(); it != children.end(); ++it) { @@ -131,7 +154,6 @@ void ASTColumnsReplaceTransformer::transform(ASTs & nodes) const replace_map.emplace(replacement.name, replacement.expr); } - UInt8 replace_column_sucess = 0; for (auto & column : nodes) { if (const auto * id = column->as()) @@ -141,7 +163,7 @@ void ASTColumnsReplaceTransformer::transform(ASTs & nodes) const { column = replace_it->second; column->setAlias(replace_it->first); - ++replace_column_sucess; + replace_map.erase(replace_it); } } else if (auto * ast_with_alias = dynamic_cast(column.get())) @@ -154,15 +176,25 @@ void ASTColumnsReplaceTransformer::transform(ASTs & nodes) const replaceChildren(new_ast, column, replace_it->first); column = new_ast; column->setAlias(replace_it->first); - ++replace_column_sucess; + replace_map.erase(replace_it); } } } - if (replace_column_sucess < replace_map.size()) + if (is_strict && !replace_map.empty()) + { + String unexisted_columns = ""; + for (auto it = replace_map.begin(); it != replace_map.end(); ++it) + { + if (unexisted_columns != "") + unexisted_columns += ", "; + unexisted_columns += it->first; + } throw Exception( - "Expressions in columns transformer REPLACE should use same column name as original column", + "Columns transformer REPLACESTRICT process unexist column : " + unexisted_columns, ErrorCodes::NO_SUCH_COLUMN_IN_TABLE); + } + } } diff --git a/src/Parsers/ASTColumnsTransformers.h b/src/Parsers/ASTColumnsTransformers.h index 4b7a933647e..ac9d9958f9a 100644 --- a/src/Parsers/ASTColumnsTransformers.h +++ b/src/Parsers/ASTColumnsTransformers.h @@ -30,6 +30,7 @@ protected: class ASTColumnsExceptTransformer : public IASTColumnsTransformer { public: + bool is_strict = false; String getID(char) const override { return "ColumnsExceptTransformer"; } ASTPtr clone() const override { @@ -66,6 +67,7 @@ public: void formatImpl(const FormatSettings & settings, FormatState &, FormatStateStacked) const override; }; + bool is_strict = false; String getID(char) const override { return "ColumnsReplaceTransformer"; } ASTPtr clone() const override { diff --git a/src/Parsers/ExpressionElementParsers.cpp b/src/Parsers/ExpressionElementParsers.cpp index 1d861c6d78a..047acc170f8 100644 --- a/src/Parsers/ExpressionElementParsers.cpp +++ b/src/Parsers/ExpressionElementParsers.cpp @@ -1236,6 +1236,10 @@ bool ParserColumnsTransformers::parseImpl(Pos & pos, ASTPtr & node, Expected & e ParserKeyword except("EXCEPT"); ParserKeyword replace("REPLACE"); ParserKeyword as("AS"); + ParserKeyword except_strict("EXCEPTSTRICT"); + ParserKeyword replace_strict("REPLACESTRICT"); + bool is_except = false; + bool is_replace = false; if (apply.ignore(pos, expected)) { @@ -1256,7 +1260,26 @@ bool ParserColumnsTransformers::parseImpl(Pos & pos, ASTPtr & node, Expected & e node = std::move(res); return true; } + else if (except_strict.ignore(pos, expected)) + { + is_except = true; + is_strict = true; + } else if (except.ignore(pos, expected)) + { + is_except = true; + } + else if (replace_strict.ignore(pos, expected)) + { + is_replace = true; + is_strict = true; + } + else if (replace.ignore(pos, expected)) + { + is_replace = true; + } + + if (is_except) { if (pos->type != TokenType::OpeningRoundBracket) return false; @@ -1282,11 +1305,13 @@ bool ParserColumnsTransformers::parseImpl(Pos & pos, ASTPtr & node, Expected & e auto res = std::make_shared(); res->children = std::move(identifiers); + res->is_strict = is_strict; node = std::move(res); return true; } - else if (replace.ignore(pos, expected)) + else if (is_replace) { + if (pos->type != TokenType::OpeningRoundBracket) return false; ++pos; @@ -1323,6 +1348,7 @@ bool ParserColumnsTransformers::parseImpl(Pos & pos, ASTPtr & node, Expected & e auto res = std::make_shared(); res->children = std::move(replacements); + res->is_strict = is_strict; node = std::move(res); return true; } diff --git a/src/Parsers/ExpressionElementParsers.h b/src/Parsers/ExpressionElementParsers.h index 702d757761a..c920e4dc339 100644 --- a/src/Parsers/ExpressionElementParsers.h +++ b/src/Parsers/ExpressionElementParsers.h @@ -92,9 +92,12 @@ protected: */ class ParserColumnsTransformers : public IParserBase { +public: + ParserColumnsTransformers(bool is_strict_ = false): is_strict(is_strict_) {} protected: const char * getName() const override { return "COLUMNS transformers"; } bool parseImpl(Pos & pos, ASTPtr & node, Expected & expected) override; + bool is_strict; }; /** A function, for example, f(x, y + 1, g(z)). diff --git a/tests/queries/0_stateless/01470_columns_transformers.reference b/tests/queries/0_stateless/01470_columns_transformers.reference index 397499f990f..2d8a1802289 100644 --- a/tests/queries/0_stateless/01470_columns_transformers.reference +++ b/tests/queries/0_stateless/01470_columns_transformers.reference @@ -8,6 +8,7 @@ 1970-04-11 1970-01-11 1970-11-21 222 18 347 111 11 173.5 +1970-04-11 1970-01-11 1970-11-21 SELECT sum(i), sum(j), @@ -50,6 +51,11 @@ SELECT avg(j + 2 AS j), avg(k) FROM columns_transformers +SELECT + toDate(any(i)), + toDate(any(j)), + toDate(any(k)) +FROM columns_transformers AS a SELECT (i + 1) + 1 AS i, j, diff --git a/tests/queries/0_stateless/01470_columns_transformers.sql b/tests/queries/0_stateless/01470_columns_transformers.sql index 755978e82c4..5aa68f91453 100644 --- a/tests/queries/0_stateless/01470_columns_transformers.sql +++ b/tests/queries/0_stateless/01470_columns_transformers.sql @@ -13,12 +13,14 @@ SELECT columns_transformers.* EXCEPT(j) APPLY(avg) from columns_transformers; -- EXCEPT after APPLY will not match anything SELECT a.* APPLY(toDate) EXCEPT(i, j) APPLY(any) from columns_transformers a; -SELECT * REPLACE(i + 1 AS col) from columns_transformers; -- { serverError 16 } +SELECT * EXCEPTSTRICT(i, j1) from columns_transformers; -- { serverError 16 } +SELECT * REPLACESTRICT(i + 1 AS col) from columns_transformers; -- { serverError 16 } SELECT * REPLACE(i + 1 AS i) APPLY(sum) from columns_transformers; SELECT columns_transformers.* REPLACE(j + 2 AS j, i + 1 AS i) APPLY(avg) from columns_transformers; SELECT columns_transformers.* REPLACE(j + 1 AS j, j + 2 AS j) APPLY(avg) from columns_transformers; -- { serverError 43 } -- REPLACE after APPLY will not match anything -SELECT a.* APPLY(toDate) REPLACE(i + 1 AS i) APPLY(any) from columns_transformers a; -- { serverError 16 } +SELECT a.* APPLY(toDate) REPLACE(i + 1 AS i) APPLY(any) from columns_transformers a; +SELECT a.* APPLY(toDate) REPLACESTRICT(i + 1 AS i) APPLY(any) from columns_transformers a; -- { serverError 16 } EXPLAIN SYNTAX SELECT * APPLY(sum) from columns_transformers; EXPLAIN SYNTAX SELECT columns_transformers.* APPLY(avg) from columns_transformers; @@ -29,6 +31,7 @@ EXPLAIN SYNTAX SELECT columns_transformers.* EXCEPT(j) APPLY(avg) from columns_t EXPLAIN SYNTAX SELECT a.* APPLY(toDate) EXCEPT(i, j) APPLY(any) from columns_transformers a; EXPLAIN SYNTAX SELECT * REPLACE(i + 1 AS i) APPLY(sum) from columns_transformers; EXPLAIN SYNTAX SELECT columns_transformers.* REPLACE(j + 2 AS j, i + 1 AS i) APPLY(avg) from columns_transformers; +EXPLAIN SYNTAX SELECT a.* APPLY(toDate) REPLACE(i + 1 AS i) APPLY(any) from columns_transformers a; -- Multiple REPLACE in a row EXPLAIN SYNTAX SELECT * REPLACE(i + 1 AS i) REPLACE(i + 1 AS i) from columns_transformers; From ad2d2cf10dda2b36915e78c6b5ef93cf3e56fc66 Mon Sep 17 00:00:00 2001 From: hexiaoting Date: Thu, 22 Oct 2020 12:40:50 +0800 Subject: [PATCH 03/10] Modify varaible name and log info --- src/Parsers/ASTColumnsTransformers.cpp | 37 +++++++++---------- src/Parsers/ExpressionElementParsers.cpp | 30 ++++----------- .../01470_columns_transformers.sql | 6 +-- 3 files changed, 27 insertions(+), 46 deletions(-) diff --git a/src/Parsers/ASTColumnsTransformers.cpp b/src/Parsers/ASTColumnsTransformers.cpp index 3a9ecba9d3f..474c3262d78 100644 --- a/src/Parsers/ASTColumnsTransformers.cpp +++ b/src/Parsers/ASTColumnsTransformers.cpp @@ -46,9 +46,7 @@ void ASTColumnsApplyTransformer::transform(ASTs & nodes) const void ASTColumnsExceptTransformer::formatImpl(const FormatSettings & settings, FormatState & state, FormatStateStacked frame) const { - settings.ostr << (settings.hilite ? hilite_keyword : ""); - settings.ostr << (is_strict ? "EXCEPTSTRICT" : "EXCEPT"); - settings.ostr << (settings.hilite ? hilite_none : "") << "("; + settings.ostr << (settings.hilite ? hilite_keyword : "") << "EXCEPT" << (is_strict ? " STRICT " : "") << (settings.hilite ? hilite_none : "") << "("; for (ASTs::const_iterator it = children.begin(); it != children.end(); ++it) { @@ -64,7 +62,8 @@ void ASTColumnsExceptTransformer::formatImpl(const FormatSettings & settings, Fo void ASTColumnsExceptTransformer::transform(ASTs & nodes) const { - ASTs unexcepted_columns(children); + ASTs expected_columns(children); + nodes.erase( std::remove_if( nodes.begin(), @@ -73,11 +72,11 @@ void ASTColumnsExceptTransformer::transform(ASTs & nodes) const { if (const auto * id = node_child->as()) { - for (size_t i = 0; i < children.size(); i++) + for (int i = children.size() - 1; i >= 0; --i) { if (children[i]->as().name == id->shortName()) { - unexcepted_columns.erase(unexcepted_columns.begin() + i); + expected_columns.erase(expected_columns.begin() + i); return true; } } @@ -86,18 +85,18 @@ void ASTColumnsExceptTransformer::transform(ASTs & nodes) const }), nodes.end()); - if (is_strict && !unexcepted_columns.empty()) + if (is_strict && !expected_columns.empty()) { - String unexisted_columns; - for (size_t i = 0; i < unexcepted_columns.size(); ++i) + String expected_columns_str; + for (size_t i = 0; i < expected_columns.size(); ++i) { if (i > 0) - unexisted_columns += ", "; - unexisted_columns += unexcepted_columns[i]->as().name; + expected_columns_str += ", "; + expected_columns_str += expected_columns[i]->as().name; } throw Exception( - "Columns transformer EXPCEPTSTRICT process unexist column : " + unexisted_columns, + "Columns transformer EXCEPT expects following column(s) : " + expected_columns_str, ErrorCodes::NO_SUCH_COLUMN_IN_TABLE); } } @@ -111,9 +110,7 @@ void ASTColumnsReplaceTransformer::Replacement::formatImpl( void ASTColumnsReplaceTransformer::formatImpl(const FormatSettings & settings, FormatState & state, FormatStateStacked frame) const { - settings.ostr << (settings.hilite ? hilite_keyword : ""); - settings.ostr << (is_strict ? "REPLACESTRICT" : "REPLACE"); - settings.ostr << (settings.hilite ? hilite_none : "") << "("; + settings.ostr << (settings.hilite ? hilite_keyword : "") << "REPLACE" << (is_strict ? " STRICT " : "") << (settings.hilite ? hilite_none : "") << "("; for (ASTs::const_iterator it = children.begin(); it != children.end(); ++it) { @@ -183,15 +180,15 @@ void ASTColumnsReplaceTransformer::transform(ASTs & nodes) const if (is_strict && !replace_map.empty()) { - String unexisted_columns = ""; + String expected_columns = ""; for (auto it = replace_map.begin(); it != replace_map.end(); ++it) { - if (unexisted_columns != "") - unexisted_columns += ", "; - unexisted_columns += it->first; + if (expected_columns != "") + expected_columns += ", "; + expected_columns += it->first; } throw Exception( - "Columns transformer REPLACESTRICT process unexist column : " + unexisted_columns, + "Columns transformer REPLACE expects following column(s) : " + expected_columns, ErrorCodes::NO_SUCH_COLUMN_IN_TABLE); } diff --git a/src/Parsers/ExpressionElementParsers.cpp b/src/Parsers/ExpressionElementParsers.cpp index 047acc170f8..dc8f39ae894 100644 --- a/src/Parsers/ExpressionElementParsers.cpp +++ b/src/Parsers/ExpressionElementParsers.cpp @@ -1236,10 +1236,7 @@ bool ParserColumnsTransformers::parseImpl(Pos & pos, ASTPtr & node, Expected & e ParserKeyword except("EXCEPT"); ParserKeyword replace("REPLACE"); ParserKeyword as("AS"); - ParserKeyword except_strict("EXCEPTSTRICT"); - ParserKeyword replace_strict("REPLACESTRICT"); - bool is_except = false; - bool is_replace = false; + ParserKeyword strict("STRICT"); if (apply.ignore(pos, expected)) { @@ -1260,27 +1257,12 @@ bool ParserColumnsTransformers::parseImpl(Pos & pos, ASTPtr & node, Expected & e node = std::move(res); return true; } - else if (except_strict.ignore(pos, expected)) - { - is_except = true; - is_strict = true; - } else if (except.ignore(pos, expected)) { - is_except = true; - } - else if (replace_strict.ignore(pos, expected)) - { - is_replace = true; - is_strict = true; - } - else if (replace.ignore(pos, expected)) - { - is_replace = true; - } - if (is_except) - { + if (strict.ignore(pos, expected)) + is_strict = true; + if (pos->type != TokenType::OpeningRoundBracket) return false; ++pos; @@ -1309,8 +1291,10 @@ bool ParserColumnsTransformers::parseImpl(Pos & pos, ASTPtr & node, Expected & e node = std::move(res); return true; } - else if (is_replace) + else if (replace.ignore(pos, expected)) { + if (strict.ignore(pos, expected)) + is_strict = true; if (pos->type != TokenType::OpeningRoundBracket) return false; diff --git a/tests/queries/0_stateless/01470_columns_transformers.sql b/tests/queries/0_stateless/01470_columns_transformers.sql index 5aa68f91453..a3d103cd876 100644 --- a/tests/queries/0_stateless/01470_columns_transformers.sql +++ b/tests/queries/0_stateless/01470_columns_transformers.sql @@ -13,14 +13,14 @@ SELECT columns_transformers.* EXCEPT(j) APPLY(avg) from columns_transformers; -- EXCEPT after APPLY will not match anything SELECT a.* APPLY(toDate) EXCEPT(i, j) APPLY(any) from columns_transformers a; -SELECT * EXCEPTSTRICT(i, j1) from columns_transformers; -- { serverError 16 } -SELECT * REPLACESTRICT(i + 1 AS col) from columns_transformers; -- { serverError 16 } +SELECT * EXCEPT STRICT(i, j1) from columns_transformers; -- { serverError 16 } +SELECT * REPLACE STRICT(i + 1 AS col) from columns_transformers; -- { serverError 16 } SELECT * REPLACE(i + 1 AS i) APPLY(sum) from columns_transformers; SELECT columns_transformers.* REPLACE(j + 2 AS j, i + 1 AS i) APPLY(avg) from columns_transformers; SELECT columns_transformers.* REPLACE(j + 1 AS j, j + 2 AS j) APPLY(avg) from columns_transformers; -- { serverError 43 } -- REPLACE after APPLY will not match anything SELECT a.* APPLY(toDate) REPLACE(i + 1 AS i) APPLY(any) from columns_transformers a; -SELECT a.* APPLY(toDate) REPLACESTRICT(i + 1 AS i) APPLY(any) from columns_transformers a; -- { serverError 16 } +SELECT a.* APPLY(toDate) REPLACE STRICT(i + 1 AS i) APPLY(any) from columns_transformers a; -- { serverError 16 } EXPLAIN SYNTAX SELECT * APPLY(sum) from columns_transformers; EXPLAIN SYNTAX SELECT columns_transformers.* APPLY(avg) from columns_transformers; From 08009631355ad31eb630e4b5f153c270cbbcf811 Mon Sep 17 00:00:00 2001 From: hexiaoting Date: Fri, 23 Oct 2020 17:15:55 +0800 Subject: [PATCH 04/10] Update parser for column transformers --- src/Parsers/ASTColumnsTransformers.cpp | 18 +++-- src/Parsers/ExpressionElementParsers.cpp | 70 ++++++++++++------- .../01470_columns_transformers.reference | 5 ++ .../01470_columns_transformers.sql | 4 ++ 4 files changed, 67 insertions(+), 30 deletions(-) diff --git a/src/Parsers/ASTColumnsTransformers.cpp b/src/Parsers/ASTColumnsTransformers.cpp index 474c3262d78..719c818b82d 100644 --- a/src/Parsers/ASTColumnsTransformers.cpp +++ b/src/Parsers/ASTColumnsTransformers.cpp @@ -33,7 +33,7 @@ void IASTColumnsTransformer::transform(const ASTPtr & transformer, ASTs & nodes) void ASTColumnsApplyTransformer::formatImpl(const FormatSettings & settings, FormatState &, FormatStateStacked) const { - settings.ostr << (settings.hilite ? hilite_keyword : "") << "APPLY" << (settings.hilite ? hilite_none : "") << "(" << func_name << ")"; + settings.ostr << (settings.hilite ? hilite_keyword : "") << "APPLY" << (settings.hilite ? hilite_none : "") << " " << func_name; } void ASTColumnsApplyTransformer::transform(ASTs & nodes) const @@ -46,7 +46,10 @@ void ASTColumnsApplyTransformer::transform(ASTs & nodes) const void ASTColumnsExceptTransformer::formatImpl(const FormatSettings & settings, FormatState & state, FormatStateStacked frame) const { - settings.ostr << (settings.hilite ? hilite_keyword : "") << "EXCEPT" << (is_strict ? " STRICT " : "") << (settings.hilite ? hilite_none : "") << "("; + settings.ostr << (settings.hilite ? hilite_keyword : "") << "EXCEPT" << (is_strict ? " STRICT " : "") << (settings.hilite ? hilite_none : ""); + + if (children.size() > 1) + settings.ostr << " ("; for (ASTs::const_iterator it = children.begin(); it != children.end(); ++it) { @@ -57,7 +60,8 @@ void ASTColumnsExceptTransformer::formatImpl(const FormatSettings & settings, Fo (*it)->formatImpl(settings, state, frame); } - settings.ostr << ")"; + if (children.size() > 1) + settings.ostr << ")"; } void ASTColumnsExceptTransformer::transform(ASTs & nodes) const @@ -110,7 +114,10 @@ void ASTColumnsReplaceTransformer::Replacement::formatImpl( void ASTColumnsReplaceTransformer::formatImpl(const FormatSettings & settings, FormatState & state, FormatStateStacked frame) const { - settings.ostr << (settings.hilite ? hilite_keyword : "") << "REPLACE" << (is_strict ? " STRICT " : "") << (settings.hilite ? hilite_none : "") << "("; + settings.ostr << (settings.hilite ? hilite_keyword : "") << "REPLACE" << (is_strict ? " STRICT " : "") << (settings.hilite ? hilite_none : ""); + + if (children.size() > 1) + settings.ostr << " ("; for (ASTs::const_iterator it = children.begin(); it != children.end(); ++it) { @@ -121,7 +128,8 @@ void ASTColumnsReplaceTransformer::formatImpl(const FormatSettings & settings, F (*it)->formatImpl(settings, state, frame); } - settings.ostr << ")"; + if (children.size() > 1) + settings.ostr << ")"; } void ASTColumnsReplaceTransformer::replaceChildren(ASTPtr & node, const ASTPtr & replacement, const String & name) diff --git a/src/Parsers/ExpressionElementParsers.cpp b/src/Parsers/ExpressionElementParsers.cpp index dc8f39ae894..501a9b00f3b 100644 --- a/src/Parsers/ExpressionElementParsers.cpp +++ b/src/Parsers/ExpressionElementParsers.cpp @@ -1240,17 +1240,24 @@ bool ParserColumnsTransformers::parseImpl(Pos & pos, ASTPtr & node, Expected & e if (apply.ignore(pos, expected)) { - if (pos->type != TokenType::OpeningRoundBracket) - return false; - ++pos; + bool with_open_round_bracket = false; + + if (pos->type == TokenType::OpeningRoundBracket) + { + ++pos; + with_open_round_bracket = true; + } String func_name; if (!parseIdentifierOrStringLiteral(pos, expected, func_name)) return false; - if (pos->type != TokenType::ClosingRoundBracket) - return false; - ++pos; + if (with_open_round_bracket) + { + if (pos->type != TokenType::ClosingRoundBracket) + return false; + ++pos; + } auto res = std::make_shared(); res->func_name = func_name; @@ -1259,14 +1266,9 @@ bool ParserColumnsTransformers::parseImpl(Pos & pos, ASTPtr & node, Expected & e } else if (except.ignore(pos, expected)) { - if (strict.ignore(pos, expected)) is_strict = true; - if (pos->type != TokenType::OpeningRoundBracket) - return false; - ++pos; - ASTs identifiers; auto parse_id = [&identifiers, &pos, &expected] { @@ -1278,12 +1280,23 @@ bool ParserColumnsTransformers::parseImpl(Pos & pos, ASTPtr & node, Expected & e return true; }; - if (!ParserList::parseUtil(pos, expected, parse_id, false)) - return false; + if (pos->type == TokenType::OpeningRoundBracket) + { + // support one or more parameter + ++pos; + if (!ParserList::parseUtil(pos, expected, parse_id, false)) + return false; - if (pos->type != TokenType::ClosingRoundBracket) - return false; - ++pos; + if (pos->type != TokenType::ClosingRoundBracket) + return false; + ++pos; + } + else + { + // only one parameter + if (!parse_id()) + return false; + } auto res = std::make_shared(); res->children = std::move(identifiers); @@ -1296,10 +1309,6 @@ bool ParserColumnsTransformers::parseImpl(Pos & pos, ASTPtr & node, Expected & e if (strict.ignore(pos, expected)) is_strict = true; - if (pos->type != TokenType::OpeningRoundBracket) - return false; - ++pos; - ASTs replacements; ParserExpression element_p; ParserIdentifier ident_p; @@ -1323,12 +1332,23 @@ bool ParserColumnsTransformers::parseImpl(Pos & pos, ASTPtr & node, Expected & e return true; }; - if (!ParserList::parseUtil(pos, expected, parse_id, false)) - return false; + if (pos->type == TokenType::OpeningRoundBracket) + { + ++pos; - if (pos->type != TokenType::ClosingRoundBracket) - return false; - ++pos; + if (!ParserList::parseUtil(pos, expected, parse_id, false)) + return false; + + if (pos->type != TokenType::ClosingRoundBracket) + return false; + ++pos; + } + else + { + // only one parameter + if (!parse_id()) + return false; + } auto res = std::make_shared(); res->children = std::move(replacements); diff --git a/tests/queries/0_stateless/01470_columns_transformers.reference b/tests/queries/0_stateless/01470_columns_transformers.reference index 2d8a1802289..9a8a02c7f98 100644 --- a/tests/queries/0_stateless/01470_columns_transformers.reference +++ b/tests/queries/0_stateless/01470_columns_transformers.reference @@ -1,4 +1,5 @@ 220 18 347 +220 18 347 110 9 173.5 1970-04-11 1970-01-11 1970-11-21 2 3 @@ -6,6 +7,10 @@ 18 347 110 173.5 1970-04-11 1970-01-11 1970-11-21 +10 324 +8 23 +101 10 324 +121 8 23 222 18 347 111 11 173.5 1970-04-11 1970-01-11 1970-11-21 diff --git a/tests/queries/0_stateless/01470_columns_transformers.sql b/tests/queries/0_stateless/01470_columns_transformers.sql index a3d103cd876..f7c30eef61b 100644 --- a/tests/queries/0_stateless/01470_columns_transformers.sql +++ b/tests/queries/0_stateless/01470_columns_transformers.sql @@ -4,6 +4,7 @@ CREATE TABLE columns_transformers (i Int64, j Int16, k Int64) Engine=TinyLog; INSERT INTO columns_transformers VALUES (100, 10, 324), (120, 8, 23); SELECT * APPLY(sum) from columns_transformers; +SELECT * APPLY sum from columns_transformers; SELECT columns_transformers.* APPLY(avg) from columns_transformers; SELECT a.* APPLY(toDate) APPLY(any) from columns_transformers a; SELECT COLUMNS('[jk]') APPLY(toString) APPLY(length) from columns_transformers; @@ -13,7 +14,10 @@ SELECT columns_transformers.* EXCEPT(j) APPLY(avg) from columns_transformers; -- EXCEPT after APPLY will not match anything SELECT a.* APPLY(toDate) EXCEPT(i, j) APPLY(any) from columns_transformers a; +SELECT * EXCEPT STRICT i from columns_transformers; +SELECT * EXCEPT STRICT i, j1 from columns_transformers; -- { serverError 47 } SELECT * EXCEPT STRICT(i, j1) from columns_transformers; -- { serverError 16 } +SELECT * REPLACE STRICT i + 1 AS i from columns_transformers; SELECT * REPLACE STRICT(i + 1 AS col) from columns_transformers; -- { serverError 16 } SELECT * REPLACE(i + 1 AS i) APPLY(sum) from columns_transformers; SELECT columns_transformers.* REPLACE(j + 2 AS j, i + 1 AS i) APPLY(avg) from columns_transformers; From ad8eac492919915dac9219bd64cd6d69d4d3b792 Mon Sep 17 00:00:00 2001 From: hexiaoting Date: Mon, 9 Nov 2020 13:58:32 +0800 Subject: [PATCH 05/10] build error and style error fix --- src/Parsers/ASTColumnsTransformers.cpp | 10 +++++----- src/Parsers/ExpressionElementParsers.cpp | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/Parsers/ASTColumnsTransformers.cpp b/src/Parsers/ASTColumnsTransformers.cpp index b28644383d1..7af9786bfdb 100644 --- a/src/Parsers/ASTColumnsTransformers.cpp +++ b/src/Parsers/ASTColumnsTransformers.cpp @@ -43,7 +43,7 @@ void ASTColumnsApplyTransformer::formatImpl(const FormatSettings & settings, For parameters->formatImpl(settings, state, frame); if (!column_name_prefix.empty()) - settings.ostr << ", '" << column_name_prefix << "')"; + settings.ostr << ", '" << column_name_prefix << "')"; } void ASTColumnsApplyTransformer::transform(ASTs & nodes) const @@ -214,12 +214,12 @@ void ASTColumnsReplaceTransformer::transform(ASTs & nodes) const if (is_strict && !replace_map.empty()) { - String expected_columns = ""; - for (auto it = replace_map.begin(); it != replace_map.end(); ++it) + String expected_columns; + for (auto & elem: replace_map) { - if (expected_columns != "") + if (!expected_columns.empty()) expected_columns += ", "; - expected_columns += it->first; + expected_columns += elem.first; } throw Exception( "Columns transformer REPLACE expects following column(s) : " + expected_columns, diff --git a/src/Parsers/ExpressionElementParsers.cpp b/src/Parsers/ExpressionElementParsers.cpp index 0a0fca53545..47fc1423d3e 100644 --- a/src/Parsers/ExpressionElementParsers.cpp +++ b/src/Parsers/ExpressionElementParsers.cpp @@ -1317,7 +1317,7 @@ bool ParserColumnsTransformers::parseImpl(Pos & pos, ASTPtr & node, Expected & e if (pos->type != TokenType::ClosingRoundBracket) return false; ++pos; - } + } auto res = std::make_shared(); res->func_name = getIdentifierName(func_name); From 5622882553ce92fc0abf6cd308c533e54b74a9dc Mon Sep 17 00:00:00 2001 From: hexiaoting Date: Mon, 9 Nov 2020 14:21:50 +0800 Subject: [PATCH 06/10] remove unused code --- src/Parsers/ASTColumnsTransformers.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/src/Parsers/ASTColumnsTransformers.cpp b/src/Parsers/ASTColumnsTransformers.cpp index 7af9786bfdb..2d4d2304bd7 100644 --- a/src/Parsers/ASTColumnsTransformers.cpp +++ b/src/Parsers/ASTColumnsTransformers.cpp @@ -48,7 +48,6 @@ void ASTColumnsApplyTransformer::formatImpl(const FormatSettings & settings, For void ASTColumnsApplyTransformer::transform(ASTs & nodes) const { - std::cout << "\033[31m" << __FILE__ << ":"<<__LINE__ << "\033[39m" << std::endl; for (auto & column : nodes) { String name; From 2b240029c148e3bfdd1bf534217b54979f573146 Mon Sep 17 00:00:00 2001 From: hexiaoting Date: Mon, 9 Nov 2020 17:13:27 +0800 Subject: [PATCH 07/10] Add space for format Impl --- src/Parsers/ASTColumnsTransformers.cpp | 41 +++++++++++++------------- 1 file changed, 20 insertions(+), 21 deletions(-) diff --git a/src/Parsers/ASTColumnsTransformers.cpp b/src/Parsers/ASTColumnsTransformers.cpp index 2d4d2304bd7..9ae32aa3f74 100644 --- a/src/Parsers/ASTColumnsTransformers.cpp +++ b/src/Parsers/ASTColumnsTransformers.cpp @@ -71,10 +71,10 @@ void ASTColumnsApplyTransformer::transform(ASTs & nodes) const void ASTColumnsExceptTransformer::formatImpl(const FormatSettings & settings, FormatState & state, FormatStateStacked frame) const { - settings.ostr << (settings.hilite ? hilite_keyword : "") << "EXCEPT" << (is_strict ? " STRICT " : "") << (settings.hilite ? hilite_none : ""); + settings.ostr << (settings.hilite ? hilite_keyword : "") << "EXCEPT" << (is_strict ? " STRICT " : " ") << (settings.hilite ? hilite_none : ""); if (children.size() > 1) - settings.ostr << " ("; + settings.ostr << "("; for (ASTs::const_iterator it = children.begin(); it != children.end(); ++it) { @@ -93,26 +93,25 @@ void ASTColumnsExceptTransformer::transform(ASTs & nodes) const { ASTs expected_columns(children); - nodes.erase( - std::remove_if( - nodes.begin(), - nodes.end(), - [&](const ASTPtr & node_child) + for (auto it = nodes.begin(); it != nodes.end();) + { + bool removed = false; + if (const auto * id = it->get()->as()) + { + for (int i = expected_columns.size() - 1; i >= 0; --i) { - if (const auto * id = node_child->as()) + if (expected_columns[i]->as().name() == id->shortName()) { - for (int i = expected_columns.size() - 1; i >= 0; --i) - { - if (expected_columns[i]->as().name() == id->shortName()) - { - expected_columns.erase(expected_columns.begin() + i); - return true; - } - } + removed = true; + expected_columns.erase(expected_columns.begin() + i); + it = nodes.erase(it); } - return false; - }), - nodes.end()); + } + } + + if (!removed) + ++it; + } if (is_strict && !expected_columns.empty()) { @@ -139,10 +138,10 @@ void ASTColumnsReplaceTransformer::Replacement::formatImpl( void ASTColumnsReplaceTransformer::formatImpl(const FormatSettings & settings, FormatState & state, FormatStateStacked frame) const { - settings.ostr << (settings.hilite ? hilite_keyword : "") << "REPLACE" << (is_strict ? " STRICT " : "") << (settings.hilite ? hilite_none : ""); + settings.ostr << (settings.hilite ? hilite_keyword : "") << "REPLACE" << (is_strict ? " STRICT " : " ") << (settings.hilite ? hilite_none : ""); if (children.size() > 1) - settings.ostr << " ("; + settings.ostr << "("; for (ASTs::const_iterator it = children.begin(); it != children.end(); ++it) { From 694ad1f45274d569a4f42635b52e1e8f82f42b90 Mon Sep 17 00:00:00 2001 From: hexiaoting Date: Tue, 10 Nov 2020 11:14:41 +0800 Subject: [PATCH 08/10] Modify except column transformer's error log --- src/Parsers/ASTColumnsTransformers.cpp | 26 ++++++++----------- .../01470_columns_transformers.reference | 2 ++ .../01470_columns_transformers.sql | 1 + 3 files changed, 14 insertions(+), 15 deletions(-) diff --git a/src/Parsers/ASTColumnsTransformers.cpp b/src/Parsers/ASTColumnsTransformers.cpp index 9ae32aa3f74..cd28d8bfae9 100644 --- a/src/Parsers/ASTColumnsTransformers.cpp +++ b/src/Parsers/ASTColumnsTransformers.cpp @@ -91,21 +91,21 @@ void ASTColumnsExceptTransformer::formatImpl(const FormatSettings & settings, Fo void ASTColumnsExceptTransformer::transform(ASTs & nodes) const { - ASTs expected_columns(children); + std::set expected_columns; + for (size_t i = 0; i < children.size(); ++i) + expected_columns.insert(children[i]->as().name()); for (auto it = nodes.begin(); it != nodes.end();) { bool removed = false; if (const auto * id = it->get()->as()) { - for (int i = expected_columns.size() - 1; i >= 0; --i) + auto expected_column = expected_columns.find(id->shortName()); + if (expected_column != expected_columns.end()) { - if (expected_columns[i]->as().name() == id->shortName()) - { - removed = true; - expected_columns.erase(expected_columns.begin() + i); - it = nodes.erase(it); - } + removed = true; + expected_columns.erase(expected_column); + it = nodes.erase(it); } } @@ -116,15 +116,11 @@ void ASTColumnsExceptTransformer::transform(ASTs & nodes) const if (is_strict && !expected_columns.empty()) { String expected_columns_str; - for (size_t i = 0; i < expected_columns.size(); ++i) - { - if (i > 0) - expected_columns_str += ", "; - expected_columns_str += expected_columns[i]->as().name(); - } + std::for_each(expected_columns.begin(), expected_columns.end(), + [&](String x) { expected_columns_str += (" " + x) ; }); throw Exception( - "Columns transformer EXCEPT expects following column(s) : " + expected_columns_str, + "Columns transformer EXCEPT expects following column(s) :" + expected_columns_str, ErrorCodes::NO_SUCH_COLUMN_IN_TABLE); } } diff --git a/tests/queries/0_stateless/01470_columns_transformers.reference b/tests/queries/0_stateless/01470_columns_transformers.reference index cfe93c927bf..a103d62167b 100644 --- a/tests/queries/0_stateless/01470_columns_transformers.reference +++ b/tests/queries/0_stateless/01470_columns_transformers.reference @@ -9,6 +9,8 @@ 1970-04-11 1970-01-11 1970-11-21 10 324 8 23 +324 +23 101 10 324 121 8 23 222 18 347 diff --git a/tests/queries/0_stateless/01470_columns_transformers.sql b/tests/queries/0_stateless/01470_columns_transformers.sql index bae0a0e5237..2da2f6e9c67 100644 --- a/tests/queries/0_stateless/01470_columns_transformers.sql +++ b/tests/queries/0_stateless/01470_columns_transformers.sql @@ -15,6 +15,7 @@ SELECT columns_transformers.* EXCEPT(j) APPLY(avg) from columns_transformers; SELECT a.* APPLY(toDate) EXCEPT(i, j) APPLY(any) from columns_transformers a; SELECT * EXCEPT STRICT i from columns_transformers; +SELECT * EXCEPT STRICT (i, j) from columns_transformers; SELECT * EXCEPT STRICT i, j1 from columns_transformers; -- { serverError 47 } SELECT * EXCEPT STRICT(i, j1) from columns_transformers; -- { serverError 16 } SELECT * REPLACE STRICT i + 1 AS i from columns_transformers; From 293ae54fab9cae6347a16c6e1f82945395781f98 Mon Sep 17 00:00:00 2001 From: hexiaoting Date: Tue, 10 Nov 2020 15:18:12 +0800 Subject: [PATCH 09/10] Modify codes to make logical clearly --- src/Parsers/ASTColumnsTransformers.cpp | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/src/Parsers/ASTColumnsTransformers.cpp b/src/Parsers/ASTColumnsTransformers.cpp index cd28d8bfae9..00a3ed68fd8 100644 --- a/src/Parsers/ASTColumnsTransformers.cpp +++ b/src/Parsers/ASTColumnsTransformers.cpp @@ -92,24 +92,23 @@ void ASTColumnsExceptTransformer::formatImpl(const FormatSettings & settings, Fo void ASTColumnsExceptTransformer::transform(ASTs & nodes) const { std::set expected_columns; - for (size_t i = 0; i < children.size(); ++i) - expected_columns.insert(children[i]->as().name()); + for (const auto & child : children) + expected_columns.insert(child->as().name()); for (auto it = nodes.begin(); it != nodes.end();) { - bool removed = false; if (const auto * id = it->get()->as()) { auto expected_column = expected_columns.find(id->shortName()); if (expected_column != expected_columns.end()) { - removed = true; expected_columns.erase(expected_column); it = nodes.erase(it); } + else + ++it; } - - if (!removed) + else ++it; } From 024848cc1b2cbc5d93f7126f0a23deec29f395e4 Mon Sep 17 00:00:00 2001 From: hexiaoting Date: Fri, 27 Nov 2020 11:21:22 +0800 Subject: [PATCH 10/10] fix issue for assert --- src/Interpreters/OpenTelemetrySpanLog.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Interpreters/OpenTelemetrySpanLog.cpp b/src/Interpreters/OpenTelemetrySpanLog.cpp index cf82f1d5209..de82f8a4d01 100644 --- a/src/Interpreters/OpenTelemetrySpanLog.cpp +++ b/src/Interpreters/OpenTelemetrySpanLog.cpp @@ -110,7 +110,7 @@ OpenTelemetrySpanHolder::~OpenTelemetrySpanHolder() // First of all, return old value of current span. auto & thread = CurrentThread::get(); - assert(thread.thread_trace_context.span_id = span_id); + assert(thread.thread_trace_context.span_id == span_id); thread.thread_trace_context.span_id = parent_span_id; // Not sure what's the best way to access the log from here.