Commit Graph

21 Commits

Author SHA1 Message Date
Alexey Milovidov
bccd33932b
Update Cargo.toml 2023-11-24 22:59:29 +03:00
Azat Khuzhin
b6bd5ecb19 Fix possible crash of fuzzy finder in client
Refs: https://github.com/lotabout/tuikit/pull/51
Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
2023-11-24 17:46:25 +01:00
Azat Khuzhin
914462d3d5 Disable skim under TSan (Rust does not supports ThreadSanitizer)
Refs: https://doc.rust-lang.org/beta/unstable-book/compiler-flags/sanitizer.html#threadsanitizer
Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
2023-10-09 12:37:40 +02:00
Azat Khuzhin
0b258dda4e Reproducible builds for Rust
From now on cargo will not download anything from the internet during
builds. This step had been moved for docker image builds (via cargo
vendor).

And now cargo inside docker.io/clickhouse/binary-builder will not use
any crates from the internet, so we don't need to add --offline for
cargo commands in cmake (corrosion_import_crate()).

Also the docker build command had been adjusted to allow following
symlinks inside build context, by using tar, this is required for Rust
packages.

Note, that to make proper Cargo.lock that could be vendored I did the
following:
- per-project locks had been removed (since there is no automatic way to
  sync the workspace Cargo.lock with per-project Cargo.lock, since cargo
  update/generate-lockfile will use only per-project Cargo.toml files
  apparently, -Z minimal-versions does not helps either)
- and to generate Cargo.lock with less changes I've pinned version in
  the Cargo.toml strictly, i.e. not 'foo = "0.1"' but 'foo = "=0.1"'
  then the Cargo.lock for workspace had been generated and afterwards
  I've reverted this part.

Plus I have to update the dependencies afterwards, since otherwise there
are conflicts with dependencies for std library. Non trivial.

Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
2023-07-22 22:46:22 +02:00
Azat Khuzhin
380b4ffe2b Reduce dependencies for skim by avoid using default features
By default skim requires cli -> clap -> termcolor -> winapi-util

Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
2023-07-19 12:29:39 +02:00
Raúl Marín
292eec2470 Run cargo update to fix build with nightly 2023-07-03 09:40:36 +02:00
Azat Khuzhin
f1058d2d9d Revert "Disable skim (Rust library) under memory sanitizer" 2023-06-05 09:08:01 +02:00
Alexey Milovidov
53ec091c8d Disable skim (Rust library) under memory sanitizer 2023-06-04 05:00:29 +02:00
Azat Khuzhin
1107988a82 Improve performance of BLAKE3 by 11% by enabling LTO for Rust
LTO in Rust produces multiple definition of `rust_eh_personality' (and
few others), and to overcome this --allow-multiple-definition has been
added.

Query for benchmark:

    SELECT ignore(BLAKE3(materialize('Lorem ipsum dolor sit amet, consectetur adipiscing elit'))) FROM numbers(1000000000) FORMAT `Null`

  upstream           : Elapsed: 2.494 sec. Processed 31.13 million rows, 249.08 MB (12.48 million rows/s., 99.86 MB/s.)
  upstream + rust lto: Elapsed: 13.56 sec. Processed 191.9 million rows, 1.5400 GB (14.15 million rows/s., 113.22 MB/s.)
  llvm BLAKE3        : Elapsed: 3.053 sec. Processed 43.24 million rows, 345.88 MB (14.16 million rows/s., 113.28 MB/s.)

Note, I thought about simply replacing it with BLAKE3 from LLVM, but:
- this will not solve LTO issues for Rust (and in future more libraries
  could be added)
- it makes integrating_rust_libraries.md useless (and there is even blog
  post)

So instead I've decided to add this quirk (--allow-multiple-definition)
to fix builds.

Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
2023-05-06 22:28:56 +02:00
Azat Khuzhin
177c98b6a9 Use "exact" matching for fuzzy search
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>
2023-02-04 14:15:02 +01:00
Azat Khuzhin
780c8ea586 Avoid leaving symbols leftovers on the screen during query fuzzy search
In case of multi-line queries in the history, skim may leave some
symbols on the screen, which looks icky.

Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
2023-02-02 19:31:45 +01:00
Robert Schulze
cfb6feffde
What happens if I remove these 139 lines of code? 2023-01-03 18:35:31 +00:00
Alexey Milovidov
ed3d70f7c0
Merge pull request #44623 from azat/build/rust-fix
Fix rust modules rebuild (previously ignores changes in cargo config.toml)
2022-12-27 17:10:33 +03:00
Azat Khuzhin
72d102d94c Fix rust modules rebuild (previously ignores changes in cargo config.toml)
This leads to the problem when you switch compiler flags, for example:

    $ cmake -DSANITIZE=memory ..
    $ ninja
    $ cmake -DSANITIZE= ..
    $ ninja

And this leads to:

    ld.lld-15: error: undefined symbol: __msan_init
    >>> referenced by lib.rs.cc
    >>>               lib.rs.o:(msan.module_ctor) in archive rust/skim/RelWithDebInfo/lib_ch_rust_skim_rust.a

Reported-by: @alexey-milovidov
Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
2022-12-27 10:35:50 +01:00
Azat Khuzhin
4d17510fca Use already written part of the query for fuzzy search (pass to skim)
Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
2022-12-26 14:52:38 +01:00
Azat Khuzhin
cf0e0436be skim: do not panic if terminal is not available
Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
2022-12-14 21:01:48 +01:00
Azat Khuzhin
e7c5b48d84 rust: fix buidling modules with CMAKE_BUILD_TYPE in a different case
Before this patch corrosion requires that CMAKE_BUILD_TYPE matches the
CMAKE_CONFIGURATION_TYPES, which is
"RelWithDebInfo;Debug;Release;MinSizeRel", so that said, that if you
were using CMAKE_BUILD_TYPE=debug, it will not work.

Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
2022-12-14 20:58:34 +01:00
Azat Khuzhin
f8c17d4a66 rust: reuse RUST_CXXFLAGS for skim
Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
2022-12-14 20:58:17 +01:00
Azat Khuzhin
82aaad61aa Integrate skim into the client/local
Note, that it can the fail the client if the skim itself will fail,
however I haven't seen it panicd, so let's try.

P.S. about adding USE_SKIM into configure header instead of just compile
option for target, it is better, because it allows not to recompile lots
of C++ headers, since we have to add skim library as PUBLIC. But anyway
this will be resolved in a different way, but separatelly.

Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
2022-12-14 20:57:41 +01:00
Azat Khuzhin
67fa185611
Revert "Builtin skim" 2022-12-14 17:17:19 +03:00
Azat Khuzhin
de58e9c02d Integrate skim into the client/local
Note, that it can the fail the client if the skim itself will fail,
however I haven't seen it panicd, so let's try.

P.S. about adding USE_SKIM into configure header instead of just compile
option for target, it is better, because it allows not to recompile lots
of C++ headers, since we have to add skim library as PUBLIC.

Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
2022-12-11 15:52:00 +01:00