[Auto Suggest] Fix Grammar Changes #9120

paulstn · 2024-12-24T19:01:40Z

Description

Redos changes made to the grammar meant for fixing bugs. Fixes those same original bugs through other means.

Also rewrites the 'redo and combine' functionality to be more robust.

The old system

The old system was simply a flag that could be set by a preferred rule to run code completion once with all the rules, and then another time without any rules, to finally combine the results from both.

The new system

The new system requires any preferred rule to submit its name, and then goes through that cumulative list to remove only that rule from the code completion process, combining every possible result it gets. This new system allows for multiple preferred rules to be left out of the code completion process if it happens to block keyword candidates, and also for every other potentially necessary preferred rule to still be in effect while it does so.

Diagram: (change made was for the "Main Parsing Loop")

Removes grammar changes from:
https://github.com/opensearch-project/OpenSearch-Dashboards/pull/8087/files#diff-3dc92487ddb63ebd741b8639f39b9440158a1f31317978cbd84af144daf3adc0

Each grammar change reverted / with its corresponding fix:

search source with logical expressions, reverting required a preferred rule on logical expression to prevent code completion issues and a new redo and combine function
qualified name (including field names) using ident instead of ID and removing the parsing ability of DOTs, fixes parsing DOTs in field names, and issues with field name appearing more than when its needed was mitigated by adding logic to the field name pref. rule to only suggest if the last token isn't ID, backticked ID, or DOT
table function and table source clause allowing for tables to be specified many times in the same query
, handled through a pref. rule but some behavior is left as is.

Screenshot

Testing the changes

Changelog

fix: PPL Grammar Parsing related issues

Check List

All tests pass
- yarn test:jest
- yarn test:jest_integration
New functionality includes testing.
New functionality has been documented.
Update CHANGELOG.md
Commits are signed per the DCO using --signoff

github-actions · 2024-12-24T19:02:13Z

❌ Empty Changelog Section

The 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 · 2024-12-24T19:16:48Z

Codecov Report

Attention: Patch coverage is 79.59184% with 10 lines in your changes missing coverage. Please review.

Project coverage is 61.70%. Comparing base (c91df47) to head (8f35ef7).
Report is 8 commits behind head on main.

Files with missing lines	Patch %	Lines
src/plugins/data/public/antlr/shared/utils.ts	80.00%	1 Missing and 5 partials ⚠️
...ntlr/opensearch_ppl/opensearch_ppl_autocomplete.ts	66.66%	0 Missing and 4 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #9120      +/-   ##
==========================================
+ Coverage   61.68%   61.70%   +0.02%     
==========================================
  Files        3816     3816              
  Lines       91693    91821     +128     
  Branches    14516    14542      +26     
==========================================
+ Hits        56557    56662     +105     
+ Misses      31510    31505       -5     
- Partials     3626     3654      +28

