-
Notifications
You must be signed in to change notification settings - Fork 7k
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
Feature/suggestions #2447
Feature/suggestions #2447
Conversation
dbms/src/Client/Completion.cpp
Outdated
return h; | ||
} | ||
|
||
int init_hash_table(HashTable *ht, size_t size) |
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.
From what source this code was copy-pasted?
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 is from https://github.com/mysql/mysql-server/blob/8.0/client/completion_hash.cc. I've put source in Completion.h file.
dbms/src/Client/Completion.cpp
Outdated
|
||
namespace Completion | ||
{ | ||
static uint hashpjw(const char *arKey, uint nKeyLength) |
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.
Bad hash function.
This is perfectly Ok, but care should be taken when there are huge amount of tables. For example, it is a typical issue for MySQL CLI: when you have huge number of tables, it will startup slowly. |
dbms/src/Client/Completion.h
Outdated
|
||
#include <sys/types.h> | ||
|
||
//All of functionality for hash was taken from mysql-server project from completion_hash.cpp file |
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.
Missing license. The license is most likely to be incompatible.
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.
Yeah, its only one way compatible. Will rewrite completion hash.
dbms/src/Client/QueryParts.h
Outdated
{(char *)"Expression"}, | ||
{(char *)"Set"}, | ||
//FUNCTIONS | ||
{(char *)"plus"}, |
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 will be difficult to support unless we will use FunctionFactory directly (or query system.functions
table for looser coupling).
dbms/programs/client/Client.cpp
Outdated
std::vector<Block> blocks; | ||
|
||
//preload all functions | ||
sendQuery("SELECT name FROM system.functions", blocks); |
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.
Aggregation function can also have suffixes
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.
Wont it be too much repetition and will pollute suggestions? For that reason i didn't want to include combinators, but can add them regardless.
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.
May be you right.
dbms/programs/client/Client.cpp
Outdated
@@ -171,6 +242,9 @@ class Client : public Poco::Util::Application | |||
/// External tables info. | |||
std::list<ExternalTable> external_tables; | |||
|
|||
/// Suggestion limit for how many databases and tables to fetch | |||
int suggestion_limit = 100; |
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.
why so low by default? For databases that can be reasonable, for tables - a bit too low
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.
@filimonov what limit will you suggest? twice the amount? not really sure :(
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 would say that having dozens of tables is quite common. Clickhouse have 24 system tables in default distribution. At that point I have 93 tables on my local clickhouse. And it can be very irritating when autocompletion works for "almost" every table.
Limit? May be 256 ? Even you you will have 64 columns per table, and 32 chars per column name it will give 512 Kb of memory, which is not a big number nowadays. Also 256 is more 'round' number :)
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.
@filimonov Bumped limit to 256. One can always adjust it to higher/lower limits by adding --suggestion_limit= to your CLI client on startup ex. ./clickhouse client --suggestion_limit=512
dbms/programs/client/Client.cpp
Outdated
delete implodedName; | ||
} | ||
} | ||
delete query; |
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.
Also dictionary autocomplete is VERY helpful.
Check that:
https://github.com/filimonov/chc/blob/8bb7b4a1a7917ff5cb75f84e7b76091463f23ecc/autocomplete.go#L196
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.
@filimonov Added dictionary name and attribute name as part of completion. Can add whole dictGetT() template as well. But I think there will be problems with complex keys.
|
||
QUERYPART queryParts[] = { | ||
// CREATE DATABASE, TABLE, VIEW | ||
{(const char *)"CREATE"}, |
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.
Good. You can also compare with https://github.com/filimonov/chc/blob/8bb7b4a1a7917ff5cb75f84e7b76091463f23ecc/autocomplete.go#L12
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.
@filimonov Compared. Added missing query keywords
dbms/src/Client/QueryParts.h
Outdated
{(const char *)"Tuple"}, | ||
{(const char *)"Nested"}, | ||
{(const char *)"Expression"}, | ||
{(const char *)"Set"}, |
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.
Nullable
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.
@filimonov added Nullable as well)
TODO: It shouldn't work when you paste a text with tabs in terminal. |
@alexey-milovidov it doesn't trigger completion on paste, but will clear tabs from pasted text (at least this is behavior in my env) If it shows different behavior I will investigate it. Also not sure why builds are failing from time to time. |
@alexey-milovidov Are there other changes that are needed ? |
Yes, it requires multiple changes. I will address them later. |
Now we have system tables for all required entities except language keywords. |
@alexey-milovidov updated PR according to information from new system tables. |
@alexey-milovidov sorry to bother, but just want to know if there are any more changes that are needed? |
It's in my queue. We have to rewrite code to modern style Implement max_block_size for Check what happens when you copy-paste a query with tab characters. Check and ensure compatibility of new client with old servers (completion may not work but client should work as before). It is possible to implement in about three days. |
…ou type SELECT DIS<TAB> #2447
sendQuery( | ||
"SELECT name FROM system.functions" | ||
" UNION ALL " | ||
"SELECT name FROM system.table_engines" |
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.
Will client be able to work with older versions of server, which doesn't have those tables?
The diff in this pull request is outdated. See master branch. |
I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/?lang=en
This PR adds autocomplete to client for base queries, types, engines, formats, and functions except combinators.
Please bear with me on this PR since i am not very familiar with C++ language and its toolchain. My apologies in advance.
P.S Can I use Clients sendQuery method for gathering data about dbs, tables?