Skip to content
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

Add dot behavior to autocomplete #3092

Merged
merged 9 commits into from
Dec 26, 2018
Merged

Conversation

gabrieldutra
Copy link
Member

@gabrieldutra gabrieldutra commented Nov 18, 2018

Description

A start on #3067 - Issues 1 and 2.
This changes the dot behavior when writing a query and start separating keyword types as Table Keyword, Column Keyword or Table-column Keyword

With that we separate 2 situations of autocomplete, instead of rendering the entire schema:

  1. Default - Show table and distinct column names;
  2. tableName. - Show specific Table columns.

This will probably improve the performance and be useful when we introduce aliases or query context.

Notes

Autocomplete not aware of the context (#3067 and #2329)

The dot by default breaks autocomplete words, so I changed its Regexp to include it.

Repeated suggestions when navigating through query pages

repeated-suggestions
This seems to be something that could be affecting the performance.
Whenever would one navigate to a new query page, another completer would be added to langTools. I've changed addCompleter to setCompleters to fix this.

Preview

Before

autocomplete-dot-before

After

autocomplete-dot

To do

  • Start it
  • Test changes with a large schema and validate the performance
  • Refactor QueryEditor constructor
  • Refactor other QueryEditor methods? 🤔

We'll probably have some conflicts with #3091, but nothing huge.

Let me know your thoughts 😁

@gabrieldutra gabrieldutra self-assigned this Nov 18, 2018

this.onLoad = (editor) => {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was there a reason for those methods to be inside QueryEditor's constructor?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤷‍♂️

@washort ?

@gabrieldutra
Copy link
Member Author

Well, I tested with a db schema with 10k tokensCount (twice the current limit of 5k) and I actually didn't have any performance issues neither before nor after these PR changes. I did notice a good change on the suggestions, but it's probably better to wait for other improvements before disabling the tokensCount rule.

LMK what you think 😁

@gabrieldutra gabrieldutra changed the title WIP: Add dot behavior to autocomplete Add dot behavior to autocomplete Nov 25, 2018
@arikfr arikfr added the review label Dec 17, 2018
@ghost ghost assigned arikfr Dec 26, 2018
@arikfr arikfr merged commit 26965b4 into getredash:master Dec 26, 2018
@ghost ghost removed the review label Dec 26, 2018
@arikfr
Copy link
Member

arikfr commented Dec 26, 2018

👍

@gabrieldutra gabrieldutra mentioned this pull request Jul 9, 2020
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants