[Selection syntax] Generalized autocomplete functionality #26273

salazarm · 2024-12-04T16:20:12Z

Summary & Motivation

This PR introduces generalized autocomplete functionality that can be re-used across grammars that follow the base structure of our asset selection syntax grammar.

As an input the autocomplete factory takes the names of attributes, their values, and the list of functions available in the grammar.

To implement auto-complete I opted to create a new grammar so that I can allow certain things to still be valid inputs.
Benefits of separate grammar:

IncompleteOrExpression, IncompleteAndExpression, IncompleteFunctionCall, etc. allow our syntax highlighting and autocomplete visitors to visit the full expression even if parts of it are malformed.
DownTraversal and UpTraversal are distinguished simplifying the visitor logic
Whitespace is differentiated between afterExpressionWhitespace and afterLogicalOperatorWhitespace, etc. greatly simplifying the visitors that provide suggestions in whitespace. (Instead of needing a visit function for every possible expression we instead have the whitespace tokens handle creating suggestions).
orToken, andToken, notToken simplifies visitor logic, rather having the various expressions that contain these tokens handle what to do when the cursor is over them, we can instead have the token itself handle what happens and reduce the amount of visitor logic.
Separate tokens for FunctionName AttributeName AttributeValue - also reduces visitor logic

Generally speaking, my strategy for the grammar was to make leaf nodes that are shared among the various expression in order to allow those leaf nodes to make suggestions and reduce the number of visit functions required in the visitor (avoid combinatorial explosion of cases).

The reason for doing this.cursorIndex - 1 in the nodeIncludesCursor function is that I want the suggestions to be determined by the token PRECEEDING the cursor, so I want the token immediately before the cursor to return "true" for nodeIncludesCursor.

I can probably clean up the grammar a bit, but quite fatigued for this PR. In a future PR I'll look to simplify the grammar even more but what I have now passes all the tests I've written.

How I Tested These Changes

Extensive jest testing and real testing.

github-actions · 2024-12-04T16:23:57Z

Deploy preview for dagit-core-storybook ready!

✅ Preview
https://dagit-core-storybook-b2aoranul-elementl.vercel.app
https://salazarm-autocomplete.core-storybook.dagster-docs.io

Built with commit 224ed83.
This pull request is being automatically deployed with vercel-action

python_modules/dagster-webserver/dagster_webserver/graphiql/graphiql.min.js

js_modules/dagster-ui/packages/ui-core/src/asset-selection/input/AssetSelectionLinter.ts

salazarm · 2024-12-05T19:00:39Z

js_modules/dagster-ui/packages/ui-core/src/selection/SelectionAutoComplete.ts

+  return function (cm: CodeMirror.Editor, _options: CodeMirror.ShowHintOptions): any {
+    const line = cm.getLine(0);
+    const {parseTrees} = parseInput(line);
+
+    let start = 0;
+    const actualCursorIndex = cm.getCursor().ch;
+
+    let visitorWithAutoComplete;
+    if (!parseTrees.length) {
+      // Special case empty string to add unmatched value results
+      visitorWithAutoComplete = new SelectionAutoCompleteVisitor({
+        attributesMap,
+        functions,
+        nameBase,
+        line,
+        cursorIndex: actualCursorIndex,
+      });
+      start = actualCursorIndex;
+      visitorWithAutoComplete.addUnmatchedValueResults('');
+    } else {
+      for (const {tree, line} of parseTrees) {
+        const cursorIndex = actualCursorIndex - start;
+        const visitor = new SelectionAutoCompleteVisitor({
+          attributesMap,
+          functions,
+          nameBase,
+          line,
+          cursorIndex,
+        });
+        visitor.visit(tree);
+        const length = line.length;
+        if (cursorIndex <= length) {
+          visitorWithAutoComplete = visitor;
+          break;
+        }
+        start += length;
+      }
+    }
+    if (visitorWithAutoComplete) {
+      return {
+        list: visitorWithAutoComplete.list,
+        from: cm.posFromIndex(start + visitorWithAutoComplete.startReplacementIndex),
+        to: cm.posFromIndex(start + visitorWithAutoComplete.stopReplacementIndex),
+      };
+    }
+  };


Add tests for this error scenario

salazarm · 2024-12-05T19:01:51Z

js_modules/dagster-ui/packages/ui-core/src/selection/SelectionAutoCompleteInputParser.ts

