MaterializeMySQL Engine column comments support #25199
Conversation
Implemented MySQL engine column comments support
robot-clickhouse
added
doc-alert
pr-feature
kssenii
changed the title
| @@ -117,6 +118,37 @@ static inline NamesAndTypesList getColumnsList(ASTExpressionList * columns_defin | |||
| return columns_name_and_type; | |||
| } | |||
|
|
|||
| static ColumnsDescription getColumnsDescription(const NamesAndTypesList & columns_name_and_type, const ASTExpressionList * columns_definition) | |||
There was a problem hiding this comment.
It has the same name as InterpreterCreateQuery::getColumnsDescription, but implementation and purpose is a little different, so It will be better to use a different name to mark the difference.
| if (columns_name_and_type.size() != columns_definition->children.size()) | ||
| throw Exception("Columns of different size provided.", ErrorCodes::BAD_ARGUMENTS); |
There was a problem hiding this comment.
It seems to rather be LOGICAL_ERROR instead of BAD_ARGUMENTS.
Though columns_name_and_type is initialized with getColumnsList(columns_definition), where columns_name_and_type is created by iterating columns_definition->children, so probably assert(columns_name_and_type.size() == columns_definition->children.size()) is enough. But LOGICAL_ERROR is also ok.
| @@ -223,3 +223,15 @@ TEST(MySQLCreateRewritten, UniqueKeysConvert) | |||
| std::string(MATERIALIZEMYSQL_TABLE_COLUMNS) + | |||
| ") ENGINE = ReplacingMergeTree(_version) PARTITION BY intDiv(id, 18446744073709551) ORDER BY (code, name, tenant_id, id)"); | |||
| } | |||
|
|
|||
| TEST(MySQLCreateRewritten, QueryWithColumnComments) | |||
There was a problem hiding this comment.
May be you can also make integration test in tests/integration/test_materialize_mysql_database/test.py?
| for ( | ||
| auto [column_name_and_type, declare_column_ast] = std::tuple{columns_name_and_type.begin(), columns_definition->children.begin()}; | ||
| column_name_and_type != columns_name_and_type.end(); | ||
| column_name_and_type++, | ||
| declare_column_ast++ |
There was a problem hiding this comment.
Seems a little over-complicated given that column_name_and_type was actually initialized as getColumnsList(columns_definition) a few line before call to this getColumnsDescription.
It could be simpler to first create columns_description (by using implementation of getColumnsList plus one check for comment in column_options - there are already such checks for other options so it will be a more correct place for that, and returning columns_description there instead) and then get columns_name_and_type from columns_description->getAllPhysical().
But I think it is also ok to leave your variant if you really want..
There was a problem hiding this comment.
That was what I tried initially. Problem for this implementation is modifying type in GetKeys (modifyPrimaryKeysToNonNullable) for ColumnsDescription, since there is no good way (or I didn't found it) to change internal ColumnDescription by iterator.
So in my implementation columns_name_and_type is mostly needed to create final types and names for columns, to later initialize descriptions from them.
There was a problem hiding this comment.
hm, why there is a problem with getKeys - I was suggesting to only modify getColumnsList to return ColumnsDescription, instead of NameAndTypesList. And on the next line to get columns_with_type_and_name via columns_description->getAllPhysical(). All other methods will still use columns_with_type_and_name. This will simplify the code a lot and there will be no need for getColumnsDescriptionFromList method, which actually does the same thing, which is done in getColumnsList but for other column features.
There was a problem hiding this comment.
Since getKeys changes columns_with_type_and_name that we generate with getColumnsList, and if we just modify getColumnsList to return ColumnsDescription - we will miss this changes in our descriptions.
There was a problem hiding this comment.
Ok, I agree.
By the way, lets rename getColumnsDescriptionFromList -> createColumnsDescription
sand6255
force-pushed
the
MySQL-column-comments-support
branch
from
35232df
to
46b230d
Compare
|
@Mergifyio update |
|
Command
|
|
Internal documentation ticket: DOCSUP-10513. |
Implemented MySQL engine column comments support
I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/?lang=en
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
ClickHouse database created with MaterializeMySQL now contains all column comments from the MySQL database that materialized.