-
Notifications
You must be signed in to change notification settings - Fork 917
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] DQL Updates #7593
[Auto Suggest] DQL Updates #7593
Conversation
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 #7593 +/- ##
==========================================
+ Coverage 64.17% 64.19% +0.01%
==========================================
Files 3659 3658 -1
Lines 80796 80814 +18
Branches 12866 12874 +8
==========================================
+ Hits 51848 51875 +27
+ Misses 25769 25757 -12
- Partials 3179 3182 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
❌ 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. |
2 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. |
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]>
Signed-off-by: Paul Sebastian <[email protected]>
Signed-off-by: Paul Sebastian <[email protected]>
…groupings Signed-off-by: Paul Sebastian <[email protected]>
Signed-off-by: Paul Sebastian <[email protected]>
…cases Signed-off-by: Paul Sebastian <[email protected]>
{ text: '_index', type: 3, insertText: '_index: ', end: -1, start: -1 }, | ||
{ text: '_score', type: 3, insertText: '_score: ', end: -1, start: -1 }, | ||
{ text: '_source', type: 3, insertText: '_source: ', end: -1, start: -1 }, | ||
{ text: '_type', type: 3, insertText: '_type: ', end: -1, start: -1 }, |
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.
should constants used for test be in a *.test.ts file?
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.
My assumption was that since the file didn't contain any tests, it shouldn't have that extension. I couldn't find any precedence for constants being used exclusively for tests, do you know if there is another way of handling these?
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.
ah @joshuali925 similar comment https://github.com/opensearch-project/OpenSearch-Dashboards/pull/7593/files#r1732297731
you can use mock
or just include this in the test.
exporting public fieldNameSuggestions could get confusing for other plugins since now any plugin can access it and this will get compiled down with the release distribution.
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.
that makes sense, i'll move everything here into those tests
seeing some failures in the ci. also qq, i know @mengweieric was working on a feature branch for his updates. does these updates have those and account for changes related to that work. since it has clean ups? |
{ text: '_score', type: 3, insertText: '_score: ' }, | ||
{ text: '_source', type: 3, insertText: '_source: ' }, | ||
{ text: '_type', type: 3, insertText: '_type: ' }, | ||
export const fieldNameSuggestions: Array<{ |
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 shouldn't store test values in constants file. constants will get bundled up and will increase the release distribution size. test files do not get compiled with the distribution.
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.
out of curiosity, could you point me to where its specified which files don't get compiled in the distribution? would it just be .test.ts files, or are there other files that wouldn't be included?
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.
there are some hard coded ignores in the repo, like
OpenSearch-Dashboards/packages/osd-pm/src/utils/targeted_build.ts
Lines 58 to 59 in 25c23db
'--ignore', | |
'**/*.test.ts,**/*.test.tsx', |
@@ -63,7 +61,7 @@ export class AutocompleteService { | |||
const provider = this.querySuggestionProviders.get(language); | |||
|
|||
if (provider) { | |||
return provider({ core: this.core, ...args }); | |||
return provider(args); |
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 this a breaking 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.
no, i'm reverting a change that i made previously with dql autocomplete. this was before i knew about a clear way to get to the http core util. but now since we're using services, its redundant and so i put it back the way it was.
Signed-off-by: Paul Sebastian <[email protected]>
The updates here and the interface changes Eric made in the feature branch don't conflict with each other, since I've included the necessary changes to DQL Autocomplete that handle those changes within that feature branch. |
Signed-off-by: Paul Sebastian <[email protected]>
Signed-off-by: Paul Sebastian <[email protected]>
The cypress failures seem to be coming and going for different tests with each trivial, unrelated commit. I'm not sure what I should be doing here other than retry until all the stars align. |
body: JSON.stringify({ query: currentValue, field: fieldName, boolFilter: [] }), | ||
}); | ||
}; | ||
|
||
const findValueSuggestions = async ( | ||
index: IndexPattern, |
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 this the indexpattern service or just object. because actually can just take the query object which now has the dataset object which has all the information you will need.
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 is the actual object, I believe its set by the query_editor.tsx function which comes from the dataset object.
0c2888a
Update to DQL Autocomplete: #7391 Every file removal under `grammar/.antlr` can be ignored. - [x] use official value suggestion methods instead of direct api call - [x] removed language specified configuration - [ ] ~~added memoization for value suggestion to reduce number of calls made~~ - [x] removed core start from query suggestion function - [x] added tests for value suggestion - [x] added more test coverage for other general cases - [ ] ~~[[RFC] Monaco Code Editor provider registration #7594](#7594) made changes based on this RFC~~ - [x] remove grammar/.antlr auto generated files - [x] updated types in code completion and related files - [x] fixed many group value suggestion bugs and edge cases with more robust parser visitor - [x] fix group value NOT suggestion bugs - [ ] ~~added basic keyword syntax highlighting~~ --------- Signed-off-by: Paul Sebastian <[email protected]> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com> (cherry picked from commit 741c0d6) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Update to DQL Autocomplete: #7391 Every file removal under `grammar/.antlr` can be ignored. - [x] use official value suggestion methods instead of direct api call - [x] removed language specified configuration - [ ] ~~added memoization for value suggestion to reduce number of calls made~~ - [x] removed core start from query suggestion function - [x] added tests for value suggestion - [x] added more test coverage for other general cases - [ ] ~~[[RFC] Monaco Code Editor provider registration #7594](#7594) made changes based on this RFC~~ - [x] remove grammar/.antlr auto generated files - [x] updated types in code completion and related files - [x] fixed many group value suggestion bugs and edge cases with more robust parser visitor - [x] fix group value NOT suggestion bugs - [ ] ~~added basic keyword syntax highlighting~~ --------- (cherry picked from commit 741c0d6) Signed-off-by: Paul Sebastian <[email protected]> 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: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>
Description
Update to DQL Autocomplete: #7391
Every file removal under
grammar/.antlr
can be ignored.added memoization for value suggestion to reduce number of calls made[RFC] Monaco Code Editor provider registration #7594 made changes based on this RFCadded basic keyword syntax highlightingIssues Resolved
Screenshot
Testing the changes
Changelog
Check List
yarn test:jest
yarn test:jest_integration