Skip to content
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

[F] Implement PG search filtering #3826

Merged
merged 13 commits into from
Mar 24, 2025
Merged

[F] Implement PG search filtering #3826

merged 13 commits into from
Mar 24, 2025

Conversation

timbot1789
Copy link
Contributor

@timbot1789 timbot1789 commented Mar 18, 2025

This implements a separate Filtering applicator that supports using the pg_search gem rather than searchkick. It also modifies several models to support pg_search based filtering. Currently pg_search can be activated in an environment by setting an environment variable 'USE_PG_SEARCH' to true.

See the apply_spec.rb for a list of currently supported models

Models that do not need to support pg_search because they already implement custom by_keyword:

  • entitlement
  • annotation
  • reading group
  • comment

Some Notes / Important differences from Searchkick:

  • pg_search can search through associations, but it's somewhat limited. It can't search more than one association deep, and it doesn't index the association. So we'll want to keep an eye on performance for some association-heavy searches.
  • Since pg_search calls into Postgres, not Ruby, all field names passed to it need to be the postgres name for the corresponding column, not the ruby method name. So if we have a column name that we alias in rails with alias name title, we need to pass pg_search name, not title. This also means that fields like title_plaintext are unavailable. They could be implemented as a PG function, however.

Possible Improvements:

  • I haven't tried to implement highlighting here, mostly because I haven't seen it in the front end in areas that seemed to be calling into keyword search. pg_search supports it though, so we can add it in another PR.
  • I haven't intensely tested around typos, incomplete words, etc

@timbot1789 timbot1789 changed the title Ts/integrate pg search [WIP] Implement PG search filtering Mar 18, 2025
@timbot1789 timbot1789 force-pushed the ts/integrate-pg-search branch 2 times, most recently from 26a87c9 to 6d07cbe Compare March 18, 2025 19:23
@timbot1789 timbot1789 force-pushed the ts/integrate-pg-search branch from 6d07cbe to ae6c881 Compare March 18, 2025 22:17
@timbot1789 timbot1789 force-pushed the ts/integrate-pg-search branch from 6f153f1 to 4af2d63 Compare March 18, 2025 23:41
@timbot1789 timbot1789 requested a review from scryptmouse March 18, 2025 23:51
@timbot1789 timbot1789 marked this pull request as ready for review March 19, 2025 23:00
@timbot1789 timbot1789 force-pushed the ts/integrate-pg-search branch from ff13d3c to 22585a6 Compare March 19, 2025 23:29
Copy link
Contributor

@scryptmouse scryptmouse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good! I think putting one abstraction in would be beneficial to build off of.

@timbot1789 timbot1789 force-pushed the ts/integrate-pg-search branch from 992c560 to f72ff2b Compare March 20, 2025 23:46
@timbot1789 timbot1789 requested a review from scryptmouse March 20, 2025 23:59
@timbot1789 timbot1789 force-pushed the ts/integrate-pg-search branch from 45bf388 to 29c0178 Compare March 21, 2025 18:48
@timbot1789 timbot1789 changed the title [WIP] Implement PG search filtering [F] Implement PG search filtering Mar 21, 2025
Copy link
Contributor

@scryptmouse scryptmouse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is continuing to look good!

@timbot1789 timbot1789 merged commit 3e4c98b into master Mar 24, 2025
3 checks passed
@timbot1789 timbot1789 deleted the ts/integrate-pg-search branch March 24, 2025 15:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants