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

Refactor populate_footprints and get_part_details #511

Merged
merged 8 commits into from
Aug 5, 2024
Merged

Refactor populate_footprints and get_part_details #511

merged 8 commits into from
Aug 5, 2024

Conversation

Bouni
Copy link
Owner

@Bouni Bouni commented Aug 2, 2024

PR #503 made the plugin way faster but introduced a problem so that no more part details are returned.

The main issue is (as far as may SQLite knowledge goes) that the named parameter was used in a wrong way.

The for loop gernerates a query like this for example, which does not yield any results:

'''SELECT "LCSC Part", "Stock", "Library Type" FROM parts WHERE parts MATCH "LCSC Part:C2286"'''

Fixing that we end up with:

'''SELECT "LCSC Part", "Stock", "Library Type" FROM parts WHERE parts MATCH C2286'''

But that return every LCSC number that includes the searched number.

So limit the reults to 1 like this:

'''SELECT "LCSC Part", "Stock", "Library Type" FROM parts WHERE parts MATCH C2286 LIMIT 1'''

Gives us just the first match, which should give us the search LCSC number only.

I'm not 100% sure about the LIMIT 1 but it seems to work.

@chmorgan Do you know FTS5 good enough that you can verify if theres another way than double quotes do not yield a exact match? (double quotes do not work)

@Bouni Bouni changed the title Use named parameter the right way, limit results to 1 Use named parameter the right way, filter results Aug 2, 2024
@Bouni
Copy link
Owner Author

Bouni commented Aug 2, 2024

I decided to remove the LIMIT 1 in favour of a list comprehension that filters for the exact LCSC number

@Bouni Bouni requested a review from chmorgan August 2, 2024 06:22
@Bouni
Copy link
Owner Author

Bouni commented Aug 2, 2024

I did a few tests, LIMIT 1 is about 10x faster than using the list comprehension.
It works every time for me.
I guess thats because the exact match is always the first match. Maybe because the parts are ordered by lcsc number in the db!?

@Bouni Bouni linked an issue Aug 2, 2024 that may be closed by this pull request
@gonzalop
Copy link
Contributor

gonzalop commented Aug 2, 2024

Not sure if this is relevant, but the MATCH query wasn't working for me and I fixed it by doing this in #505 -> db2aef5

@chmorgan
Copy link
Collaborator

chmorgan commented Aug 4, 2024

@Bouni agree with removing the limit one and checking the list because I don't see any reason the search is guaranteed to return the exact match first.

I wrote a bunch of fts5 tests here that compare the queries etc but haven't cleaned them up for upstream use, and they are in rust.

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.

Looks "ok" to me although I'm not an fts5 expert. If it works in your testing then its approved by me.

@Bouni
Copy link
Owner Author

Bouni commented Aug 5, 2024

@gonzalop You're right, that was the root problem of PR #503

But that way the method still found hundrets if not thousand results, so no exact match.
I did a few tests and using the column in the search does not result in faster results (0.04755043983459473s vs. 0.04729413986206055s), so I went for just using a named parameter instead.
I also removed the LIMIT 1 because @chmorgan is right, there's no guarantee that the first result is always the first match.
Instead a added the list comprehension back in, which woks fine for me and the speed is "good enough".

But then I realized that we have to search the results again in populate_footprints. So I decided to refactor this whole thing a bit and search for one numebr at a time and put it into a dict which should speed up the lookup even more.

Thanks everybody for helping with all this 👍🏽

@Bouni Bouni requested a review from chmorgan August 5, 2024 05:43
@Bouni Bouni changed the title Use named parameter the right way, filter results Refactor get_part_details Aug 5, 2024
@Bouni Bouni changed the title Refactor get_part_details Refactor populate_footprints and get_part_details Aug 5, 2024
@Bouni Bouni removed the request for review from chmorgan August 5, 2024 11:59
@Bouni Bouni merged commit ed495c0 into main Aug 5, 2024
2 checks passed
@Bouni Bouni deleted the fix-510 branch September 30, 2024 08:24
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.

Stock and Type are no longer updated
3 participants