port making autocomplete for large schemas faster #2746
Conversation
pull upstream to alison's fork july 4
pull upstream to fork 7.22.18
pull from upstream 7.29.18
pull upstream to my fork
|
This pull request is automatically deployed with Now. To access deployments, click Details below or on the icon next to each push. |
| getCompletions(state, session, pos, prefix, callback) { | ||
| if (prefix.length === 0 || !$scope.schema) { | ||
| // make a variable for the auto completion in the query editor | ||
| $scope.autoCompleteSchema = $scope.schema; // removeExtraSchemaInfo( |
There was a problem hiding this comment.
Could you elaborate what this does?
There was a problem hiding this comment.
It essentially separates the interface schema (schema) and the auto complete schema (autoCompleteSchema) to handle UI differences between them.
There was a problem hiding this comment.
To be able to modify the colums further down below?
If we remove the [P] code since #2764 isn't going to merged adding it, then what leaves this PR us with?
There was a problem hiding this comment.
Given all of this, I'd be okay with closing this PR and closing the issue to port this change upstream. What do you think @jezdez?
There was a problem hiding this comment.
Yeah, let's revisit this if the follow-up for #2764 is merged.
| parensStartAt = 1; // linter complains without this line | ||
| } | ||
| // remove '[P] ' for partition keys | ||
| if (c.charAt(0) === '[') { |
There was a problem hiding this comment.
I don't think we have [P] in this code base yet.
There was a problem hiding this comment.
Correct, but that is a pending PR and because it's an if statement it doesn't hurt anything prior to that PR being merged.
There was a problem hiding this comment.
Hm, I see, I'm afraid this will need to be removed for now since I got feedback from Arik about #2764 being problematic in terms of UI design. I will follow-up on this in the PR there and ask the Redash UX people to weigh in on a way forward. In the meantime we shouldn't hold back this PR here.
|
|
||
| table.columns.forEach((c) => { | ||
| keywords[c] = 'Column'; | ||
| table.columns.forEach((c) => { // autoCompleteColumns |
There was a problem hiding this comment.
Could you leave a code comment what this does?
| if (c.charAt(0) === '[') { | ||
| c = c.substring(4, c.length); | ||
| } | ||
| // keywords[c] = 'Column'; // dups columns |
There was a problem hiding this comment.
Why the commented out line here?
There was a problem hiding this comment.
This is essentially my note to self that keywords[c] will duplicate columns in the list, that's why I had to make it keywords[${table.name}.${c}]. This will prevent me from trying to change it back later to reduce complexity and create a bug.
| editor.setOption('enableBasicAutocompletion', false); | ||
| } else { | ||
| editor.setOption('enableLiveAutocompletion', true); | ||
| editor.setOption('enableBasicAutocompletion', true); |
There was a problem hiding this comment.
@jezdez to answer your question about how this makes the performance faster, the key to the performance improvement is in these 2 lines about enableBasicAutocompletion. The text editor redash uses is Ace Editor and without these lines there were autocompletion calls happening via the default configuration through Ace Editor that were causing a performance issue.
Yes, a lot of other things seem to have ended up in the commit that this PR is porting upstream.
There was a problem hiding this comment.
Looking further into this, this seems to actually just add an additional command to the editor (https://github.com/ajaxorg/ace/blob/51a9b7c74671b86dceab01296a7d09e877853484/lib/ace/ext/language_tools.js#L175-L203) that triggers the auto-complete menu when typing ctrl+space (et al.). Since this is in addition to the live auto-completion (where "live" means that is happens on any input change of the editor), I don't see how this improves performance per-se. Instead this seems like a drive-by change that was shipped in the original code change in the Mozilla fork. Not a big deal, but just want to state that this is a thing.
| let parensStartAt = c.indexOf('(') - 1; | ||
| c = c.substring(0, parensStartAt); | ||
| parensStartAt = 1; // linter complains without this line | ||
| } |
There was a problem hiding this comment.
So this removes columns that end with a closing parenthesis at the point where the opening parenthesis is? Can you elaborate which data sources have those kind of colums?
There was a problem hiding this comment.
@jezdez It's not a data source thing, it's a Mozilla fork thing. In the Mozilla fork the field type is appended to the field name. So a field named id becomes id (integer) in the schema browser.
There was a problem hiding this comment.
I see, let's remove this then.
|
@alison985 Before we merge this I think we need to further review what the changes done after mozilla#232 mean for this repo that doesn't have all the changes made in the fork. |
|
See comment #2746 (comment) Jannis and I agreed to close this one. Some of the changes may be relevant in the future, but not until the related features are upstream. |
Addresses mozilla#454 which fixes mozilla#232