+    // Initialize ANTLR input stream, lexer, and parser
+    const inputStream = CharStreams.fromString(substring);
+    const lexer = new SelectionAutoCompleteLexer(inputStream);
+    const tokenStream = new CommonTokenStream(lexer);
+    const parser = new SelectionAutoCompleteParser(tokenStream);
+
+    // Attach custom error listener
+    const errorListener = new CustomErrorListener();
+    parser.removeErrorListeners();
+    parser.addErrorListener(errorListener);
+
+    // Set the error handler to bail on error to prevent infinite loops
+    parser.errorHandler = new BailErrorStrategy();
+
+    let tree: ParseTree | null = null;
+    try {
+      // Parse using the 'expr' rule instead of 'start' to allow partial parsing
+      tree = parser.expr();
+      parseTrees.push({tree, line: substring});
+
+      // Advance currentPosition to the end of the parsed input
+      const lastToken = tokenStream.get(tokenStream.index - 1);
+      currentPosition += lastToken.stopIndex + 1;
+    } catch (e) {
+      // Parsing error occurred
+      const currentErrors = errorListener.getErrors();
+
+      if (currentErrors.length > 0) {
+        const error = currentErrors[0]!;
+        const errorCharPos = error.column;
+        const errorIndex = currentPosition + errorCharPos;
+
+        // Parse up to the error
+        const validInput = input.substring(currentPosition, errorIndex);


make sure this is covered by tests

hellendag · 2024-12-05T16:49:49Z

js_modules/dagster-ui/packages/ui-core/src/selection/SelectionAutoCompleteUtil.ts

+
+export const removeQuotesFromString = (value: string) => {
+  if (value.length > 1 && value[0] === '"' && value[value.length - 1] === '"') {
+    return value.slice(1, value.length - 1);


value.slice(1, -1)?

bengotow

This looks very cool 🙌 Nicely done! Cool that this can be attached to a CodeMirror parse tree visitor, I think I'd missed the implementation of that with the old AssetSelection one.

…autocomplete

…#26273) ## Summary & Motivation This PR introduces generalized autocomplete functionality that can be re-used across grammars that follow the base structure of our asset selection syntax grammar. As an input the autocomplete factory takes the names of attributes, their values, and the list of functions available in the grammar. To implement auto-complete I opted to create a new grammar so that I can allow certain things to still be valid inputs. Benefits of separate grammar: - `IncompleteOrExpression`, `IncompleteAndExpression`, `IncompleteFunctionCall`, etc. allow our syntax highlighting and autocomplete visitors to visit the full expression even if parts of it are malformed. - DownTraversal and UpTraversal are distinguished simplifying the visitor logic - Whitespace is differentiated between `afterExpressionWhitespace` and `afterLogicalOperatorWhitespace`, etc. greatly simplifying the visitors that provide suggestions in whitespace. (Instead of needing a visit function for every possible expression we instead have the whitespace tokens handle creating suggestions). - `orToken`, `andToken`, `notToken` simplifies visitor logic, rather having the various expressions that contain these tokens handle what to do when the cursor is over them, we can instead have the token itself handle what happens and reduce the amount of visitor logic. - Separate tokens for `FunctionName` `AttributeName` `AttributeValue` - also reduces visitor logic Generally speaking, my strategy for the grammar was to make leaf nodes that are shared among the various expression in order to allow those leaf nodes to make suggestions and reduce the number of visit functions required in the visitor (avoid combinatorial explosion of cases). The reason for doing `this.cursorIndex - 1` in the `nodeIncludesCursor` function is that I want the suggestions to be determined by the token PRECEEDING the cursor, so I want the token immediately before the cursor to return "true" for `nodeIncludesCursor`. I can probably clean up the grammar a bit, but quite fatigued for this PR. In a future PR I'll look to simplify the grammar even more but what I have now passes all the tests I've written. ## How I Tested These Changes Extensive jest testing and real testing. <img width="937" alt="Screenshot 2024-12-04 at 11 20 58 AM" src="https://github.com/user-attachments/assets/c29e702e-6048-40cb-ada6-04bb1a1a2a66">

rewrite auto-complete

323a599

salazarm requested review from alangenfeld, bengotow, hellendag and briantu December 4, 2024 16:20

salazarm added 6 commits December 5, 2024 10:31

mo

fb760ac

mo

431614a

all tests fixed

c25f30e

.

0fd865c

.

126c4d7

.

0ed15f1

salazarm force-pushed the salazarm/autocomplete branch from c188430 to 0ed15f1 Compare December 5, 2024 16:49

salazarm added 4 commits December 5, 2024 11:59

revert random changes

5de3aee

revert random changes

9c0ba4d

revert random changes

88bb1ce

revert random changes

5c78ba4

salazarm commented Dec 5, 2024

View reviewed changes

python_modules/dagster-webserver/dagster_webserver/graphiql/graphiql.min.js Outdated Show resolved Hide resolved

salazarm commented Dec 5, 2024

View reviewed changes

js_modules/dagster-ui/packages/ui-core/src/asset-selection/input/AssetSelectionLinter.ts Outdated Show resolved Hide resolved

salazarm commented Dec 5, 2024

View reviewed changes

js_modules/dagster-ui/packages/ui-core/src/asset-selection/input/AssetSelectionLinter.ts Show resolved Hide resolved

salazarm commented Dec 5, 2024

View reviewed changes

hellendag approved these changes Dec 5, 2024

View reviewed changes

salazarm added 2 commits December 5, 2024 14:11

fix

b893403

mo

de3d1df

bengotow approved these changes Dec 9, 2024

View reviewed changes

salazarm added 3 commits December 9, 2024 18:51

Merge branch 'master' of github.com:dagster-io/dagster into salazarm/…

50efe3c

…autocomplete

Merge branch 'master' of github.com:dagster-io/dagster into salazarm/…

dbfc0bf

…autocomplete

more tests

224ed83

salazarm merged commit c6928b7 into master Dec 10, 2024
1 of 2 checks passed

salazarm deleted the salazarm/autocomplete branch December 10, 2024 20:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Selection syntax] Generalized autocomplete functionality #26273

[Selection syntax] Generalized autocomplete functionality #26273

salazarm commented Dec 4, 2024 •

edited

Loading

github-actions bot commented Dec 4, 2024 •

edited

Loading

salazarm Dec 5, 2024

salazarm Dec 5, 2024

hellendag Dec 5, 2024

bengotow left a comment

[Selection syntax] Generalized autocomplete functionality #26273

[Selection syntax] Generalized autocomplete functionality #26273

Conversation

salazarm commented Dec 4, 2024 • edited Loading

Summary & Motivation

How I Tested These Changes

github-actions bot commented Dec 4, 2024 • edited Loading

salazarm Dec 5, 2024

Choose a reason for hiding this comment

salazarm Dec 5, 2024

Choose a reason for hiding this comment

hellendag Dec 5, 2024

Choose a reason for hiding this comment

bengotow left a comment

Choose a reason for hiding this comment

salazarm commented Dec 4, 2024 •

edited

Loading

github-actions bot commented Dec 4, 2024 •

edited

Loading