New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add database engine of MySQL type #5599
Add database engine of MySQL type #5599
Conversation
Co-authored-by: zhang2014 <coswde@gmail.com> Co-authored-by: TCeason <tai_chong@foxmail.com>
968f51d
to
6ccef4d
Compare
2416a81
to
d8a14f5
Compare
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? |
Now everything is green. |
{ | ||
std::stringstream ostr; | ||
formatAST(*engine_define, ostr, false, false); | ||
throw Exception("Unknown database engine: " + ostr.str(), ErrorCodes::UNKNOWN_DATABASE_ENGINE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
dbms/src/Common/dataTypeTransform.h
Outdated
@@ -0,0 +1,22 @@ | |||
#pragma once |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The file name is too generic. You can name it convertMySQLDataType
.
dbms/src/Common/dataTypeTransform.h
Outdated
namespace DB | ||
{ | ||
|
||
ASTPtr getDataTypeAST(const DataTypePtr & data_type); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Non-descriptive function name.
Missing comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(the method name was Ok before, because it was hidden inside single cpp file)
|
||
else if (engine_name == "MySQL") | ||
{ | ||
ASTFunction * engine = engine_define->engine; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const?
dbms/src/Databases/DatabaseFactory.h
Outdated
@@ -2,6 +2,7 @@ | |||
|
|||
#include <Common/ThreadPool.h> | |||
#include <Databases/IDatabase.h> | |||
#include <Parsers/ASTCreateQuery.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not necessarily to include header, because you can use forward declaration.
dbms/src/Databases/DatabaseMySQL.cpp
Outdated
} | ||
} | ||
|
||
cond.wait_for(lock, cleaner_sleep_time, quit_requested); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can move it in a while condition, making quit_requested
lambda unneeded.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might make the code less readable ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But now it looks redundant.
dbms/src/Databases/DatabaseMySQL.cpp
Outdated
|
||
static constexpr const std::chrono::seconds cleaner_sleep_time{30}; | ||
|
||
String toQueryStringWithQuote(const std::vector<String> "e_list) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style (whitespace around &)
* 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, how it was working without cache?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what would happen if you returned different storage to the same query. Maybe we should write a test ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, the cache is reasonable.
dbms/src/Databases/DatabasesCommon.h
Outdated
@@ -76,13 +76,13 @@ class DatabaseWithOwnTablesBase : public IDatabase | |||
public: | |||
bool isTableExist( | |||
const Context & context, | |||
const String & table_name) const override; | |||
const String & table_name) override; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like removing const
from methods... maybe it is better to make cache mutable
.
@@ -164,6 +115,9 @@ StoragePtr TableFunctionMySQL::executeImpl(const ASTPtr & ast_function, const Co | |||
|
|||
} | |||
|
|||
if (columns.empty()) | |||
throw Exception("MySQL table " + database_name + "." + table_name + " doesn't exist..", ErrorCodes::UNKNOWN_TABLE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
exist..
- Two dots.
@@ -164,6 +115,9 @@ StoragePtr TableFunctionMySQL::executeImpl(const ASTPtr & ast_function, const Co | |||
|
|||
} | |||
|
|||
if (columns.empty()) | |||
throw Exception("MySQL table " + database_name + "." + table_name + " doesn't exist..", ErrorCodes::UNKNOWN_TABLE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
backQuoteIfNeed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost Ok, just a few questions remain.
3c65409
to
162d05b
Compare
162d05b
to
ec8d735
Compare
(*block.getByPosition(1).column)[i].safeGet<String>(), | ||
(*block.getByPosition(2).column)[i].safeGet<UInt64>() && context.getSettings().external_table_functions_use_nulls, | ||
(*block.getByPosition(3).column)[i].safeGet<UInt64>(), | ||
(*block.getByPosition(4).column)[i].safeGet<UInt64>())); | ||
|
||
} | ||
|
||
if (columns.empty()) | ||
throw Exception("MySQL table `" + database_name + "`.`" + table_name + "` doesn't exist.", ErrorCodes::UNKNOWN_TABLE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But there is a function named backQuoteIfNeed
that will also do proper escaping.
I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/?lang=en
Category:
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