Add full text search for queries based on the Postgres tsvector type. #2041

jezdez · 2017-10-17T21:39:17Z

This needs some more tests with a prod like database and probably another
look for someone with more experience with custom alembic migrations.

This is related to #1400 of course and uses SQLAlchemy-searchable.

arikfr · 2017-10-18T12:04:14Z

migrations/versions/5ec5c84ba61e_.py

+def downgrade():
+    conn = op.get_bind()
+
+    ss.drop_trigger(conn, 'article', 'search_vector')


arikfr · 2017-10-18T12:04:48Z

redash/cli/database.py

@@ -7,6 +7,8 @@
 def create_tables():
    """Create the database tables."""
    from redash.models import db
+    # make sure the additional search mappers are configured
+    db.configure_mappers()


What are the search mappers?

The comment is misleading, this basically makes sure that all mappers that have been registered with the SQLAlchemy ORM have been properly configured. Since SQLAlchemy-Searchable installs database triggers that have to act on when ORM model instances are created (e.g. to track updates), this call is needed.

Here's what the SA-Searchable docs say:

It is very important here that you either access your searchable class or call configure_mappers before the creation of tables. SA-Searchable adds DDL listeners on the configuration phase of models.

Since this code happens in the phase in which tables are created, we can't really access the searchable class, but have to call configure_mappers. I'll update the comment to reflect this.

arikfr · 2017-10-18T12:06:55Z

redash/models.py

-            where |= Query.id == term
-
-        where &= Query.is_archived == False
+    def search_queries(cls, term, group_ids, include_drafts=False, limit=20):


Why append _queries? It's a method on the Query class isn't it obvious that it searches for queries? And in the handlers code you still call .search...

Ugh, yeah, this is just a leftover from figuring out how to use the search method that the SearchQueryMixin adds to the query_class of the Query model. Will fix.

arikfr · 2017-10-18T12:08:56Z

redash/models.py

 from sqlalchemy.orm.exc import NoResultFound  # noqa: F401
 from sqlalchemy.types import TypeDecorator
 from functools import reduce
+from sqlalchemy_searchable import SearchQueryMixin, make_searchable, search
+from sqlalchemy_utils.types import TSVectorType


Not that I mind much about adding sqlalchemy_utils as a dependency, but doesn't sqlalchemy_searchable provide something for this?

Yeah, the SQLAlchemy-Utils package is from the same author as SQLAlchemy-Searchable and contains a whole bunch of things.

The code behind TSVectorType is trivial relatively speaking, so I guess we could also just ship our own. The only risk I see with this is that we could trade one dependency less for having to keep an eye on changes in SQLAlchemy-Searchable that require changes in the TSVectorType class as well.

On the other hand, although quite out of scope of this PR, is the question if there are things in SQLAlchemy-Utils that may be useful for Redash in general, given its swiss-army-knife-ness. I'm fine with whatever way you want to go with this.

👍 ok, let's use it. No need to find more places to use it, we can gradually introduce it where applicable.

arikfr · 2017-10-24T10:04:49Z

migrations/versions/5ec5c84ba61e_.py

+                                       nullable=True))
+
+    ss.sync_trigger(conn, 'queries', 'search_vector',
+                    ['name', 'description', 'query'])


Will this also update all existing queries?

Yes, looking at the docs it says: "Updates all rows for given search vector by running a column=column update query for given table."

I've also manually added a bunch of queries against Redash's own Postgres database in the master branch, then switch to this branch, did the Alembic upgrade and then was able to use the search feature right away.

arikfr · 2017-10-24T10:05:13Z

migrations/versions/5ec5c84ba61e_.py

+    conn = op.get_bind()
+
+    ss.drop_trigger(conn, 'queries', 'search_vector')
+    op.drop_index('ix_queries_search_vector', table_name='queries')


This is a byproduct of ss.sync_trigger?

Yep, although weirdly it raised a ProgrammingError exception when I tried roll it back, will look into this.

This was an oversight when setting up SA-Searchable. The migration that I fixed in eb6966f now reflects adding an index for the search vector column. Another call to db.configure_mappers() was needed to have the mapper processor of SA-Searchable to correctly add the index.

👍

Another call to db.configure_mappers() was needed

It seems that it's at the top level of redash.models. So I guess the one in redash.cli.database is redundant?

True indeed, good catch, removed it in 36aeef7.

jezdez · 2017-10-26T16:17:41Z

@arikfr Just added a bunch of tests for the more advanced search queries as described on https://sqlalchemy-searchable.readthedocs.io/en/latest/search_query_parser.html.

