-
Notifications
You must be signed in to change notification settings - Fork 884
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 & SQL Value Suggestion #8275
base: main
Are you sure you want to change the base?
[Auto Suggest] PPL & SQL Value Suggestion #8275
Conversation
Signed-off-by: Paul Sebastian <paulstn@amazon.com>
Signed-off-by: Paul Sebastian <paulstn@amazon.com>
Signed-off-by: Paul Sebastian <paulstn@amazon.com>
❌ 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 #8275 +/- ##
==========================================
- Coverage 60.85% 60.82% -0.04%
==========================================
Files 3793 3793
Lines 90368 90373 +5
Branches 14181 14184 +3
==========================================
- Hits 54997 54970 -27
- Misses 31893 31928 +35
+ Partials 3478 3475 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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@dev-dsk-paulstn-2b-a4d3f011.us-west-2.amazon.com>
Signed-off-by: Paul Sebastian <paulstn@dev-dsk-paulstn-2b-a4d3f011.us-west-2.amazon.com>
Signed-off-by: Paul Sebastian <paulstn@dev-dsk-paulstn-2b-a4d3f011.us-west-2.amazon.com>
❌ 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. |
1 similar comment
❌ 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. |
Signed-off-by: Paul Sebastian <paulstn@dev-dsk-paulstn-2b-a4d3f011.us-west-2.amazon.com>
❌ 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. |
5 similar comments
❌ 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. |
❌ 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. |
❌ 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. |
Signed-off-by: Paul Sebastian <paulstn@dev-dsk-paulstn-2b-a4d3f011.us-west-2.amazon.com>
src/plugins/data/public/antlr/opensearch_ppl/grammar/OpenSearchPPLParser.g4
Show resolved
Hide resolved
src/plugins/data/public/antlr/opensearch_ppl/grammar/OpenSearchPPLParser.g4
Show resolved
Hide resolved
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.
seems it's simplifying some of the comparison grammars, any context on why they are needed?
@@ -291,7 +291,7 @@ expression | |||
|
|||
predicate | |||
: expressionAtom # expressionAtomPredicate | |||
| left = predicate comparisonOperator right = predicate # binaryComparisonPredicate | |||
| left = predicate comparisonOperator right = constant # binaryComparisonPredicate |
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 do we need this change?
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.
This line, along with line 307/308, was changed so that values don't get suggested for the left side of a predicate, while the right side doesn't have field names suggested and only values suggested instead. Although having a constant on the left and a field on the right does get parsed, suggesting it is practically useless/ information overload. There would be too many suggestions and it doesn't make sense to show every single possibility.
src/plugins/data/public/autocomplete/providers/query_suggestion_provider.ts
Show resolved
Hide resolved
export type ValueSuggestionsGetFn = (args: ValueSuggestionsGetFnArgs) => Promise<any[]>; | ||
export type ValueSuggestionsGetFn = ( | ||
args: ValueSuggestionsGetFnArgs | ValueSuggestionsSQLGetFnArgs | ||
) => Promise<any[]>; |
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. might be better to put args type and return type as generic, to allow extending and keeps autocomplete generic to what languages are used
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'm not sure I fully understand what making the args or the return type generic would accomplish. Since this file is meant to include the implementation for functions that would do the value completion, they require specific inputs that may not be related to the output.
To keep autocomplete generic to the languages used, should I just change the name of the function to move away from "SQL"?
Just summarize some of the concerns for this PR that I feel we need to address: Grammar Changes: Test Coverage: Value Suggestions: Unaddressed Comments: |
Description
Only done for opensearch SQL + PPL
SQL:
Screen.Recording.2024-10-09.at.11.51.51.AM.mov
PPL:
Screen.Recording.2024-10-09.at.11.42.04.AM.mov
src/plugins/data/public/antlr/*
For PPL & SQL, uses the SQL query
to find the most popular column values to display within autocomplete whenever a value should be suggested.
src/plugins/data/server/ui_settings.ts
There were two UI settings added.
query:enhancements:suggestValues
is a boolean that determines if value suggestion will be usedquery:enhancements:suggestValuesLimit
is a number that specifies how many values should be queried, defaulting to 200src/plugins/data/public/autocomplete/autocomplete_service.ts
The ability to add a value suggestion provided was included in this pr, but isn't in use so far. Creating a value suggestion provider that would make the api call (with all of the same parameters that exist right now) could be done but would require a lot of complexity for what is essentially done in 10 lines today.
This PR also contains various fixes and quality of life updates within SQL & PPL suggestions.
Issues Resolved
Screenshot
Testing the changes
Field level security testing:
Screen.Recording.2024-10-21.at.2.22.46.PM.mov
Field level masking testing:
Screen.Recording.2024-10-21.at.2.09.44.PM.mov
Changelog
Check List
yarn test:jest
yarn test:jest_integration