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 Trie iter_prefixes_with_ids method to return (prefix, id) pairs #83

Merged
merged 5 commits into from
Mar 25, 2023

Conversation

dfuhry
Copy link
Contributor

@dfuhry dfuhry commented Mar 25, 2023

I would like to get both prefixes and ids (from common_prefix_search) in a single pass through the trie data structure.

Trie has iteritems() method to return (prefix, id) pairs from _trie.predictive_search(). However, there is no corresponding method to return (prefix, id) pairs from _trie.common_prefix_search().

This pull request adds a Trie iter_prefixes_with_ids() method which does that.

@BoboTiG
Copy link
Contributor

BoboTiG commented Mar 25, 2023

Thanks @dfuhry!

Could you rebase on master, and add at least 2 tests (one when the key is present, and one when it's not preset)?

@BoboTiG
Copy link
Contributor

BoboTiG commented Mar 25, 2023

Oh, I just tilted. You need to run ./update_cpp.sh. And commit the changes.

@dfuhry
Copy link
Contributor Author

dfuhry commented Mar 25, 2023

Hi @BoboTiG , thank you for the help. I have added a new test_iter_prefixes_with_keys() method in tests/test_trie.py which tests the cases you requested.

I've also tried to rebase, which I guess I've done right as my branch doesn't show being behind on commits any more.

To rebuild the cpp scripts I had to change the command in update_cpp.sh from "cython" to "cython3" locally. Not sure if that creates any problems. If not, not sure if you want me to add that change to this branch.

@BoboTiG
Copy link
Contributor

BoboTiG commented Mar 25, 2023

Can you rebuild using Cython 0.29.32?

@BoboTiG
Copy link
Contributor

BoboTiG commented Mar 25, 2023

And no need to push the change from update_cpp.sh :)

@BoboTiG
Copy link
Contributor

BoboTiG commented Mar 25, 2023

It seems good now 💪🏻
As soon as the CI is green I'll merge 👍🏻

@BoboTiG BoboTiG merged commit 34413c9 into pytries:master Mar 25, 2023
@BoboTiG
Copy link
Contributor

BoboTiG commented Mar 25, 2023

Thanks a lot for your patience @dfuhry :)

@dfuhry
Copy link
Contributor Author

dfuhry commented Mar 25, 2023

Thank you!

@BoboTiG
Copy link
Contributor

BoboTiG commented Mar 25, 2023

I'll try to cut a new release ASAP.

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