Flag	Coverage Δ
Linux_1	`28.99% <4.08%> (-0.06%)`	⬇️
Linux_2	`56.46% <ø> (ø)`
Linux_3	`39.18% <79.59%> (+0.09%)`	⬆️
Linux_4	`28.91% <4.08%> (-0.07%)`	⬇️
Windows_1	`29.00% <4.08%> (-0.06%)`	⬇️
Windows_2	`56.41% <ø> (ø)`
Windows_3	`39.18% <79.59%> (+0.09%)`	⬆️
Windows_4	`28.91% <4.08%> (-0.07%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

github-actions · 2025-01-08T18:25:56Z

❌ Empty Changelog Section

The 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.

github-actions · 2025-01-08T18:38:19Z

❌ Empty Changelog Section

The 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]>

… work with any lang and any number of rules Signed-off-by: Paul Sebastian <[email protected]>

Signed-off-by: Paul Sebastian <[email protected]>

… a general nonspecific combine function Signed-off-by: Paul Sebastian <[email protected]>

Signed-off-by: Paul Sebastian <[email protected]>

mengweieric · 2025-01-24T21:30:48Z

src/plugins/data/public/antlr/opensearch_sql/opensearch_sql_autocomplete.ts

@@ -223,6 +223,11 @@ export function processVisitedRules(
              '.' + removePotentialBackticks(nextToken?.text ?? '');
            continue;
          }
+          if (validIDToken(token)) {
+            token.text = removePotentialBackticks(token.text ?? '');


will clicking a backtick removed suggestion add backtick back to the query? Also why is it called potential backtick?

The formation of this list sigTokens is only used by this preferred rule on RULE_predicate to interpret the predicate as its formed. So everything interacting with backticks is just removing them so that if we want to use value completion on a field that's already in the query that happens to have backticks, it can match the field name to the index pattern's field.
This is needed because value completion has default behavior of NOT sending an aggregation query if it can't match the field name to the current index's field list.
This change simply allows for values to be suggested if there are backticks in the field within the current predicate.
Regarding your question on if it will readd the backtick, it doesn't affect the field already in the query because this pref. rule doesn't suggest things that extend beyond the current 'word', so it will only replace the current token.
It's called potential backtick because I'm running this method on every token that could be an ID or backticked-ID, so it's just some regex that looks to remove backticks at the front and back, and doesn't require any specific type of input that has backticks for sure.

mengweieric · 2025-01-24T21:54:42Z

src/plugins/data/public/antlr/shared/utils.ts

+      context,
+    });
+
+    // combine result and nextResult


nit. good to have comments to describe the intent and any potential edge cases as I feel that the for and switch statement handles various data types and use cases, but it might not be immediately clear why certain cases (e.g., boolean or undefined) are treated a specific way.

I'll add a big comment outlining the details on combining results

mengweieric · 2025-01-24T21:55:19Z

src/plugins/data/public/antlr/opensearch_ppl/code_completion.ts

@@ -65,18 +65,10 @@ export const getSuggestions = async ({
        ...PPL_AGGREGATE_FUNCTIONS.map((af) => ({
          text: `${af}()`,
          type: monaco.languages.CompletionItemKind.Function,
-          insertText: af + '(${1:expr}) ',
+          insertText: af + ' ',


why remove?

there were asks to remove it, since not many people knew how to use snippets (tab + keep tabbing to get to the next cursor place). this is primarily fueled by the version of monaco we're using, where snippets are a little buggy and also don't have the full functionality that current monaco has.
we should reintroduce them when monaco is updated.

mengweieric · 2025-01-24T21:57:06Z

could you check if failing CIs relevant?

Signed-off-by: Paul Sebastian <[email protected]>

opensearch-trigger-bot · 2025-01-28T23:31:05Z

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/OpenSearch-Dashboards/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/OpenSearch-Dashboards/backport-2.x
# Create a new branch
git switch --create backport/backport-9120-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 d227bc59c56a0ac4e3ffd691b9a0df1635839a84
# Push it to GitHub
git push --set-upstream origin backport/backport-9120-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/OpenSearch-Dashboards/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-9120-to-2.x.

* sql more field name tests Signed-off-by: Paul Sebastian <[email protected]> * undo search command change w/ updated logic, generalize rerun flag to work with any lang and any number of rules Signed-off-by: Paul Sebastian <[email protected]> * beginning of updating combined results properly Signed-off-by: Paul Sebastian <[email protected]> * Changeset file for PR opensearch-project#9120 created/updated * cleaner non-recursive system to go through rerunWithout rule list and a general nonspecific combine function Signed-off-by: Paul Sebastian <[email protected]> * revert qualified name grammar Signed-off-by: Paul Sebastian <[email protected]> * revert from and table source grammars Signed-off-by: Paul Sebastian <[email protected]> * fix ppl field name issue Signed-off-by: Paul Sebastian <[email protected]> * remove ppl agg func snippets and update ppl field name test Signed-off-by: Paul Sebastian <[email protected]> * leave a space for sql agg func Signed-off-by: Paul Sebastian <[email protected]> * ppl autocomplete more pref. rules test Signed-off-by: Paul Sebastian <[email protected]> * remove backticks from ppl field name to match for value completion Signed-off-by: Paul Sebastian <[email protected]> * add comment explaining result combination Signed-off-by: Paul Sebastian <[email protected]> --------- Signed-off-by: Paul Sebastian <[email protected]> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com> Signed-off-by: Ubuntu <[email protected]>

github-actions bot added the seasoned-contributor label Dec 24, 2024

github-actions bot added the failed changeset label Dec 24, 2024

paulstn mentioned this pull request Dec 24, 2024

[Auto Suggest] PPL & SQL Value Suggestion #8275

Merged

7 tasks

paulstn force-pushed the autosuggest-redo-grammar-changes branch from e9fff9a to d91f591 Compare January 8, 2025 00:38

opensearch-changeset-bot bot added a commit to paulstn/OpenSearch-Dashboards that referenced this pull request Jan 8, 2025

Changeset file for PR opensearch-project#9120 created/updated

524212c

github-actions bot removed the failed changeset label Jan 8, 2025

paulstn force-pushed the autosuggest-redo-grammar-changes branch from e2e3843 to 81454db Compare January 9, 2025 20:03

paulstn pushed a commit to paulstn/OpenSearch-Dashboards that referenced this pull request Jan 9, 2025

Changeset file for PR opensearch-project#9120 created/updated

74322e2

paulstn force-pushed the autosuggest-redo-grammar-changes branch from cc91a16 to f310ed4 Compare January 9, 2025 20:12

paulstn pushed a commit to paulstn/OpenSearch-Dashboards that referenced this pull request Jan 9, 2025

Changeset file for PR opensearch-project#9120 created/updated

26c7163

paulstn force-pushed the autosuggest-redo-grammar-changes branch from f310ed4 to b12ce80 Compare January 9, 2025 21:33

paulstn pushed a commit to paulstn/OpenSearch-Dashboards that referenced this pull request Jan 16, 2025

Changeset file for PR opensearch-project#9120 created/updated

96e1710

paulstn force-pushed the autosuggest-redo-grammar-changes branch from b12ce80 to bcc4e5f Compare January 16, 2025 21:36

paulstn marked this pull request as ready for review January 16, 2025 21:39

paulstn requested review from ananzh, kavilla, AMoo-Miki, ashwin-pc, joshuarrrr, abbyhu2000, zengyan-amazon, zhongnansu, manasvinibs, ZilongX, Flyingliuhub and curq as code owners January 16, 2025 21:40

paulstn requested review from LDrago27, virajsanghvi, sejli, joshuali925 and huyaboo as code owners January 16, 2025 21:40

paulstn and others added 9 commits January 17, 2025 23:43

sql more field name tests

e1cb957

Signed-off-by: Paul Sebastian <[email protected]>

undo search command change w/ updated logic, generalize rerun flag to…

b428547

… work with any lang and any number of rules Signed-off-by: Paul Sebastian <[email protected]>

beginning of updating combined results properly

ecb6aee

Signed-off-by: Paul Sebastian <[email protected]>

Changeset file for PR opensearch-project#9120 created/updated

1fa34f7

cleaner non-recursive system to go through rerunWithout rule list and…

af923f2

… a general nonspecific combine function Signed-off-by: Paul Sebastian <[email protected]>

revert qualified name grammar

c4ff5d5

Signed-off-by: Paul Sebastian <[email protected]>

revert from and table source grammars

b789521

Signed-off-by: Paul Sebastian <[email protected]>

fix ppl field name issue

91e3783

Signed-off-by: Paul Sebastian <[email protected]>

remove ppl agg func snippets and update ppl field name test

435e2ea

Signed-off-by: Paul Sebastian <[email protected]>

paulstn force-pushed the autosuggest-redo-grammar-changes branch from 25d0ef5 to 435e2ea Compare January 17, 2025 23:43

paulstn added 3 commits January 17, 2025 23:47

leave a space for sql agg func

6ff856b

Signed-off-by: Paul Sebastian <[email protected]>

ppl autocomplete more pref. rules test

d673ceb

Signed-off-by: Paul Sebastian <[email protected]>

remove backticks from ppl field name to match for value completion

ffea3a3

Signed-off-by: Paul Sebastian <[email protected]>

mengweieric previously approved these changes Jan 24, 2025

View reviewed changes

add comment explaining result combination

dccb88f

Signed-off-by: Paul Sebastian <[email protected]>

paulstn dismissed mengweieric’s stale review via dccb88f January 24, 2025 22:33

Merge branch 'main' into autosuggest-redo-grammar-changes

8f35ef7

joshuali925 approved these changes Jan 24, 2025

View reviewed changes

mengweieric approved these changes Jan 24, 2025

View reviewed changes

ananzh merged commit d227bc5 into opensearch-project:main Jan 28, 2025
69 of 71 checks passed

sejli added the backport 2.x label Jan 28, 2025

opensearch-trigger-bot bot added the failed backport label Jan 28, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Auto Suggest] Fix Grammar Changes #9120

[Auto Suggest] Fix Grammar Changes #9120

paulstn commented Dec 24, 2024 •

edited

Loading

github-actions bot commented Dec 24, 2024

codecov bot commented Dec 24, 2024 •

edited

Loading

github-actions bot commented Jan 8, 2025

github-actions bot commented Jan 8, 2025

mengweieric Jan 24, 2025

paulstn Jan 24, 2025 •

edited

Loading

mengweieric Jan 24, 2025

paulstn Jan 24, 2025

mengweieric Jan 24, 2025

paulstn Jan 24, 2025

mengweieric commented Jan 24, 2025

opensearch-trigger-bot bot commented Jan 28, 2025

[Auto Suggest] Fix Grammar Changes #9120

[Auto Suggest] Fix Grammar Changes #9120

Conversation

paulstn commented Dec 24, 2024 • edited Loading

Description

The old system

The new system

Each grammar change reverted / with its corresponding fix:

Screenshot

Testing the changes

Changelog

Check List

github-actions bot commented Dec 24, 2024

❌ Empty Changelog Section

codecov bot commented Dec 24, 2024 • edited Loading

Codecov Report

github-actions bot commented Jan 8, 2025

❌ Empty Changelog Section

github-actions bot commented Jan 8, 2025

❌ Empty Changelog Section

mengweieric Jan 24, 2025

Choose a reason for hiding this comment

paulstn Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

mengweieric Jan 24, 2025

Choose a reason for hiding this comment

paulstn Jan 24, 2025

Choose a reason for hiding this comment

mengweieric Jan 24, 2025

Choose a reason for hiding this comment

paulstn Jan 24, 2025

Choose a reason for hiding this comment

mengweieric commented Jan 24, 2025

opensearch-trigger-bot bot commented Jan 28, 2025

paulstn commented Dec 24, 2024 •

edited

Loading

codecov bot commented Dec 24, 2024 •

edited

Loading

paulstn Jan 24, 2025 •

edited

Loading