Skip to content

#9056 Add YQL keywords suggestions to YDB CLI #12386

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

Merged
merged 55 commits into from
Feb 26, 2025

Conversation

vityaman
Copy link
Contributor

@vityaman vityaman commented Dec 8, 2024

Changelog entry

Add YQL keywords suggestions to YDB CLI

Changelog category

  • Improvement

Additional information

Copy link

github-actions bot commented Dec 8, 2024

Hi! Thank you for contributing!
The tests on this PR will run after a maintainer adds an ok-to-test label to this PR manually. Thank you for your patience!

@vityaman
Copy link
Contributor Author

vityaman commented Dec 8, 2024

After merge will need to

  • remove IsAnsiQuery and use QuerySyntaxMode instead

  • share some common things between highlighting and completion (like function names)

  • extract yql color schema to separate from hightlighting schema

  • format all files at commands/interactive with ya style

  • rearrange packages to have something like this

|- commands/interactive
   |- interactive_cli.(h|c)pp
   |- line_reader.(h|c)pp
   |- yql
     |- highlighting
        |- yql_highlighting_ut.cpp
        |- yql_highlighting.(h|c)pp
        |- yql_color_schema.(h|c)pp
        |- yql_position.(h|c)pp
     |- completion
        |- ut
           |- alter
              |- alter_group_ut.cpp
              ...
              |- alter_ut.cpp
           |- create
              |- create_external_ut.cpp
              ...
              |- create_ut.cpp
           ...
        |- c3_engine.h
        |- string_util.(c|h)pp
        |- yql_completion.(c|h)pp
        |- yql_namespace.(c|h)pp
     |- yql_syntax.(c|h)pp
  • translate more tests from the WebSql, maybe design some testing framework to compare suggestions quality

  • support simple names completion with SchemaList : YQLNamespace decorated with CachedYQLNamespace

  • explore behaviour differences with WebSql

@vityaman vityaman marked this pull request as ready for review December 17, 2024 17:56
@vityaman vityaman requested a review from a team as a code owner December 17, 2024 17:56
@vityaman
Copy link
Contributor Author

vityaman commented Dec 17, 2024

Found some differences with WebSQL on query prefixes:

alter sequence
grant alter
grant create
explain reduce
explain restore
select set

will explore them somewhen

@vityaman
Copy link
Contributor Author

vityaman commented Feb 24, 2025

@vitstn, внес изменения, следуя плану:

  1. Определить top-level интерфейс автодополнения.

  2. Тестировать именно top-level интерфейс.

Теперь

  1. В файле sql_complete.h определен top-level интерфейс автодополнения.

Я все-таки решил настоять на использовании ECandidateKind в интерфейсе, так как это дает улушенную типобезопасность. При этом, для ECandidateKind определен Out<NSQLComplete::ECandidateKind>, поэтому его просто конвертировать в TString, который мы потенциально можем отдавать уже из, например, GRPC API.

Также Complete возвращает TCompletedToken. Это нужно, чтобы автокомплит сам имел право выделять тот суффикс префикса запроса, который он считает переписываемым в данный момент. Это может быть полезно, чтобы автодополнять какие-то более сложные конструкции, состоящие из нескольких лексем. Ну и вообще, разные редакторы могут по-разному определять word break.

Я же правильно понимаю, что мы не поддерживаем SQLv0?

  1. Done.

Пока не сортирую кандидатов в тестах, так как полагаюсь на текущее "ранжирование" просто сортировкой пар (ECandidateKind, TString) в реализации. Когда появится другой алгоритм ранжирования, добавлю сортировку в тесты, а ту выкинем из реализации.

Помню про твой совета о том, что лучше для тестирования определять ToString для сравниваемых данных, чтобы более выразительные ассерты писать, но решил все-таки оставить TVector<TCandidate>, так как оно и так выглядит достаточно читаемо.

  1. Выделил внутренний компонент для получения контекста запроса в заданной позиции, который назвал ISqlContextInference (файл: sql_context.h).

Лучше названия не придумал. Другие компоненты выделять не стал. Ранжирование пока что тривиальное, а получения имен еще не нужно.

vitstn
vitstn previously approved these changes Feb 24, 2025
nepal
nepal previously approved these changes Feb 24, 2025
@vityaman vityaman dismissed stale reviews from nepal and vitstn via 98eb532 February 26, 2025 09:54
@@ -25,7 +25,7 @@ COPY_FILE(
RUN_ANTLR4(
${SQL_GRAMMAR}
-no-listener
-package NALPAnsiAntlr4
-package NALAAnsiAntlr4
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seg. Fault происходил из-за коллизии имен antlr_ast и proto_ast и выбора неверной реализации. Мы не смогли расшифровать ALP, но я интерпретировал его как ANTLR Library Protobuf, поэтому по аналогии переименовал в ANTLR Library ANTLR. Короче, последняя буква этой аббревиатуры будет обозначать таргет ANTLR: proto_ast или antlr_ast.

@pnv1 pnv1 added the ok-to-test Special label used to approve a PR for testing on our infrastructure label Feb 26, 2025
@github-actions github-actions bot removed the ok-to-test Special label used to approve a PR for testing on our infrastructure label Feb 26, 2025
Copy link

github-actions bot commented Feb 26, 2025

2025-02-26 10:29:20 UTC Pre-commit check linux-x86_64-relwithdebinfo for 5ec3988 has started.
2025-02-26 10:29:25 UTC Artifacts will be uploaded here
2025-02-26 10:31:50 UTC ya make is running...
🟢 2025-02-26 10:49:49 UTC Tests successful.

Test history | Ya make output | Test bloat

TESTS PASSED ERRORS FAILED SKIPPED MUTED?
1065 1046 0 0 10 9

🟢 2025-02-26 10:50:03 UTC Build successful.
🟡 2025-02-26 10:50:13 UTC ydbd size 2.1 GiB changed* by +179.9 KiB, which is >= 100.0 KiB vs main: Warning

ydbd size dash main: 0c1ebc0 merge: 5ec3988 diff diff %
ydbd size 2 263 580 664 Bytes 2 263 764 888 Bytes +179.9 KiB +0.008%
ydbd stripped size 477 118 136 Bytes 477 143 000 Bytes +24.3 KiB +0.005%

*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation

Copy link

github-actions bot commented Feb 26, 2025

2025-02-26 10:30:37 UTC Pre-commit check linux-x86_64-release-asan for 5ec3988 has started.
2025-02-26 10:30:52 UTC Artifacts will be uploaded here
2025-02-26 10:33:14 UTC ya make is running...
🟢 2025-02-26 10:46:21 UTC Tests successful.

Test history | Ya make output | Test bloat

TESTS PASSED ERRORS FAILED SKIPPED MUTED?
648 635 0 0 6 7

🟢 2025-02-26 10:46:30 UTC Build successful.
🟡 2025-02-26 10:46:43 UTC ydbd size 3.7 GiB changed* by +355.9 KiB, which is >= 100.0 KiB vs main: Warning

ydbd size dash main: 1be0464 merge: 5ec3988 diff diff %
ydbd size 3 942 835 648 Bytes 3 943 200 112 Bytes +355.9 KiB +0.009%
ydbd stripped size 1 374 042 608 Bytes 1 374 169 648 Bytes +124.1 KiB +0.009%

*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation

@pnv1 pnv1 merged commit 8d25dd3 into ydb-platform:main Feb 26, 2025
15 checks passed
lberserq pushed a commit to lberserq/ydb that referenced this pull request Mar 3, 2025
blinkov pushed a commit that referenced this pull request Mar 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants