Sorry for the clickbaity title. This is about static method
ConnectionTimeouts::getHTTPTimeouts(). It was be declared in header
IO/ConnectionTimeouts.h, and defined in header
IO/ConnectionTimeoutsContext.h (!). This is weird and caused issues with
linking on s390x (##45520). There was an attempt to fix some
inconsistencies (#45848) but neither did @Algunenano nor me at first
really understand why the definition is in the header.
Turns out that ConnectionTimeoutsContext.h is only #include'd from
source files which are part of the normal server build BUT NOT part of
the keeper standalone build (which must be enabled via CMake
-DBUILD_STANDALONE_KEEPER=1). This dependency was not documented and as
a result, some misguided workarounds were introduced earlier, e.g.
0341c6c54b
The deeper cause was that getHTTPTimeouts() is passed a "Context". This
class is part of the "dbms" libary which is deliberately not linked by
the standalone build of clickhouse-keeper. The context is only used to
read the settings and the "Settings" class is part of the
clickhouse_common library which is linked by clickhouse-keeper already.
To resolve this mess, this PR
- creates source file IO/ConnectionTimeouts.cpp and moves all
ConnectionTimeouts definitions into it, including getHTTPTimeouts().
- breaks the wrong dependency by passing "Settings" instead of "Context"
into getHTTPTimeouts().
- resolves the previous hacks
Right now fuzzy search is too smart for SQL, it even takes into account
the case, which should not be accounted (you don't want to type "SELECT"
instead of "select" to find the query).
And to tell the truth, I think too smart fuzzy searching for SQL queries
is not required, and is only harming.
Exact matching seems better algorithm for SQL, it is not 100% exact, it
splits by space, and apply separate matcher actually for each word.
Note, that if you think that "space is not enough" as the delimiter,
then you should first know that this is the delimiter only for the
input query, so to match "system.query_log" you can use "sy qu log"
(also you can disable exact mode by prepending "'" char).
But it ignores the case by default, and the behaviour what is expected
from the CaseMatching::Ignore.
TL;DR;
Just for the history I will describe what had been tried.
At first I tried CaseMatching::Ignore - it does not helps for
SkimV1/SkimV2/Clangd matches.
So I converted lines from the history and input query, to the lower
case. However this does not work for UPPER CASE, since only initial
portion of the query had been converted to the lower.
Then I've looked into skim/fuzzy-matcher crates code, and look for the
reason why CaseMatching::Ignore does not work, and found that there is
still a penalty for case mismatch, but there is no way to pass it from
the user code, so I've tried guerrilla to monkey patch the library's
code and it works:
// Avoid penalty for case mismatch (even with CaseMatching::Ignore)
let _guard = guerrilla::patch0(SkimScoreConfig::default, || {
let score_match = 16;
let gap_start = -3;
let gap_extension = -1;
let bonus_first_char_multiplier = 2;
return SkimScoreConfig{
score_match,
gap_start,
gap_extension,
bonus_first_char_multiplier,
bonus_head: score_match / 2,
bonus_break: score_match / 2 + gap_extension,
bonus_camel: score_match / 2 + 2 * gap_extension,
bonus_consecutive: -(gap_start + gap_extension),
// penalty_case_mismatch: gap_extension * 2,
penalty_case_mismatch: 0,
};
});
But this does not sounds like a trivial code, so I decided, to look
around, and realized that "exact" matching should do what is required
for the completion of queries (at least from my point of view).
Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
