feat: Introduce function-based API for tokenizers - #110
Conversation
philippemnoel
left a comment
There was a problem hiding this comment.
As discussed in Slack, we want to support the edge_ngram tokenizer and add tests for all tokenizers
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #110 +/- ##
==========================================
- Coverage 91.55% 90.94% -0.61%
==========================================
Files 11 11
Lines 1397 1370 -27
Branches 303 287 -16
==========================================
- Hits 1279 1246 -33
- Misses 68 76 +8
+ Partials 50 48 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
I quite like this new proposed API. Would want @rebasedming to sign off on it, but I'd be in favour. Would we also make this change to the other ORMs, or is this Django-specific? |
|
Cool! One thought I had is that it would probably be better to use a dictionary for named args so that we don't have to try to keep all of them in sync; I expect that would be painful. So it would look something like this instead: Match("running shoes", operator="AND", tokenizer=Tokenizer.ngram(3,3, {"positions": "true"}))I realized after proposing this that SQLAlchemy has already had a similar API for specifying tokenizers for index creation specifically. But it isn't used in querying. So I went ahead and standardized use of that in my other PR. I haven't looked closely at that part in rails, but I'm sure we could do something similar. |
Even then, this looks better to me than the current situation. However, would it really be that painful? Tokenizers don't change much. We sometimes expose new fields from existing tokenizers, but rarely add new tokenizers now. I don't think it'd be that painful. Perhaps I'm understimating the issue or missing something? |
If they did change often, I could see it being hard to keep all the changes in sync as we don't have automated detection for that kind of change right now. It would be annoying for users to try to use a particular field and then find we hadn't added it yet. But, if they don't change very often then it seems fine to expose all of the arguments. It looks like each tokenizer has ~15 named args. Do we want to expose all of them in the function signatures? Do all of these look up to date to you? I tried using Seeing that each tokenizer has so many arguments is another reason I was initially hesitant to add them all. |
Oh man yeah that might be a lot then... I'll defer to Ming on this |
|
Thinking about this more, accepting a dictionary of named arguments instead of wrapping all of them seems like the way to go here to me. |
Sounds good to me. |
433d0c4 to
e947048
Compare
This reverts commit bbfeb71a58761c090b1e33164a352984b9104483.
Raise if an unkown tokenizer is used instead of quoting it.
This reverts commit 55891d6.
e947048 to
f9acdc8
Compare
# Ticket(s) Closed ## What Updates the doc snippets for each ORM to use the new tokenizer APIs and bumps the ORM versions. Once paradedb/django-paradedb#110 is merged and release this PR can be merged. ## Why ## How ## Tests
Ticket(s) Closed
What
Why
How
Tests
I added a test to explicitly exercise all of the tokenizers.