-
Notifications
You must be signed in to change notification settings - Fork 411
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
FLASH-796: support rename pk column #379
Conversation
@@ -247,44 +259,37 @@ void SchemaBuilder<Getter>::applyDiff(const SchemaDiff & diff) | |||
switch (diff.type) | |||
{ | |||
case SchemaActionCreateTable: | |||
case SchemaActionRecoverTable: | |||
{ | |||
case SchemaActionRecoverTable: { |
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.
changed by clang-format
/run-integration-tests |
/run-integration-tests |
1 similar comment
/run-integration-tests |
/run-integration-tests |
1 similar comment
/run-integration-tests |
if (params.size() != 1) | ||
{ | ||
throw Exception("There is an internal error for rename columns", ErrorCodes::LOGICAL_ERROR); | ||
throw Exception("There is an internal error for rename columns, params size should be 1", ErrorCodes::LOGICAL_ERROR); |
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.
When you are reporting errors like this, it's always better to say "SOME SIZE is Y, should be X" than "SOME SIZE is not X".
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.
LGTM
/run-integration-tests |
/run-integration-tests |
When changing pk column, we should change the pk_ast in
MergeTreeData
at the same time。test is added in full_stack_test, because common integration tests don't support create pk col.