add support for SQLite Full Text Search#623
Conversation
radex
left a comment
radex
left a comment
There was a problem hiding this comment.
Very good! Thank you so much @Kenneth-KT for working on this. I think we're very close to merge, I just want to cross the t's and dot the i's to make sure that 🍉 users don't accidentally use this feature in ways that it doesn't support yet
|
|
||
| it('supports textMatches as fts join', () => { | ||
| const query = Q.buildQueryDescription([ | ||
| Q.textMatches('searchable', 'hello world'), |
There was a problem hiding this comment.
this test doesn't seem right - did you mean Q.where('searchable', Q.textMatches('hello world'))?
BTW. This test shouldn't pass. Q.buildQueryDescription in DEV should probably check that args passed there are valid where/and/or -- but i guess that's out of scope for this PR
| if (column.isSearchable) { | ||
| logger.warn( | ||
| '[DB][Worker] Support for migrations and isSearchable is still to be implemented', | ||
| ) |
There was a problem hiding this comment.
If there's no support for schema migrations for FTS, that's fine, I can merge that - but throw an error if someone tries to do that - not just on add columns, but for create table migration as well
| Note that this only works when `useWebWorker: false` | ||
| - [SQLiteAdapter] Added support for Full Text Search for SQLite adapter: | ||
| Add `isSearchable` boolean flag to schema column descriptor for creating Full Text Search-able columns | ||
| Add `Q.textMatches(value)` that compiles to `match 'value'` SQL for performing Full Text Search using SQLite adpater |
There was a problem hiding this comment.
can you add to docs-master/ .... queries document a few words about Full Text Search?
| return value.replace(nonLikeSafeRegexp, '_') | ||
| } | ||
|
|
||
| export function textMatches(value: string): Comparison { |
There was a problem hiding this comment.
one more place that takes Queries is encodeMatcher (used by query observation system). I think it would be appropriate to throw an error if you try to encode a matcher for a query that contains the unsupported textMatches operator. (Even better would be to support it, or to make .observe() de-opt to reloadingObserver, but that's out of scope for this PR)
|
hey @Kenneth-KT -- will you have time in the coming weeks to look at my comments and finish your two awesome PRs? I'd love to see them merged |
|
@Kenneth-KT I'm interested in getting this PR to the finish line. Do you have time to work on this? If not, perhaps I will. Another question: Are you currently shipping an app that uses this patch, or is it only in development? I want to change names of the virtual FTS tables, but if you're already shipping, maybe I don't care so much and will avoid causing trouble for you and your users |
|
@Kenneth-KT Hey, pinging you again. Can you please take a look at the comment above? |
|
@radex is this PR stale? I might have some time to finish it I tried to rebase and making it work, but I noticed this solution will bloat my database too much and it seems to have other ways to do the same. I may help if I better understand the expected result and why something like following isn't enough: |
|
@sidferreira Yes, stale. Would very much like to see it finished up, but didn't have the need myself, and the OP disappeared. So please do! Read up on SQLite Full Text Search. If for your application, using Q.like is enough -- great! Use it. FTS adds overhead, db size, etc... The reward is there for large databases, and also FTS finds different results from Q.like |
|
Closing in favor of #984. Thank you @Kenneth-KT, @stigi & @sidferreira ! |
4 participants
[SQLiteAdapter] Added support for Full Text Search for SQLite adapter:
Add
isSearchableboolean flag to schema column descriptor for creating Full Text Search-able columnsAdd
Q.textMatches(value)that compiles tomatch 'value'SQL for performing Full Text Search using SQLite adpater