-
Notifications
You must be signed in to change notification settings - Fork 14.7k
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
[SQL Lab] Improve autocomplete #8305
[SQL Lab] Improve autocomplete #8305
Conversation
Codecov Report
@@ Coverage Diff @@
## master #8305 +/- ##
==========================================
- Coverage 73.53% 67.75% -5.79%
==========================================
Files 115 451 +336
Lines 12396 22702 +10306
Branches 0 2370 +2370
==========================================
+ Hits 9116 15381 +6265
- Misses 3280 7184 +3904
- Partials 0 137 +137
Continue to review full report at Codecov.
|
@@ -27,6 +27,11 @@ import { areArraysShallowEqual } from '../../reduxUtils'; | |||
|
|||
const langTools = ace.acequire('ace/ext/language_tools'); | |||
|
|||
const SQL_KEYWORD_AUTOCOMPLETE_SCORE = 100; |
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.
where are those score: XXXXX_ SCORE
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.
They're used by ace editor's autocomplete logic to determine the ordering of autocomplete results
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.
LGTM
const SQL_KEYWORD_AUTOCOMPLETE_SCORE = 100; | ||
const SCHEMA_AUTOCOMPLETE_SCORE = 60; | ||
const TABLE_AUTOCOMPLETE_SCORE = 55; | ||
const COLUMN_AUTOCOMPLETE_SCORE = 50; |
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.
When writing a query, one is probably spending more time autocompleting column names than table/schema names. Wondering if having a higher score for column names wrt schema/table names could result in a better user experience, too?
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.
Hmm, I'm not sure about this. I'm going to merge this refactor in, reach out to some sql lab powerusers, and come back with the results. I anticipate making some more changes here as we have other feedback about autocomplete to fix.
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.
Absolutely; this was not meant as a blocking comment. Btw, I briefly checked DataGrip, and it seems it gives a higher score/priority for columns than tables, so I might be on to something here after all 😄
CATEGORY
Choose one
SUMMARY
when a user would type
sel
into the autocomplete, we'd prioritize schemas containingsel
as opposed to the SQL keywordSELECT
. This fixes that bug, and also refactors some of the code to prevent a similar issue and be clearer (read more immutable).BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TEST PLAN
Type
sel
into the editor, and seeSELECT
as the autocomplete optionADDITIONAL INFORMATION
REVIEWERS
@graceguo-supercat @michellethomas @betodealmeida