Skip to content
This repository has been archived by the owner on Oct 16, 2024. It is now read-only.

Fix boolean search #2132

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

Conversation

kilbergr
Copy link
Collaborator

@kilbergr kilbergr commented Apr 6, 2023

This PR addresses the issue around boolean search failure due to ES not searching across multiple fields when using the AND keyword.
To fix, I implemented copy_to to grab the content of all the fields we wanted to search into a single field that could be searched.
This is what happens when you search "Argued before BELL" AND "rationale of Gardner" on the live site:
Screenshot 2023-04-06 at 3 47 47 PM
That's because the first phrase is in the head_matter field and the second is in casebody_data.opinions.text.

This is what happens when you run the same search with this change:
Screenshot 2023-04-06 at 3 48 39 PM

Deploying this will require running fab rebuild_search_index on prod, so we will have to plan when that is appropriate.

@kilbergr kilbergr marked this pull request as ready for review April 6, 2023 19:50
@kilbergr kilbergr requested a review from a team as a code owner April 6, 2023 19:50
@kilbergr kilbergr requested review from bensteinberg and removed request for a team April 6, 2023 19:50
@kilbergr kilbergr changed the title Rek update search Fix boolean search Apr 6, 2023
@codecov
Copy link

codecov bot commented Apr 6, 2023

Codecov Report

Merging #2132 (d881f5b) into develop (b96efcb) will increase coverage by 0.00%.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           develop    #2132   +/-   ##
========================================
  Coverage    63.19%   63.20%           
========================================
  Files          106      106           
  Lines        11518    11521    +3     
========================================
+ Hits          7279     7282    +3     
  Misses        4239     4239           

see 3 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Contributor

@bensteinberg bensteinberg left a comment

Choose a reason for hiding this comment

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

LGTM! But I will add @jcushman as a reviewer as well.

@bensteinberg bensteinberg requested a review from jcushman April 6, 2023 19:58
@jcushman
Copy link
Contributor

jcushman commented Apr 7, 2023

LGTM2! Note that this will increase the index size, so disk space on prod could be a question; not sure if there's a way to estimate.

@kilbergr
Copy link
Collaborator Author

kilbergr commented Apr 7, 2023

That is a good question. Let me try to find out.
Also @bensteinberg I wondered whether we'd want to push the piece that is necessary for the reindex, run the reindex, then push the change to search. Idk if that's overkill...

@kilbergr
Copy link
Collaborator Author

kilbergr commented Apr 7, 2023

@jcushman can you clarify for me: what is our plan should the index size outgrow disk space? Will we increase the disk space?

@mdellabitta
Copy link

@jcushman can you clarify for me: what is our plan should the index size outgrow disk space? Will we increase the disk space?

That's my expectation. But Ben would have to be in charge of how that happens, and LOE.

SFQUEEN94

This comment was marked as spam.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants