-
-
Notifications
You must be signed in to change notification settings - Fork 502
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
feat: add support for full text searches #3317
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3317 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 203 205 +2
Lines 12456 12534 +78
Branches 2872 2889 +17
=========================================
+ Hits 12456 12534 +78
Continue to review full report at Codecov.
|
This pull request introduces 1 alert when merging 5f8033e into 2380f0a - view on LGTM.com new alerts:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking great, thanks a lot! Did a quick first pass and left some comments. Would be definitely good to describe this more in the docs, otherwise it could get lost.
Co-authored-by: Martin Adámek <banan23@gmail.com>
Co-authored-by: Martin Adámek <banan23@gmail.com>
Co-authored-by: Martin Adámek <banan23@gmail.com>
Co-authored-by: Martin Adámek <banan23@gmail.com>
Co-authored-by: Martin Adámek <banan23@gmail.com>
This pull request introduces 1 alert when merging f6aaf2d into 2380f0a - view on LGTM.com new alerts:
|
It would be also great to catch up with the code coverage. But I am open for merging it without full branch coverage, will be happy to fill that gap myself afterwards. |
What to do in the future:
Do I forget any? |
I have covered some more. Now we need two more tests:
For the second I tried to switch the column type to 'tsvector' by doing |
Oh right, I see what's happening, unknown types will get mapped to Btw I had to remove the default value (empty string), otherwise the query would fail. Maybe that's something to handle as well, not sure what the semantics are, never user this feature myself. |
Nice, I will merge the changes and rewrite the test. Thanks!
You mean the column default? I will test it now to see what you mean. I added tsvector columns with a default of empty string and it worked fine? Edit: I see what you mean, if the column is changed to type tsvector it doesn't understand the existing default value. How could we hook into this? It has to update the default value (from Edit: I still have no clue how we could check the default type. It only occurs when you are changing a column type to tsvector. I have added a custom type for performant full text searches exported as |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gave it another look, last piece I havent really reviewed is the docs.
Generally looking pretty much ready to me, just few nits.
I'd like to include this in 5.3, ideally I would ship that today evening or something like that. Let me know if you have time to finish this, I'll be happy to merge it and do the final steps myself if not, in the end its pretty much done. |
Co-authored-by: Martin Adámek <banan23@gmail.com>
Co-authored-by: Martin Adámek <banan23@gmail.com>
I have applied your suggestions, I will not be home until later tonight. So if you could apply the last suggestions, that’d be great (especially if you ship it tonight). Else i can implement them around 22 or 23h. |
anyone experiencing this in postgres? for migration entity i tried using this but im not sure what values to add in db migration. Thanks |
This seems wrong, I think you should not override the column type, it needs to be
I dont understand this, you are not using schema generator? |
The tabs on the documentation site don't show correctly (https://mikro-orm.io/docs/query-conditions#postgresql). I understand the confusion. It should show two possible ways to do FTS on Postgres.
As noted above, you use
|
Thank you for all your efforts. I'm using PostgreSQL. Is there any update for the
|
Btw, possible to include the highlighting function? Thank you, |
Just reviewed your code and found the to_tsvector was written fixed in the code https://github.com/mikro-orm/mikro-orm/blob/8b8f14071b92e91161a32aa272315a0ecce1bc0b/packages/postgresql/src/types/FullTextType.ts#L13 so it's not possible to use Here is a code snippet which might be an option. @Index({ type: 'fulltext' })
@Property({
type: FullTextType,
nullable: true,
onCreate: (book: Book) => `setweight(to_tsvector('english', '${book.title}'),'A') || setweight(to_tsvector('english', '${book.description}'),'B')`,
onUpdate: (book: Book) => `setweight(to_tsvector('english', '${book.title}'),'A') || setweight(to_tsvector('english', '${book.description}'),'B')`,
})
searchableContent!: string; @jsprw Let me know your comments and see if I can help. Thanks, |
You can create your own |
That's right. Have to update AbstractSqlDriver.ts to make the proposed API work. |
As said above, create your own custom type (or maybe extend the existing |
I have not worked on any of the proposed updates yet, have to find some time to work on them. I will suggest some changes below, if those are fine. I will try to implement them this week.
This is a change which I would like to implement myself as well. As @B4nan already mentioned, you could extend the @Index({ type: 'fulltext' })
@Property({ type: new FullTextType("english"), onUpdate: (book) => book.title })
searchableTitle!: string; Altough this would work if changed to english, the search query would still be converted to regconfig
So, we would also need to access this regconfig property in the methods where the full text indexes are created or converted to be used in a query. @B4nan: don't have the project open right now, but would this information be available in these methods?
Reading https://www.postgresql.org/docs/current/textsearch-controls.html, I see that you only have the letters A till D for the weighted function. What about adding an check that if you set an object (with type Then you can use it like this which is converted to the right query. @Property({
type: FullTextType,
nullable: true,
onCreate: (book: Book) => { [FullTextWeight.A]: book.title, [FullTextWeight.B]: book.description },
onUpdate: (book: Book) => { [FullTextWeight.A]: book.title, [FullTextWeight.B]: book.description },
})
searchableContent!: string; It means that we would have to detect an object in the method
It would probably work like this (haven't tested yet): convertToDatabaseValueSQL(key: string | Partial<Record<FullTextWeight, string>>) {
if (typeof key === "object") {
return Object.entries(key).map(([weight, value]) => `setweight(to_tsvector(coalesce(${value},'')), '${weight}')`).join(" || ");
}
return `to_tsvector('simple', ${key})`;
} ( |
In QB yes, you can get the property object and access the type instance, in SchemaGenerator you'd have to propagate it in the place where we convert metadata to I don't mind adding the constructor, PR welcome. |
Thank you. I will look into this. @jsprw your idea looks much more promising. Thanks for taking the time to do this. |
…nfig (#3805) Adds support for a custom regconfig and weighted tsvectors as requested by #3317 (comment). For more context, see #3317 (comment). Docs: also (hopefully) fixed the tabs which caused the confusion as seen here: #3317 (comment).
…nfig (#3805) Adds support for a custom regconfig and weighted tsvectors as requested by #3317 (comment). For more context, see #3317 (comment). Docs: also (hopefully) fixed the tabs which caused the confusion as seen here: #3317 (comment).
…nfig (#3805) Adds support for a custom regconfig and weighted tsvectors as requested by #3317 (comment). For more context, see #3317 (comment). Docs: also (hopefully) fixed the tabs which caused the confusion as seen here: #3317 (comment).
…nfig (#3805) Adds support for a custom regconfig and weighted tsvectors as requested by #3317 (comment). For more context, see #3317 (comment). Docs: also (hopefully) fixed the tabs which caused the confusion as seen here: #3317 (comment).
…nfig (#3805) Adds support for a custom regconfig and weighted tsvectors as requested by #3317 (comment). For more context, see #3317 (comment). Docs: also (hopefully) fixed the tabs which caused the confusion as seen here: #3317 (comment).
…nfig (#3805) Adds support for a custom regconfig and weighted tsvectors as requested by #3317 (comment). For more context, see #3317 (comment). Docs: also (hopefully) fixed the tabs which caused the confusion as seen here: #3317 (comment).
…nfig (#3805) Adds support for a custom regconfig and weighted tsvectors as requested by #3317 (comment). For more context, see #3317 (comment). Docs: also (hopefully) fixed the tabs which caused the confusion as seen here: #3317 (comment).
…nfig (#3805) Adds support for a custom regconfig and weighted tsvectors as requested by #3317 (comment). For more context, see #3317 (comment). Docs: also (hopefully) fixed the tabs which caused the confusion as seen here: #3317 (comment).
…nfig (#3805) Adds support for a custom regconfig and weighted tsvectors as requested by #3317 (comment). For more context, see #3317 (comment). Docs: also (hopefully) fixed the tabs which caused the confusion as seen here: #3317 (comment).
…nfig (#3805) Adds support for a custom regconfig and weighted tsvectors as requested by #3317 (comment). For more context, see #3317 (comment). Docs: also (hopefully) fixed the tabs which caused the confusion as seen here: #3317 (comment).
…nfig (#3805) Adds support for a custom regconfig and weighted tsvectors as requested by #3317 (comment). For more context, see #3317 (comment). Docs: also (hopefully) fixed the tabs which caused the confusion as seen here: #3317 (comment).
…nfig (#3805) Adds support for a custom regconfig and weighted tsvectors as requested by #3317 (comment). For more context, see #3317 (comment). Docs: also (hopefully) fixed the tabs which caused the confusion as seen here: #3317 (comment).
…nfig (#3805) Adds support for a custom regconfig and weighted tsvectors as requested by #3317 (comment). For more context, see #3317 (comment). Docs: also (hopefully) fixed the tabs which caused the confusion as seen here: #3317 (comment).
…nfig (#3805) Adds support for a custom regconfig and weighted tsvectors as requested by #3317 (comment). For more context, see #3317 (comment). Docs: also (hopefully) fixed the tabs which caused the confusion as seen here: #3317 (comment).
…nfig (#3805) Adds support for a custom regconfig and weighted tsvectors as requested by #3317 (comment). For more context, see #3317 (comment). Docs: also (hopefully) fixed the tabs which caused the confusion as seen here: #3317 (comment).
…nfig (#3805) Adds support for a custom regconfig and weighted tsvectors as requested by #3317 (comment). For more context, see #3317 (comment). Docs: also (hopefully) fixed the tabs which caused the confusion as seen here: #3317 (comment).
Add a $fulltext operator that can be used in queries on SQL databases.
Based on #621 by @thekevinbrown. This version has also added support for creating required indexes with the decorator
@Index({ type: 'fulltext' })
and full text searching in MongoDB. Documentation could possibly be improved with more clear instructions.Closes: #620