Add database engine of MySQL type by zhang2014 · Pull Request #5599 · ClickHouse/ClickHouse

zhang2014 · 2019-06-13T03:57:44Z

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

Category:

New Feature

Detailed description (optional):

It doesn't make any manipulations with filesystem.
All tables are created by calling code after real-time pull-out structure from remote MySQL

Co-authored-by: zhang2014 <[email protected]> Co-authored-by: TCeason <[email protected]>

zhang2014 · 2019-06-13T12:17:56Z

There was no way to determine from the returned log whether the error was related to the modification, and I need some help.

BTW, is there more information with the other two pending tests?

alexey-milovidov · 2019-06-13T14:53:37Z

Now everything is green.

alexey-milovidov · 2019-06-14T05:45:18Z

dbms/src/Databases/DatabaseFactory.cpp

+        {
+            std::stringstream ostr;
+            formatAST(*engine_define, ostr, false, false);
+            throw Exception("Unknown database engine: " + ostr.str(), ErrorCodes::UNKNOWN_DATABASE_ENGINE);


You can make error message more descriptive, like "Database engine XYZ cannot have arguments".

PS. And parameters, primary_key, order_by, sample_by, settings should not ever be in CREATE DATABASE statement.

alexey-milovidov · 2019-06-14T06:00:21Z

dbms/src/Common/dataTypeTransform.h

@@ -0,0 +1,22 @@
+#pragma once


The file name is too generic. You can name it convertMySQLDataType.

alexey-milovidov · 2019-06-14T06:00:51Z

dbms/src/Common/dataTypeTransform.h

+namespace DB
+{
+
+ASTPtr getDataTypeAST(const DataTypePtr & data_type);


Non-descriptive function name.
Missing comments.

(the method name was Ok before, because it was hidden inside single cpp file)

alexey-milovidov · 2019-06-14T06:01:37Z

dbms/src/Databases/DatabaseFactory.cpp

+
+    else if (engine_name == "MySQL")
+    {
+        ASTFunction * engine = engine_define->engine;


alexey-milovidov · 2019-06-14T06:02:09Z

dbms/src/Databases/DatabaseFactory.h


 #include <Common/ThreadPool.h>
 #include <Databases/IDatabase.h>
+#include <Parsers/ASTCreateQuery.h>


It's not necessarily to include header, because you can use forward declaration.

alexey-milovidov · 2019-06-14T06:06:50Z

dbms/src/Databases/DatabaseMySQL.cpp

+            }
+        }
+
+        cond.wait_for(lock, cleaner_sleep_time, quit_requested);


You can move it in a while condition, making quit_requested lambda unneeded.

This might make the code less readable ?

But now it looks redundant.

alexey-milovidov · 2019-06-14T06:07:58Z

dbms/src/Databases/DatabaseMySQL.cpp

+
+static constexpr const std::chrono::seconds cleaner_sleep_time{30};
+
+String toQueryStringWithQuote(const std::vector<String> &quote_list)


Style (whitespace around &)

alexey-milovidov · 2019-06-14T06:13:36Z

dbms/src/Databases/DatabaseMySQL.h

+ *  It doesn't make any manipulations with filesystem.
+ *  All tables are created by calling code after real-time pull-out structure from remote MySQL
+ */
+class DatabaseMySQL : public IDatabase


BTW, how it was working without cache?

The current way of working is not very efficient. Currently, a simple query(no join, no subquery) will call the tryGetStorage method five or six times.

The MySQL database engine attempts to query MySQL system tables every time tryGetStorage is called. This will exist the following scenarios:

exist in cache & no exist in mysql : Move tables to outdated list for asynchronous drop.

no exist in cache & exist in mysql : Create tables and add to cache.

exist in cache & exist in mysql, but table structure are updated: Execute both cases simultaneously

I'm not sure what would happen if you returned different storage to the same query. Maybe we should write a test ?

Ok, the cache is reasonable.

alexey-milovidov · 2019-06-14T06:14:34Z

dbms/src/Databases/DatabasesCommon.h

    bool isTableExist(
        const Context & context,
-        const String & table_name) const override;
+        const String & table_name) override;


I don't like removing const from methods... maybe it is better to make cache mutable.

alexey-milovidov · 2019-06-14T06:15:39Z

dbms/src/TableFunctions/TableFunctionMySQL.cpp

    }

+    if (columns.empty())
+        throw Exception("MySQL table " + database_name + "." + table_name + " doesn't exist..", ErrorCodes::UNKNOWN_TABLE);


exist.. - Two dots.

alexey-milovidov · 2019-06-14T06:16:33Z

dbms/src/TableFunctions/TableFunctionMySQL.cpp

    }

+    if (columns.empty())
+        throw Exception("MySQL table " + database_name + "." + table_name + " doesn't exist..", ErrorCodes::UNKNOWN_TABLE);


backQuoteIfNeed

alexey-milovidov

Almost Ok, just a few questions remain.

alexey-milovidov · 2019-06-15T11:51:09Z

dbms/src/TableFunctions/TableFunctionMySQL.cpp

    }

+    if (columns.empty())
+        throw Exception("MySQL table `" + database_name + "`.`" + table_name + "` doesn't exist.", ErrorCodes::UNKNOWN_TABLE);


But there is a function named backQuoteIfNeed that will also do proper escaping.

TCeason and others added 2 commits June 10, 2019 15:51

feature: if MySQL table doesn't exits should report the error.

e01138a

Add database engine of MySQL type

6ccef4d

Co-authored-by: zhang2014 <[email protected]> Co-authored-by: TCeason <[email protected]>

zhang2014 force-pushed the fix_createtable_bug branch from 968f51d to 6ccef4d Compare June 13, 2019 03:59

alexey-milovidov added the can be tested label Jun 13, 2019

try fix unbundled build failure

d8a14f5

zhang2014 force-pushed the fix_createtable_bug branch from 2416a81 to d8a14f5 Compare June 13, 2019 10:01

alexey-milovidov added the pr-feature Pull request with new product feature label Jun 13, 2019

alexey-milovidov reviewed Jun 14, 2019

View reviewed changes

alexey-milovidov requested changes Jun 14, 2019

View reviewed changes

zhang2014 force-pushed the fix_createtable_bug branch from 3c65409 to 162d05b Compare June 14, 2019 17:04

some review fixes

ec8d735

zhang2014 force-pushed the fix_createtable_bug branch from 162d05b to ec8d735 Compare June 15, 2019 03:39

alexey-milovidov reviewed Jun 15, 2019

View reviewed changes

alexey-milovidov merged commit 807bfa9 into ClickHouse:master Jun 15, 2019

alexey-milovidov mentioned this pull request Jun 15, 2019

Fixed quotes in exceptions and log messages. #5639

Merged

This was referenced Jun 20, 2019

Couldn't insert the nullable data with the MySQL database engine. #5697

Closed

DOCAPI-7062: MySQL database engine docs. Update of MergeTree TTL docs. #5706

Merged

BayoNet mentioned this pull request Aug 8, 2019

DOCAPI-7062: MySQL database engine and MergeTree TTL docs. EN review, RU translation #6407

Merged


		static constexpr const std::chrono::seconds cleaner_sleep_time{30};

		String toQueryStringWithQuote(const std::vector<String> &quote_list)

Conversation

zhang2014 commented Jun 13, 2019

Uh oh!

zhang2014 commented Jun 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexey-milovidov commented Jun 13, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexey-milovidov Jun 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhang2014 Jun 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexey-milovidov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

zhang2014 commented Jun 13, 2019 •

edited

Loading

alexey-milovidov Jun 14, 2019 •

edited

Loading

zhang2014 Jun 14, 2019 •

edited

Loading