-
Notifications
You must be signed in to change notification settings - Fork 894
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 <[email protected]>
Signed-off-by: Paul Sebastian <[email protected]>
Signed-off-by: Paul Sebastian <[email protected]>
❌ 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.78% 60.67% -0.12%
==========================================
Files 3798 3798
Lines 90701 90862 +161
Branches 14284 14321 +37
==========================================
- Hits 55133 55130 -3
- Misses 32068 32232 +164
Partials 3500 3500
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 <[email protected]>
Signed-off-by: Paul Sebastian <[email protected]>
Signed-off-by: Paul Sebastian <[email protected]>
Signed-off-by: Paul Sebastian <[email protected]>
Signed-off-by: Paul Sebastian <[email protected]>
Signed-off-by: Paul Sebastian <[email protected]>
Signed-off-by: Paul Sebastian <[email protected]>
❌ 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 <[email protected]>
❌ 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 <[email protected]>
src/plugins/data/public/antlr/opensearch_ppl/grammar/OpenSearchPPLParser.g4
Outdated
Show resolved
Hide resolved
src/plugins/data/public/antlr/opensearch_ppl/grammar/OpenSearchPPLParser.g4
Outdated
Show resolved
Hide resolved
src/plugins/data/public/antlr/opensearch_sql/code_completion.ts
Outdated
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/antlr/opensearch_ppl/code_completion.ts
Outdated
Show resolved
Hide resolved
src/plugins/data/public/autocomplete/providers/query_suggestion_provider.ts
Outdated
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: |
Signed-off-by: Paul Sebastian <[email protected]>
Signed-off-by: Paul Sebastian <[email protected]>
…ators, values Signed-off-by: Paul Sebastian <[email protected]>
// : left = valueExpression binaryOperator = (STAR | DIVIDE | MODULE) right = valueExpression # binaryArithmetic | ||
// | left = valueExpression binaryOperator = (PLUS | MINUS) right = valueExpression # binaryArithmetic | ||
: primaryExpression # valueExpressionDefault | ||
: left = valueExpression binaryOperator = (STAR | DIVIDE | MODULE) right = valueExpression # binaryArithmetic |
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.
NOTE: this is reverting back a change made in a previous pr, to go back to the original grammar.
Signed-off-by: Paul Sebastian <[email protected]>
Signed-off-by: Paul Sebastian <[email protected]>
Description
Only done for opensearch SQL + PPL. For SQL, value completion only occurs inside of a binary comparison predicate (for example
column = value
, and for an in-predicate with the keywordIN
(e.g.column IN ("value1", "value2", "value3")
). These are the only cases in which it makes sense for SQL to suggest values for a particular column.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