-
Notifications
You must be signed in to change notification settings - Fork 165
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
Match on module aliases for auto import suggestions #730
Changes from 4 commits
5b914ef
4702c3a
dcd12ba
1665040
12c4bbe
fdfbeca
eda133a
cf83daf
d2639bc
1667b67
253fdee
9e494c3
9134d03
b266add
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -85,6 +85,27 @@ class Metadata(Model): | |
objects = Query(table_name, columns) | ||
|
||
|
||
class Alias(Model): | ||
table_name = "aliases" | ||
schema = { | ||
"alias": "TEXT", | ||
"module": "TEXT", | ||
} | ||
columns = list(schema.keys()) | ||
objects = Query(table_name, columns) | ||
|
||
@classmethod | ||
def create_table(cls, connection): | ||
super().create_table(connection) | ||
connection.execute("CREATE INDEX IF NOT EXISTS alias ON aliases(alias)") | ||
|
||
modules = Query( | ||
"(SELECT DISTINCT aliases.*, package, source, type FROM aliases INNER JOIN names on aliases.module = names.module)", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not a DB expert, but the names table can comprise 10,000 - 100,000 rows, so I am wondering if we should run this inner join on every autoimport request (which can happen with every keystroke when rope is run inside of a language server). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The joins are pretty fast, I made a notebook to test it out. The join time should be dominated by the Alias table, not the Names table, because the Names table has an index on the module column. Also, here we're including a where clause which makes the left side of the join even smaller. Most DB engines are pretty good about pushing down the filter past the join and sqlite3 seems to handle it well. I thought about this a little bit before testing out this implementations I see 3 main paths forward:
@tkrabel what do you think? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The current approach has the benefit that we never have to do any updates on the aliases tables. The names table is the source of truth of that is available to the user. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @MrBago thanks for doing testing the performance notebook, the notebook brings up something that is interesting/surprising to me, in that the module search_by_name_like query is much slower than what I was expecting. A prefix search using an index should not have been that slow. That is an unrelated issue from this PR though, so I've created another ticket for that #736, but with the fixed index the Alias query should hopefully become faster as well. 883ms for an inner join between a large table and a very small table doesn't smell right to me that seems to indicate a full table scan as well. I'll see if I can fix this tomorrow, but in the meantime, apologies but I'll be holding off on merging this PR yet until that is fixed and then we can see the new performance impact. |
||
columns + ["package", "source", "type"], | ||
) | ||
search_modules_with_alias = modules.where("alias LIKE (?)") | ||
|
||
|
||
class Name(Model): | ||
table_name = "names" | ||
schema = { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,6 +19,7 @@ | |
from rope.contrib.autoimport import models | ||
from rope.contrib.autoimport.defs import ( | ||
ModuleFile, | ||
Alias, | ||
Name, | ||
NameType, | ||
Package, | ||
|
@@ -293,6 +294,12 @@ def _search_module( | |
yield SearchResult( | ||
f"import {module}", module, source, NameType.Module.value | ||
) | ||
for alias, module, source in self._execute( | ||
models.Alias.search_modules_with_alias.select("alias", "module", "source"), (name,) | ||
): | ||
yield SearchResult( | ||
f"import {module} as {alias}", alias, source, NameType.Module.value | ||
) | ||
|
||
def get_modules(self, name) -> List[str]: | ||
"""Get the list of modules that have global `name`.""" | ||
|
@@ -434,11 +441,14 @@ def clear_cache(self): | |
""" | ||
with self.connection: | ||
self._execute(models.Name.objects.drop_table()) | ||
self._execute(models.Alias.objects.drop_table()) | ||
self._execute(models.Package.objects.drop_table()) | ||
self._execute(models.Metadata.objects.drop_table()) | ||
models.Name.create_table(self.connection) | ||
models.Alias.create_table(self.connection) | ||
models.Package.create_table(self.connection) | ||
models.Metadata.create_table(self.connection) | ||
self.add_aliases(self.project.prefs.import_aliases) | ||
data = ( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So if I understand this correctly, this will add the aliases into the database only when the database is created. IIUC, this would need to depend on the database being re-created when preference changes. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're right. I didn't look at the different ways that prefs can change, we could add a method to clear the aliases table and reset it and invoke that when the prefs are updated. |
||
versioning.calculate_version_hash(self.project), | ||
json.dumps(versioning.get_version_hash_data(self.project)), | ||
|
@@ -548,6 +558,10 @@ def _convert_name(name: Name) -> tuple: | |
name.source.value, | ||
name.name_type.value, | ||
) | ||
|
||
def add_aliases(self, aliases: Iterable[Alias]): | ||
if aliases: | ||
self._executemany(models.Alias.objects.insert_into(), aliases) | ||
|
||
def _add_names(self, names: Iterable[Name]): | ||
if names is not None: | ||
|
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.
I'm adding an autoimport prefs table in #516 , can we move this there?
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.
@lieryan do you have a preference where the "import_aliases" option goes. I tried moving it, but I was having trouble with the nested Prefs. Specifically I wasn't able to set the prefs for testing here.