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

make get_part_details 2500x faster #503

Merged
merged 2 commits into from
Jul 23, 2024
Merged

make get_part_details 2500x faster #503

merged 2 commits into from
Jul 23, 2024

Conversation

Oscilllator
Copy link
Contributor

Before this function took about 5 seconds for my modest 51 line BOM. Now it takes 0.05s 😎

@gyohng
Copy link
Contributor

gyohng commented Jul 20, 2024

Maybe it's best to add a column filter and do some validation so that fuzzy matches don't end up in the output set. I haven't tested your patch. However, I recall trying something similar until I exhausted all ways of checking it with FTS5 and was unable to make it perform fast. Does the plugin window pop up quickly with the MATCH logic?

Disclaimer: I have no merge rights — just another contributor waiting for his PRs to get some love. My personal opinion is that using FTS5 across the entire database might have been a hasty choice - we do get marginally more convenient part searches now, but at the cost of the database being seriously bloated.

cmorganBE

This comment was marked as duplicate.

Copy link
Collaborator

@chmorgan chmorgan left a comment

Choose a reason for hiding this comment

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

Wrong account was why I couldn’t approve. Changes look good. Hard to tell if match syntax is picking up additional entries but otherwise looks clean.

@gyohng
Copy link
Contributor

gyohng commented Jul 20, 2024

@chmorgan It'll return items if something that looks like an LCSC part number appears in any column of the database. There's no column filtering, and there's no post-validation of the returned result set.

@chmorgan
Copy link
Collaborator

@gyohng my comments on approving were along the same lines.

@Oscilllator are there any tests in the plugin yet? It would be helpful to adjust the match so it’s only matching on the appropriate column(s) to avoid mistaken results like @gyohng mentioned.

@chmorgan
Copy link
Collaborator

Maybe it's best to add a column filter and do some validation so that fuzzy matches don't end up in the output set. I haven't tested your patch. However, I recall trying something similar until I exhausted all ways of checking it with FTS5 and was unable to make it perform fast. Does the plugin window pop up quickly with the MATCH logic?

Disclaimer: I have no merge rights — just another contributor waiting for his PRs to get some love. My personal opinion is that using FTS5 across the entire database might have been a hasty choice - we do get marginally more convenient part searches now, but at the cost of the database being seriously bloated.

I had tested here for some time before proposing the fts5 changes. If there is a better approach with fuzzy matching I’d be interested I hearing about it, imo it’s a trade off.

There was talk of a hosted db but not sure if anyone is still working on that.

@gyohng
Copy link
Contributor

gyohng commented Jul 20, 2024

I think the best approach would be to add both of these two things:

  • FTS5 column filtering for the MATCH clause (and maybe ^ to match the first token)
  • a just-in-case test after the query so that only the requested results are returned.

@chmorgan my main concern was that FTS5 made the plugin much slower at every corner, but with the performance updates, I think it's a solution that works. I'm not sure about this specific PR though - I haven't tested it, but if it works visibly as fast as my indexed updates for, let's say, a BOM with 300 parts, then it's a smaller and less intrusive change indeed. Subject to testing.

Storage is not terribly expensive these days anyway, so I have no real objections to FTS5 once the performance is addressed. There are a few alternative solutions, but they are not without their downsides too, and FTS5 works fine.

@Oscilllator
Copy link
Contributor Author

There are no tests in the plugin yet. This is my first time modifying a kicad plugin, I wouldn't know how to set up the infrastructure. I will poke around a little bit today and see if I can at least add an assert that the function worked correctly and hasn't returned results that had a lcsc part number in a different column.

…olumn, incidentally making the query _another_ 25x faster
@Oscilllator
Copy link
Contributor Author

OK I am not an sql expert or anything but this looks like it will ensure that the LCSC part number search now only matches against the values in the LCSC part number column. Please take a look and lmk if you agree. Also the query now takes 2ms, so the total speedup is 2500x faster.

@Oscilllator Oscilllator changed the title make get_part_details 100x faster make get_part_details 2500x faster Jul 21, 2024
@Bouni
Copy link
Owner

Bouni commented Jul 23, 2024

Nice, I've just tested this PR and it lets the UI open in an instant, not in like 5 - 10 seconds as before 🤩

@Bouni Bouni merged commit a997824 into Bouni:main Jul 23, 2024
2 checks passed
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.

5 participants