CLICKHOUSE-3837 Add support for table function in remote() and cluster() #2708

VadimPlh · 2018-07-24T13:13:00Z

I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/?lang=en

alexey-milovidov · 2018-07-24T16:25:38Z

dbms/src/Client/ConnectionPoolWithFailover.h

@@ -47,6 +47,8 @@ class ConnectionPoolWithFailover : public IConnectionPool, private PoolWithFailo
      */
    std::vector<Entry> getMany(const Settings * settings, PoolMode pool_mode);

+    std::vector<TryResult> getManyForTableFunc(const Settings * settings, PoolMode pool_mode);


Missing comment.
Let's use getManyForTableFunction name.

alexey-milovidov · 2018-07-24T16:29:47Z

dbms/src/Interpreters/ClusterProxy/SelectStreamFactory.cpp

+        if (table_func_ptr)
+        {
+            auto table_function = static_cast<ASTFunction *>(table_func_ptr.get());
+            main_table_storage = TableFunctionFactory::instance().get(table_function->name, context)->execute(table_func_ptr, context);


It's better to not execute table function on initiating server.
Because it may execute successfully only on remote nodes.
For example:
remote('host', merge(db, '^table$'))

database db may be absent locally and it is Ok.

alexey-milovidov · 2018-07-24T16:31:24Z

dbms/src/Interpreters/ClusterProxy/SelectStreamFactory.cpp

    {
        auto stream = std::make_shared<RemoteBlockInputStream>(shard_info.pool, query, header, context, nullptr, throttler, external_tables, processed_stage);
        stream->setPoolMode(PoolMode::GET_MANY);
        stream->setMainTable(main_table);
        res.emplace_back(std::move(stream));
    };
+
+    auto emplace_remote_stream_for_func = [&]()


It is almost identical, why don't just branch on table_func_ptr inside?

alexey-milovidov · 2018-07-24T16:31:44Z

dbms/src/Interpreters/ClusterProxy/SelectStreamFactory.h

        const Tables & external_tables);
+
+    SelectStreamFactory(


Missing comment.

alexey-milovidov · 2018-07-24T16:32:02Z

dbms/src/Interpreters/evaluateConstantExpression.cpp

@@ -51,14 +53,19 @@ std::pair<Field, std::shared_ptr<const IDataType>> evaluateConstantExpression(co


 ASTPtr evaluateConstantExpressionAsLiteral(const ASTPtr & node, const Context & context)
-{
+{   


Excessive whitespace.

alexey-milovidov · 2018-07-24T16:33:12Z

dbms/src/Interpreters/evaluateConstantExpression.cpp

    if (typeid_cast<const ASTLiteral *>(node.get()))
        return node;
-
+
+    if (auto table_func_ptr = typeid_cast<ASTFunction *>(node.get()))


Missing comment.

alexey-milovidov · 2018-07-24T16:40:06Z

dbms/src/Parsers/ASTSelectQuery.cpp

-        table_expression->database_and_table_name = std::make_shared<ASTIdentifier>(database_name + "." + table_name, ASTIdentifier::Table);
-        table_expression->database_and_table_name->children = {database, table};
+
+    if (table_function_ptr) {


alexey-milovidov · 2018-07-24T16:40:44Z

dbms/src/Parsers/ASTSelectQuery.h

@@ -46,7 +46,7 @@ class ASTSelectQuery : public IAST
    bool array_join_is_left() const;
    bool final() const;
    void setDatabaseIfNeeded(const String & database_name);
-    void replaceDatabaseAndTable(const String & database_name, const String & table_name);
+    void replaceDatabaseAndTable(const String & database_name, const String & table_name, ASTPtr table_function_name = nullptr);


Missing comment.

Maybe better to have two different methods?

alexey-milovidov · 2018-07-24T16:41:24Z

dbms/src/Storages/StorageDistributed.cpp

@@ -67,10 +67,10 @@ namespace

 /// select query has database and table names as AST pointers
 /// Creates a copy of query, changes database and table names.


Obsolete comment.

alexey-milovidov · 2018-07-24T16:41:43Z

dbms/src/Storages/StorageDistributed.cpp

@@ -154,7 +154,7 @@ StorageDistributed::StorageDistributed(
    bool attach)
    : IStorage{columns_},
    table_name(table_name_),
-    remote_database(remote_database_), remote_table(remote_table_),
+    remote_database(remote_database_), remote_table(remote_table_), remote_table_function_ptr(nullptr),


This doesn't make sense.

alexey-milovidov · 2018-07-24T16:44:17Z

dbms/src/Interpreters/evaluateConstantExpression.cpp

    if (typeid_cast<const ASTLiteral *>(node.get()))
        return node;
-
+
+    if (auto table_func_ptr = typeid_cast<ASTFunction *>(node.get()))


Missing comment.

alexey-milovidov · 2018-07-24T16:44:20Z

dbms/src/Storages/StorageDistributed.cpp

+        ClusterProxy::SelectStreamFactory select_stream_factory(
+            header, processed_stage, remote_table_function_ptr, context.getExternalTables());
+        return ClusterProxy::executeQuery(
+          select_stream_factory, cluster, modified_query_ast, context, settings);


alexey-milovidov · 2018-07-24T16:44:27Z

dbms/src/Storages/StorageDistributed.cpp

+    {
+        ClusterProxy::SelectStreamFactory select_stream_factory(
+            header, processed_stage, QualifiedTableName{remote_database, remote_table}, context.getExternalTables());
+        return ClusterProxy::executeQuery(
        select_stream_factory, cluster, modified_query_ast, context, settings);


alexey-milovidov · 2018-07-24T16:47:02Z

dbms/src/Storages/StorageDistributed.cpp

+    {
+        ClusterProxy::SelectStreamFactory select_stream_factory(
+            header, processed_stage, QualifiedTableName{remote_database, remote_table}, context.getExternalTables());
+        return ClusterProxy::executeQuery(
        select_stream_factory, cluster, modified_query_ast, context, settings);


alexey-milovidov · 2018-07-24T16:47:46Z

dbms/src/Storages/StorageDistributed.h

+    static StoragePtr createWithOwnCluster(
+        const std::string & table_name_,
+        const ColumnsDescription & columns_,
+        const String & remote_database_,       /// database on remote servers.


Why we need these two parameters: remote_database, remote_table, if we have remote_table_function_ptr?

alexey-milovidov · 2018-07-24T16:48:07Z

dbms/src/Storages/StorageDistributed.h

+        const ColumnsDescription & columns_,
+        const String & remote_database_,       /// database on remote servers.
+        const String & remote_table_,          /// The name of the table on the remote servers.
+        ASTPtr remote_table_function_ptr_,     /// Table function ptr.


Why not pass by reference?

alexey-milovidov · 2018-07-24T18:26:55Z

dbms/src/Storages/getStructureOfRemoteTable.cpp

+            return TableFunctionFactory::instance().get(table_function->name, context)->execute(table_func_ptr, context)->getColumns();
+        }
+
+        auto table_func_name = table_func_ptr->getAliasOrColumnName();


Better to use queryToString, because column name doesn't necessarily correspond to serialized expression in query.

alexey-milovidov · 2018-07-24T18:28:40Z

dbms/src/Storages/getStructureOfRemoteTable.cpp

    input->readPrefix();

    const DataTypeFactory & data_type_factory = DataTypeFactory::instance();

    ParserExpression expr_parser;
-
+    


Excessive whitespace.

alexey-milovidov · 2018-07-24T18:31:07Z

dbms/src/TableFunctions/TableFunctionRemote.cpp

@@ -196,8 +196,9 @@ StoragePtr TableFunctionRemote::executeImpl(const ASTPtr & ast_function, const C

    String cluster_name;
    String cluster_description;
-    String remote_database;
-    String remote_table;
+    String remote_database = "";


It doesn't make sense.

alexey-milovidov · 2018-07-24T18:31:51Z

dbms/src/TableFunctions/TableFunctionRemote.cpp

-    size_t dot = remote_database.find('.');
-    if (dot != String::npos)
+
+    auto table_function = static_cast<ASTFunction *>(args[arg_num].get());


alexey-milovidov · 2018-07-26T00:25:29Z

Missing functional test. At least as simple as
SELECT * FROM remote('127.0.0.{1,2}', numbers(10))

alexey-milovidov · 2018-07-26T00:26:05Z

Also should test remote(..., merge(...))

alexey-milovidov · 2018-07-26T11:39:05Z

You have a test only for local shard, and only for the case of single shard.
Better to add at least one remote shard like 127.0.0.{1,2}

VadimPE and others added 3 commits July 24, 2018 16:10

add support for table function in remote() and cluster()

8dfb680

Merge branch 'master' into CLICKHOUSE-3837

437f3f2

[CLICKHOUSE-3837] fix

18e7ef4

VadimPlh changed the title ~~add support for table function in remote() and cluster()~~ [CLICKHOUSE-3837] Add support for table function in remote() and cluster() Jul 24, 2018

[CLICKHOUSE-3837] fix error in build

46dec0d

VadimPlh requested a review from KochetovNicolai July 24, 2018 14:25

VadimPlh changed the title ~~[CLICKHOUSE-3837] Add support for table function in remote() and cluster()~~ CLICKHOUSE-3837 Add support for table function in remote() and cluster() Jul 24, 2018

alexey-milovidov reviewed Jul 24, 2018

View reviewed changes

Update evaluateConstantExpression.cpp

fb807c1

alexey-milovidov reviewed Jul 24, 2018

View reviewed changes

alexey-milovidov added 2 commits July 24, 2018 21:30

Update TableFunctionFactory.cpp

7b7c3a8

Update TableFunctionFactory.h

ca40a08

alexey-milovidov reviewed Jul 24, 2018

View reviewed changes

VadimPE added 4 commits July 25, 2018 15:31

CLICKHOUSE-3837 fix mistakes

867a3eb

CLICKHOUSE-3837 fix

1b07873

CLICKHOUSE-3837 fix build

1d5d097

CLICKHOUSE-3837 fix TableFunctionRemote

489f1f8

VadimPE and others added 2 commits July 26, 2018 11:06

CLICKHOUSE-3837 add tests

63ac512

Update 00675_remote_with_table_function.sql

2854818

VadimPE and others added 5 commits July 26, 2018 18:13

CLICKHOUSE-3837 fix tests

bb5d8db

CLICKHOUSE-3837 fix test

5c4b4e9

CLICKHOUSE-3837

894c5b3

Update 00675_shard_remote_with_table_function.sql

da41c5c

Update 00675_shard_remote_with_table_function.sql

ad00616

alexey-milovidov merged commit 9242588 into ClickHouse:master Jul 26, 2018

alexey-milovidov added a commit that referenced this pull request Jul 27, 2018

Fixed bad code #2708

8bc9541

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CLICKHOUSE-3837 Add support for table function in remote() and cluster() #2708

CLICKHOUSE-3837 Add support for table function in remote() and cluster() #2708

VadimPlh commented Jul 24, 2018

alexey-milovidov Jul 24, 2018

alexey-milovidov Jul 24, 2018 •

edited

Loading

alexey-milovidov Jul 24, 2018

alexey-milovidov Jul 24, 2018

alexey-milovidov Jul 24, 2018

alexey-milovidov Jul 24, 2018

alexey-milovidov Jul 24, 2018

alexey-milovidov Jul 24, 2018

alexey-milovidov Jul 24, 2018

alexey-milovidov Jul 24, 2018

alexey-milovidov Jul 24, 2018

alexey-milovidov Jul 24, 2018

alexey-milovidov Jul 24, 2018

alexey-milovidov Jul 24, 2018

alexey-milovidov Jul 24, 2018

alexey-milovidov Jul 24, 2018

alexey-milovidov Jul 24, 2018

alexey-milovidov Jul 24, 2018

alexey-milovidov Jul 24, 2018

alexey-milovidov Jul 24, 2018

alexey-milovidov Jul 24, 2018

alexey-milovidov commented Jul 26, 2018

alexey-milovidov commented Jul 26, 2018 •

edited

Loading

alexey-milovidov commented Jul 26, 2018

		@@ -67,10 +67,10 @@ namespace

		/// select query has database and table names as AST pointers
		/// Creates a copy of query, changes database and table names.

CLICKHOUSE-3837 Add support for table function in remote() and cluster() #2708

CLICKHOUSE-3837 Add support for table function in remote() and cluster() #2708

Conversation

VadimPlh commented Jul 24, 2018

Choose a reason for hiding this comment

alexey-milovidov Jul 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexey-milovidov commented Jul 26, 2018

alexey-milovidov commented Jul 26, 2018 • edited Loading

alexey-milovidov commented Jul 26, 2018

alexey-milovidov Jul 24, 2018 •

edited

Loading

alexey-milovidov commented Jul 26, 2018 •

edited

Loading