Merge pull request #28888 from azat/mysql-in-fix

Fix queries to external databases (i.e. MySQL) with multiple columns in IN ( i.e. `(k,v) IN ((1, 2))` )

base/mysqlxx/Query.cpp

@@ -77,7 +77,9 @@ void Query::executeImpl()
         case CR_SERVER_LOST:
             throw ConnectionLost(errorMessage(mysql_driver), err_no);
         default:
-            throw BadQuery(errorMessage(mysql_driver), err_no);
+            /// Add query to the exception message, since it may differs from the user input query.
+            /// (also you can use this and create query with an error to see what query ClickHouse created)
+            throw BadQuery(errorMessage(mysql_driver) + " (query: " + query_string + ")", err_no);
         }
     }
 }

programs/copier/ClusterCopier.cpp

@@ -1274,13 +1274,14 @@ TaskStatus ClusterCopier::processPartitionPieceTaskImpl(
     auto get_select_query = [&] (const DatabaseAndTableName & from_table, const String & fields, bool enable_splitting, String limit = "")
     {
         String query;
+        query += "WITH " + task_partition.name + " AS partition_key ";
         query += "SELECT " + fields + " FROM " + getQuotedTable(from_table);
 
         if (enable_splitting && experimental_use_sample_offset)
             query += " SAMPLE 1/" + toString(number_of_splits) + " OFFSET " + toString(current_piece_number) + "/" + toString(number_of_splits);
 
         /// TODO: Bad, it is better to rewrite with ASTLiteral(partition_key_field)
-        query += " WHERE (" + queryToString(task_table.engine_push_partition_key_ast) + " = (" + task_partition.name + " AS partition_key))";
+        query += " WHERE (" + queryToString(task_table.engine_push_partition_key_ast) + " = partition_key)";
 
         if (enable_splitting && !experimental_use_sample_offset)
             query += " AND ( cityHash64(" + primary_key_comma_separated + ") %" + toString(number_of_splits) + " = " + toString(current_piece_number) + " )";
@@ -1851,9 +1852,9 @@ bool ClusterCopier::checkShardHasPartition(const ConnectionTimeouts & timeouts,
     TaskTable & task_table = task_shard.task_table;
 
     WriteBufferFromOwnString ss;
+    ss << "WITH " + partition_quoted_name + " AS partition_key ";
     ss << "SELECT 1 FROM " << getQuotedTable(task_shard.table_read_shard);
-    ss << " WHERE (" << queryToString(task_table.engine_push_partition_key_ast);
-    ss << " = (" + partition_quoted_name << " AS partition_key))";
+    ss << " WHERE (" << queryToString(task_table.engine_push_partition_key_ast) << " = partition_key)";
     if (!task_table.where_condition_str.empty())
         ss << " AND (" << task_table.where_condition_str << ")";
     ss << " LIMIT 1";
@@ -1882,13 +1883,15 @@ bool ClusterCopier::checkPresentPartitionPiecesOnCurrentShard(const ConnectionTi
 
     UNUSED(primary_key_comma_separated);
 
-    std::string query = "SELECT 1 FROM " + getQuotedTable(task_shard.table_read_shard);
+    std::string query;
+
+    query += "WITH " + partition_quoted_name + " AS partition_key ";
+    query += "SELECT 1 FROM " + getQuotedTable(task_shard.table_read_shard);
 
     if (experimental_use_sample_offset)
         query += " SAMPLE 1/" + toString(number_of_splits) + " OFFSET " + toString(current_piece_number) + "/" + toString(number_of_splits);
 
-    query += " WHERE (" + queryToString(task_table.engine_push_partition_key_ast)
-                        + " = (" + partition_quoted_name + " AS partition_key))";
+    query += " WHERE (" + queryToString(task_table.engine_push_partition_key_ast) + " = partition_key)";
 
     if (!experimental_use_sample_offset)
         query += " AND (cityHash64(" + primary_key_comma_separated + ") % "

src/Parsers/ExpressionElementParsers.cpp

@@ -103,26 +103,34 @@ bool ParserParenthesisExpression::parseImpl(Pos & pos, ASTPtr & node, Expected &
 
     const auto & expr_list = contents_node->as<ASTExpressionList &>();
 
-    /// empty expression in parentheses is not allowed
+    /// Empty expression in parentheses is not allowed.
     if (expr_list.children.empty())
     {
         expected.add(pos, "non-empty parenthesized list of expressions");
         return false;
     }
 
+    /// Special case for one-element tuple.
     if (expr_list.children.size() == 1 && is_elem)
     {
-        node = expr_list.children.front();
-    }
-    else
-    {
-        auto function_node = std::make_shared<ASTFunction>();
-        function_node->name = "tuple";
-        function_node->arguments = contents_node;
-        function_node->children.push_back(contents_node);
-        node = function_node;
+        auto * ast_literal = expr_list.children.front()->as<ASTLiteral>();
+        /// But only if its argument is not tuple,
+        /// since otherwise it will do incorrect transformation:
+        ///
+        ///     (foo,bar) IN (('foo','bar')) -> (foo,bar) IN ('foo','bar')
+        if (!(ast_literal && ast_literal->value.getType() == Field::Types::Tuple))
+        {
+            node = expr_list.children.front();
+            return true;
+        }
     }
 
+    auto function_node = std::make_shared<ASTFunction>();
+    function_node->name = "tuple";
+    function_node->arguments = contents_node;
+    function_node->children.push_back(contents_node);
+    node = function_node;
+
     return true;
 }
 

src/Storages/tests/gtest_transform_query_for_external_database.cpp

@@ -109,7 +109,7 @@ static void check(
     std::string transformed_query = transformQueryForExternalDatabase(
         query_info, state.getColumns(), IdentifierQuotingStyle::DoubleQuotes, "test", "table", state.context);
 
-    EXPECT_EQ(transformed_query, expected);
+    EXPECT_EQ(transformed_query, expected) << query;
 }
 
 
@@ -128,6 +128,18 @@ TEST(TransformQueryForExternalDatabase, InWithSingleElement)
           R"(SELECT "column" FROM "test"."table" WHERE "column" NOT IN ('hello', 'world'))");
 }
 
+TEST(TransformQueryForExternalDatabase, InWithMultipleColumns)
+{
+    const State & state = State::instance();
+
+    check(state, 1,
+          "SELECT column FROM test.table WHERE (1,1) IN ((1,1))",
+          R"(SELECT "column" FROM "test"."table" WHERE 1)");
+    check(state, 1,
+          "SELECT field, value FROM test.table WHERE (field, value) IN (('foo', 'bar'))",
+          R"(SELECT "field", "value" FROM "test"."table" WHERE ("field", "value") IN (('foo', 'bar')))");
+}
+
 TEST(TransformQueryForExternalDatabase, InWithTable)
 {
     const State & state = State::instance();

src/Storages/transformQueryForExternalDatabase.cpp

@@ -105,9 +105,9 @@ void dropAliases(ASTPtr & node)
 }
 
 
-bool isCompatible(const IAST & node)
+bool isCompatible(IAST & node)
 {
-    if (const auto * function = node.as<ASTFunction>())
+    if (auto * function = node.as<ASTFunction>())
     {
         if (function->parameters)   /// Parametric aggregate functions
             return false;
@@ -135,8 +135,14 @@ bool isCompatible(const IAST & node)
 
         /// A tuple with zero or one elements is represented by a function tuple(x) and is not compatible,
         /// but a normal tuple with more than one element is represented as a parenthesized expression (x, y) and is perfectly compatible.
-        if (name == "tuple" && function->arguments->children.size() <= 1)
-            return false;
+        /// So to support tuple with zero or one elements we can clear function name to get (x) instead of tuple(x)
+        if (name == "tuple")
+        {
+            if (function->arguments->children.size() <= 1)
+            {
+                function->name.clear();
+            }
+        }
 
         /// If the right hand side of IN is a table identifier (example: x IN table), then it's not compatible.
         if ((name == "in" || name == "notIn")

tests/integration/test_storage_mysql/test.py

@@ -367,6 +367,36 @@ def test_settings_connection_wait_timeout(started_cluster):
     drop_mysql_table(conn, table_name)
     conn.close()
 
+# Regression for (k, v) IN ((k, v))
+def test_mysql_in(started_cluster):
+    table_name = 'test_mysql_in'
+    node1.query(f'DROP TABLE IF EXISTS {table_name}')
+
+    conn = get_mysql_conn(started_cluster, cluster.mysql_ip)
+    drop_mysql_table(conn, table_name)
+    create_mysql_table(conn, table_name)
+
+    node1.query('''
+        CREATE TABLE {}
+        (
+            id UInt32,
+            name String,
+            age UInt32,
+            money UInt32
+        )
+        ENGINE = MySQL('mysql57:3306', 'clickhouse', '{}', 'root', 'clickhouse')
+        '''.format(table_name, table_name)
+    )
+
+    node1.query("INSERT INTO {} (id, name) SELECT number, concat('name_', toString(number)) from numbers(10) ".format(table_name))
+    node1.query("SELECT * FROM {} WHERE (id) IN (1)".format(table_name))
+    node1.query("SELECT * FROM {} WHERE (id) IN (1, 2)".format(table_name))
+    node1.query("SELECT * FROM {} WHERE (id, name) IN ((1, 'name_1'))".format(table_name))
+    node1.query("SELECT * FROM {} WHERE (id, name) IN ((1, 'name_1'),(1, 'name_1'))".format(table_name))
+
+    drop_mysql_table(conn, table_name)
+    conn.close()
+
 if __name__ == '__main__':
     with contextmanager(started_cluster)() as cluster:
         for name, instance in list(cluster.instances.items()):

tests/queries/0_stateless/00132_sets.sql

@@ -7,7 +7,7 @@ SELECT (1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, '') IN (1, 2, 3,
 SELECT (number AS n, n + 1, n + 2, n + 3) IN (1, 2, 3, 4) FROM system.numbers LIMIT 3;
 SELECT (number AS n, n + 1, n + 2, n + 3, n - 1) IN (1, 2, 3, 4, 0) FROM system.numbers LIMIT 3;
 SELECT (number AS n, n + 1, toString(n + 2), n + 3, n - 1) IN (1, 2, '3', 4, 0) FROM system.numbers LIMIT 3;
-SELECT number, tuple FROM (SELECT 1 AS number, (2, 3) AS tuple) WHERE (number, tuple) IN (((1, (2, 3)), (4, (5, 6))));
+SELECT number, tuple FROM (SELECT 1 AS number, (2, 3) AS tuple) WHERE (number, tuple) IN ( (/*number*/1, /*tuple*/(2, 3)), (/*number*/4, /*tuple*/(5, 6)) );
 SELECT number, tuple FROM (SELECT 2 AS number, (2, 3) AS tuple) WHERE (number, tuple) IN ((2, (2, 3)));
 SELECT number, tuple FROM (SELECT 3 AS number, (2, 3) AS tuple) WHERE (number, tuple) IN (3, (2, 3));
 SELECT number, tuple FROM (SELECT 4 AS number, (2, 3) AS tuple) WHERE (number, tuple) IN (SELECT 4, (2, 3));

tests/queries/0_stateless/01016_input_null_as_default.sh

@@ -57,8 +57,8 @@ $CLICKHOUSE_CLIENT --query="TRUNCATE TABLE null_as_default";
 
 echo 'Values'
 echo '(NULL, '\''1'\'', (null), '\''2019-07-22'\'', ([10, 20, 30]), (NuLl)),
-(1, '\''world'\'', (3), '\''2019-07-23'\'', (NULL), (('\''tuple'\'', 3.14))),
-(2, null, (123), null, ([]), (('\''test'\'', 2.71828))),
+(1, '\''world'\'', (3), '\''2019-07-23'\'', (NULL), ('\''tuple'\'', 3.14)),
+(2, null, (123), null, ([]), ('\''test'\'', 2.71828)),
 (3, null, (null), null, (null), (null))' | $CLICKHOUSE_CLIENT --input_format_null_as_default=1 --query="INSERT INTO null_as_default VALUES";
 $CLICKHOUSE_CLIENT --query="SELECT * FROM null_as_default ORDER BY i";
 $CLICKHOUSE_CLIENT --query="DROP TABLE null_as_default";