-
Notifications
You must be signed in to change notification settings - Fork 892
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
[Auto Suggest] PPL autocomplete and interface changes #7932
Conversation
❌ Empty Changelog SectionThe Changelog section in your PR description is empty. Please add a valid changelog entry or entries. If you did add a changelog entry, check to make sure that it was not accidentally included inside the comment block in the Changelog section. |
❌ Empty Changelog SectionThe Changelog section in your PR description is empty. Please add a valid changelog entry or entries. If you did add a changelog entry, check to make sure that it was not accidentally included inside the comment block in the Changelog section. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7932 +/- ##
==========================================
- Coverage 64.33% 60.97% -3.36%
==========================================
Files 3677 3684 +7
Lines 81200 87038 +5838
Branches 12944 13376 +432
==========================================
+ Hits 52240 53074 +834
- Misses 25749 30750 +5001
- Partials 3211 3214 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
"generate:opensearchsqlantlr": "./node_modules/antlr4ng-cli/index.js -Dlanguage=TypeScript -o ./src/plugins/data/public/antlr/opensearch_sql/.generated -visitor -no-listener -Xexact-output-dir ./src/plugins/data/public/antlr/opensearch_sql/grammar/OpenSearchSQLLexer.g4 ./src/plugins/data/public/antlr/opensearch_sql/grammar/OpenSearchSQLParser.g4", | ||
"generate:opensearchpplantlr": "./node_modules/antlr4ng-cli/index.js -Dlanguage=TypeScript -o ./src/plugins/data/public/antlr/opensearch_ppl/.generated -visitor -no-listener -Xexact-output-dir ./src/plugins/data/public/antlr/opensearch_ppl/grammar/OpenSearchPPLLexer.g4 ./src/plugins/data/public/antlr/opensearch_ppl/grammar/OpenSearchPPLParser.g4" |
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.
Cant we have a single command to generate all the antlr grammer? Alternatively, not for this PR by we can also use the osd
script to add thsi as a command there to not bloat up the package.json commands. e.g. yarn osd antlr-grammar
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.
We could, but I'd assume more granularity would be better when needing to regenerate the antlr files. Do you think we should make this change now or handle it when moving it to the osd script?
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.
+1 i think in @mengweieric PR made a similiar comment. Like post 2.17, we should have something in the scripts
folder that lets us run yarn antlr:generateGrammar --sql
or something
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.
Nice!
autoComplete.addQuerySuggestionProvider('SQL', getSQLSuggestions); | ||
autoComplete.addQuerySuggestionProvider('kuery', getDQLSuggestions); | ||
autoComplete.addQuerySuggestionProvider('PPL', getPPLSuggestions); |
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.
nit: we should move adding the suggestion provider for PPL and SQL to the query enhancements plugin since registering the language happens there as well. Making the suggestion provider also be a part of the language service's register function would bring all these language related features into one place too.
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.
Can we add some tests for this?
…d move suggestion provider registration location (opensearch-project#7758) * add table/source as prefix to suggested fields Signed-off-by: Eric <menwe@amazon.com> * add type to column Signed-off-by: Eric <menwe@amazon.com> * move registeration to osd/monaco Signed-off-by: Eric <menwe@amazon.com> * add detail Signed-off-by: Eric <menwe@amazon.com> * Changeset file for PR opensearch-project#7758 created/updated --------- Signed-off-by: Eric <menwe@amazon.com> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>
* add initial ppl autocomplete Signed-off-by: Eric <menwe@amazon.com> * untrack .antlr files Signed-off-by: Eric <menwe@amazon.com> * ignore intermediate grammar auto-gen files Signed-off-by: Eric <menwe@amazon.com> * add rules and related functionalities Signed-off-by: Eric <menwe@amazon.com> * Changeset file for PR opensearch-project#7810 created/updated * Changeset file for PR opensearch-project#7810 created/updated * minor comment cleanning Signed-off-by: Eric <menwe@amazon.com> * add ppl generation command Signed-off-by: Eric <menwe@amazon.com> * add rules Signed-off-by: Eric <menwe@amazon.com> * correct typo Signed-off-by: Eric <menwe@amazon.com> * fix inserting text issue Signed-off-by: Eric <menwe@amazon.com> * remove colon for PPL field Signed-off-by: Eric <menwe@amazon.com> --------- Signed-off-by: Eric <menwe@amazon.com> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>
Signed-off-by: Paul Sebastian <paulstn@amazon.com>
Signed-off-by: Paul Sebastian <paulstn@amazon.com>
Signed-off-by: Paul Sebastian <paulstn@amazon.com>
Signed-off-by: Paul Sebastian <paulstn@amazon.com>
Signed-off-by: Paul Sebastian <paulstn@amazon.com>
Signed-off-by: Paul Sebastian <paulstn@amazon.com>
…implemented dql value WS suggs Signed-off-by: Paul Sebastian <paulstn@amazon.com>
72a0e63
to
949e046
Compare
Signed-off-by: Paul Sebastian <paulstn@amazon.com>
18427b6
to
98058b3
Compare
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.
lgtm, just questions
@@ -0,0 +1,2 @@ | |||
feat: | |||
- Add OpenSearch PPL autocomplete to discover 2.0 with query enhancements ([#7810](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/7810)) |
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.
np. there are two changelogs but non of them is for #7932?
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.
Just updated it, added more details :)
[/\/.*$/, 'comment'], | ||
|
||
[/".*?"/, 'string'], | ||
[/'.*?'/, 'constant'], |
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.
curious how are these defined? i don't think PPL supports comments, and the string definition is a bit different from sql
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 catch, i'm not sure why that's in there. best guess is it was part of the boilerplate. i'll look it over and make the needed 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.
is the boilerplate generated by something or copied from some pre-existing code? or is this written from scratch by this PR? asking because looks like the rules are from some standard but might not completely apply to PPL syntax. probably won't break but just curious
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 just mean copied from the other monarch lexer rule files, i agree on it not completely applying to ppl
range, | ||
})) | ||
? suggestions | ||
.filter((s) => 'detail' in s) // remove suggestion not of type MonacoCompatible |
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.
what is detail?
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.
detail is a field in a possible suggestion that is a label on what that suggestion is. for example, if we're suggesting a field of type geo_point, it will read Field: geo_point
next to the suggestion. the reason it's being used here is as a type guard since detail is a monaco specific suggestion field
Signed-off-by: Paul Sebastian <paulstn@amazon.com>
2de8689
to
6754d6c
Compare
* [Autocomplete - SQL] Minor interface change to add suggestion type and move suggestion provider registration location (#7758) * add table/source as prefix to suggested fields Signed-off-by: Eric <menwe@amazon.com> * add type to column Signed-off-by: Eric <menwe@amazon.com> * move registeration to osd/monaco Signed-off-by: Eric <menwe@amazon.com> * add detail Signed-off-by: Eric <menwe@amazon.com> * Changeset file for PR #7758 created/updated --------- Signed-off-by: Eric <menwe@amazon.com> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com> * [Autocomplete] PPL Autocomplete (#7810) * add initial ppl autocomplete Signed-off-by: Eric <menwe@amazon.com> * untrack .antlr files Signed-off-by: Eric <menwe@amazon.com> * ignore intermediate grammar auto-gen files Signed-off-by: Eric <menwe@amazon.com> * add rules and related functionalities Signed-off-by: Eric <menwe@amazon.com> * Changeset file for PR #7810 created/updated * Changeset file for PR #7810 created/updated * minor comment cleanning Signed-off-by: Eric <menwe@amazon.com> * add ppl generation command Signed-off-by: Eric <menwe@amazon.com> * add rules Signed-off-by: Eric <menwe@amazon.com> * correct typo Signed-off-by: Eric <menwe@amazon.com> * fix inserting text issue Signed-off-by: Eric <menwe@amazon.com> * remove colon for PPL field Signed-off-by: Eric <menwe@amazon.com> --------- Signed-off-by: Eric <menwe@amazon.com> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com> * small interface updates Signed-off-by: Paul Sebastian <paulstn@amazon.com> * small type update Signed-off-by: Paul Sebastian <paulstn@amazon.com> * make inclusion for pipe, comma, and equal tokens Signed-off-by: Paul Sebastian <paulstn@amazon.com> * refactor and generalize field fetcher for dql and ppl, including details Signed-off-by: Paul Sebastian <paulstn@amazon.com> * use field fetching util for sql and update sugg detail for dql Signed-off-by: Paul Sebastian <paulstn@amazon.com> * detail for ppl Signed-off-by: Paul Sebastian <paulstn@amazon.com> * create range parameter to help identify suggestions with whitespace, implemented dql value WS suggs Signed-off-by: Paul Sebastian <paulstn@amazon.com> * single line editor overflow initial override for sugg window Signed-off-by: Paul Sebastian <paulstn@amazon.com> * update dql tests to account for details and value ranges Signed-off-by: Paul Sebastian <paulstn@amazon.com> --------- Signed-off-by: Eric <menwe@amazon.com> Signed-off-by: Paul Sebastian <paulstn@amazon.com> Co-authored-by: Eric Wei <menwe@amazon.com> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com> (cherry picked from commit 0245540) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
* [Autocomplete - SQL] Minor interface change to add suggestion type and move suggestion provider registration location (#7758) * add table/source as prefix to suggested fields * add type to column * move registeration to osd/monaco * add detail * Changeset file for PR #7758 created/updated --------- * [Autocomplete] PPL Autocomplete (#7810) * add initial ppl autocomplete * untrack .antlr files * ignore intermediate grammar auto-gen files * add rules and related functionalities * Changeset file for PR #7810 created/updated * Changeset file for PR #7810 created/updated * minor comment cleanning * add ppl generation command * add rules * correct typo * fix inserting text issue * remove colon for PPL field --------- * small interface updates * small type update * make inclusion for pipe, comma, and equal tokens * refactor and generalize field fetcher for dql and ppl, including details * use field fetching util for sql and update sugg detail for dql * detail for ppl * create range parameter to help identify suggestions with whitespace, implemented dql value WS suggs * single line editor overflow initial override for sugg window * update dql tests to account for details and value ranges --------- (cherry picked from commit 0245540) Signed-off-by: Eric <menwe@amazon.com> Signed-off-by: Paul Sebastian <paulstn@amazon.com> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: Eric Wei <menwe@amazon.com> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>
Description
Adds PPL Autocomplete using ANTLR and C3 Code Completion.
Also makes interface changes to include a 'details' aspect to the suggestions which will include a relevant label for each one. Another interface change allows a suggestion to specify the range of characters that it should replace.
Note: Every file worth reviewing is under
src/plugins/data/public
Issues Resolved
Screenshot
Testing the changes
Changelog
Check List
yarn test:jest
yarn test:jest_integration