arikfr · 2017-11-22T16:33:38Z

@jezdez this is finalyl ready to be merged, but I think I saw some fixes you did to this code in Mozilla's fork. Are they present here already or this need an update?

Thanks.

jezdez · 2017-11-22T16:49:38Z

@arikfr They aren't in this PR yet, I'll get them in today and let you know. Thanks for reaching out again!

arikfr · 2017-11-22T16:50:44Z

Super, thanks!

jezdez · 2018-01-08T21:49:52Z

@arikfr Okay, I've updated the PR with the recent things in our fork and updated the design to match the query-list and my-queries page as well. Does this work?

jezdez · 2018-01-08T21:50:32Z

@arikfr BTW, apologies for taking so long :(

jezdez · 2018-01-10T18:56:29Z

@arikfr Any idea how we can retrigger the code climate check? It seems to be stuck.

arikfr · 2018-01-11T18:46:46Z

Thanks ! 👍

Add full text search for queries based on the Postgres tsvector type.

jezdez force-pushed the fulltext-search branch 2 times, most recently from 2d70074 to 6164efa Compare October 17, 2017 21:47

arikfr reviewed Oct 18, 2017

View reviewed changes

arikfr reviewed Oct 24, 2017

View reviewed changes

jezdez force-pushed the fulltext-search branch 2 times, most recently from 36aeef7 to 294cff0 Compare October 26, 2017 16:16

jezdez force-pushed the fulltext-search branch from 294cff0 to a9cc685 Compare October 27, 2017 15:17

jezdez mentioned this pull request Oct 27, 2017

Add full text search for queries based on the Postgres tsvector type. mozilla/redash#270

Merged

jezdez force-pushed the fulltext-search branch 2 times, most recently from f80e088 to 388ec5e Compare January 8, 2018 20:46

Add full text search for queries based on the Postgres tsvector type.

9f6b4fb

jezdez force-pushed the fulltext-search branch from 388ec5e to 9f6b4fb Compare January 8, 2018 21:47

jezdez mentioned this pull request Jan 10, 2018

Query search result is limited to 20 queries mozilla/redash#298

Closed

jezdez added a commit to mozilla/redash that referenced this pull request Jan 10, 2018

Backport a few UI fixes from getredash#2041.

22d940d

arikfr merged commit 691dc6a into getredash:master Jan 11, 2018

kyoshidajp mentioned this pull request Jun 23, 2018

Can't search query correctly with non-ASCII chars #2622

Closed

dairyo pushed a commit to KiiCorp/redash that referenced this pull request Mar 1, 2019

Merge pull request getredash#2041 from jezdez/fulltext-search

d3d2af3

Add full text search for queries based on the Postgres tsvector type.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add full text search for queries based on the Postgres tsvector type. #2041

Add full text search for queries based on the Postgres tsvector type. #2041

jezdez commented Oct 17, 2017 •

edited

Loading

arikfr Oct 18, 2017

jezdez Oct 23, 2017

arikfr Oct 18, 2017

jezdez Oct 23, 2017 •

edited

Loading

arikfr Oct 18, 2017

jezdez Oct 23, 2017

arikfr Oct 18, 2017

jezdez Oct 23, 2017

arikfr Oct 24, 2017

arikfr Oct 24, 2017

jezdez Oct 25, 2017

arikfr Oct 24, 2017

jezdez Oct 25, 2017

jezdez Oct 25, 2017

arikfr Oct 25, 2017

jezdez Oct 25, 2017

jezdez commented Oct 26, 2017

arikfr commented Nov 22, 2017

jezdez commented Nov 22, 2017

arikfr commented Nov 22, 2017

jezdez commented Jan 8, 2018

jezdez commented Jan 8, 2018

jezdez commented Jan 10, 2018

arikfr commented Jan 11, 2018

Add full text search for queries based on the Postgres tsvector type. #2041

Add full text search for queries based on the Postgres tsvector type. #2041

Conversation

jezdez commented Oct 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jezdez Oct 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jezdez commented Oct 26, 2017

arikfr commented Nov 22, 2017

jezdez commented Nov 22, 2017

arikfr commented Nov 22, 2017

jezdez commented Jan 8, 2018

jezdez commented Jan 8, 2018

jezdez commented Jan 10, 2018

arikfr commented Jan 11, 2018

jezdez commented Oct 17, 2017 •

edited

Loading

jezdez Oct 23, 2017 •

edited

Loading