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
Remind properly if use clickhouse-client --file without preceeding --external #34765
Remind properly if use clickhouse-client --file without preceeding --external #34765
Conversation
@taiyang-li need to fix fast test. |
I will fix them soon. |
8a05a1e
to
7f69507
Compare
clickhouse-client will remind like this:
|
Done @kitaisreal |
Failed integration tests irrelavent with clickhouse-client |
@kitaisreal All checks have passed. |
@@ -1234,6 +1236,160 @@ void Client::processConfig() | |||
client_info.quota_key = config().getString("quota_key", ""); | |||
} | |||
|
|||
void Client::readArguments( |
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.
@taiyang-li could you please explain, why we extracted this method as ClientBase abstraction ?
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.
@kitaisreal Only in clickhouse-client we need to distinguish whether an option is in external group or not, because options_description.external_description
is not empty. But in clickhouse-local, we don't have to consider it. So implmentation for clickhouse-client and clickhouse-local seperately is necessary.
For example, options like --file
, --structure
in clickhouse-client is in external group, but not the same in clickhouse-local
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.
@kitaisreal do you think it is ok ?
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.
Yes, totally ok.
@kitaisreal Do you think this pr could be merged? Pls let me know if there are any other issues |
@kitaisreal Waiting for you reply, thanks! |
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 code LGTM.
…_option Merging #34765
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Remind properly if use clickhouse-client --file without preceeding --external. Close #34747