full-text search for query text, name, description #1400
Conversation
| db.database.execute_sql("CREATE INDEX tsv_idx ON queries USING gin(tsv)") | ||
| db.database.execute_sql("UPDATE queries SET tsv = setweight(to_tsvector(coalesce(name, '')), 'A') || setweight(to_tsvector(coalesce(description, '')), 'B') || setweight(to_tsvector(coalesce(query, '')), 'C')") | ||
| db.database.execute_sql(""" | ||
| CREATE FUNCTION query_search_trigger() RETURNS trigger AS $f$ |
There was a problem hiding this comment.
The migration will work for existing deployments. But what about new ones? Have you considered an before_save/after_save hook instead of a DB trigger?
Also, did you check if it's too much of a performance hit if we do the calculation of the vector in "realtime" instead of storing it?
There was a problem hiding this comment.
Also, need to update all existing queries, right?
There was a problem hiding this comment.
Good points. Might be worth leaving this off until after #1417 is done.
washort
force-pushed
the
query-search
branch
from
3c68c28
to
42ceb34
Compare
|
Updated to use alembic, tested upgrade with some sample queries. I didn't benchmark it but I'm fine with not adding the |
|
I will review this (and other pending pull requests) after master is stable and v1 is released. Just don't have the bandwidth to do both :( |
|
@arikfr Hey there, I'd like to help out with this feature and wondered if there is anything blocking from your side for the feature in general. Since I've got time to spend on this, please let me know how we can proceed. In terms of general direction of the feature I'd like to propose to move in a different direct than this PR and adopt SQLAlchemy-Searchable since it solves the full text search problem on top of PostgreSQL more complete, e.g. flask-sqlalchemy integration, advanced search queries with search operators (AND, OR, paratheses, negatives etc) and alembic integration. If that direction makes sense to you, I'd be happy to work on a PR to get the ball rolling again. |
|
Hi @jezdez 👋 SQLAlchemy-Searchable looks good and seems to make implementation much easier! Should I close this PR then? In the future it will be nice to have a unified search experience for all objects (queries, dashboards, alerts, users). I was wondering whether we should have a single table with the search vectors for all of them. But realized that we can continue with the current approach (each table has its own vector), but later create a view the unifies them. Thanks! |
|
@arikfr Hello :) Thanks for your quick reply! I think it's fair to close the PR, I'll keep an eye on it when I work on the implementation based on SQLAlchemy-Searchable so there are no surprises for the review. I'd like to suggest to collect some experience with the package with the queries first and then work from there to add a unified search, especially since there is likely some work to be done from the UX perspective to get the search results rendering right, since the objects are quite different in terms of what people may want to search for. |
Agreed. 👍 |
|
@washort thank you for "paving" the way 👏 |
Adds a tsvector field to
queriesand a trigger to automatically fill it when the query text or metadata changes.