-
Notifications
You must be signed in to change notification settings - Fork 6.6k
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
Added connection string to clickhouse-client #50689
Conversation
Good day @rschu1ze please be reviewer of this pull request and add a label 'can be tested'. |
This is an automated comment for commit 4db8fa3 with description of existing statuses. It's updated for the latest CI running
|
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
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.
Haven't looked at the C++ uri parsing code and the test yet but my first impression is positive. Thanks!
I didn't have time to finish the changes today. I plan to finish and update on Tuesday. I'm still not sure about the renaming:
Also not sure about disabling I'm planning a couple more changes: |
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 remaining comments are minor and of cosmetic nature. Besides them, everything looks good, thanks!
else | ||
{ | ||
// in case of user_info == 'user:', ':' is specified, but password is empty | ||
// then add password argument "\n" which means: Ask user for a password. |
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.
Interesting. Didn't know that clickhouse-client had such a shortcut.
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.
Small inline question about constants:
Should we add public constant constexpr std::string_view ASK_USER_PASSWORD = "\n"sv
somewhere in ClientBase.h?
Using "\n" as a special case for password requires adding comments in several parts of code, it is not oblivious for other non-familiar developers with code base what does "\n" means without comments.
ASK_USER_PASSWORD.
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, that would be a good idea. But it is not urgent, totally fine to do this in some follow-up PR (e.g. here: #50966)
Closes #29880
Connection string syntax:
The connection string must be passed as first argument to clickhouse-client. It can be combined with arbitrary other command line options except
--host(h)
and--port
.Examples
Error has the same root cause as: #50459. Internally, the connection string is converted to command line options.
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
clickhouse-client can now be called with a connection instead of "--host", "--port", "--user" etc.