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

Adds opensearch geo_bounding_box query support #789

Merged
merged 1 commit into from
Feb 13, 2024
Merged

Conversation

JPrevost
Copy link
Member

@JPrevost JPrevost commented Feb 12, 2024

This has not yet been integrated with GraphQL, so the best way to validate this work is to use the rails console. To test locally, you'll need an opensearch instance running with appropriate data loaded.

bin/rails console

q = {geobox: {max_latitude: "42.886",
              min_latitude: "41.239",
              max_longitude: "-73.928",
              min_longitude: "-69.507"}}

Opensearch.new.search(0, q, Timdex::OSClient)

You can also combine keyword (or filters) using our existing query syntax:

q = {geobox: {max_latitude: "42.886",
              min_latitude: "41.239",
              max_longitude: "-73.928",
              min_longitude: "-69.507"},
       q: "rail stations"}

Opensearch.new.search(0, q, Timdex::OSClient)

Relevant ticket(s):

How does this address that need:

  • Adds an optional geo_bounding_box search to our existing opensearch query builder

Developer

  • All new ENV is documented in README
  • All new ENV has been added to Heroku Pipeline, Staging and Prod
  • ANDI or Wave has been run in accordance to
    our guide and
    all issues introduced by these changes have been resolved or opened as new
    issues (link to those issues in the Pull Request details above)
  • Stakeholder approval has been confirmed (or is not needed)

Code Reviewer

  • The commit message is clear and follows our guidelines
    (not just this pull request message)
  • There are appropriate tests covering any new functionality
  • The documentation has been updated or is unnecessary
  • The changes have been verified
  • New dependencies are appropriate or there were no changes

Requires database migrations?

NO

Includes new or updated dependencies?

NO

@mitlib mitlib temporarily deployed to timdex-pr-789 February 12, 2024 19:44 Inactive
@matt-bernhardt matt-bernhardt self-assigned this Feb 13, 2024
Copy link
Member

@matt-bernhardt matt-bernhardt left a comment

Choose a reason for hiding this comment

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

I have a fiddly question about the refutation test, but I don't think it is a blocking concern. If you'd rather merge this as-is rather than make a change, I'm fine with that.

:shipit:

@@ -279,6 +279,8 @@ class OpensearchTest < ActiveSupport::TestCase
os.instance_variable_set(:@params,
{ geodistance: { latitude: '42.361145', longitude: '-71.057083', distance: '50mi' } })

refute(os.matches.to_json.include?('{"multi_match":{"query":"rail stations","fields":'))
Copy link
Member

Choose a reason for hiding this comment

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

The intent of this test is to confirm that the query builder isn't hallucinating a keyword search? I think I can understand that motivation, but wonder if there's a smaller string match to look for - like maybe refute on just multi_match or something. The way this reads right now, I think hallucinating a search for popcorn would pass the test - as long as it doesn't hallucinate rail stations?

Copy link
Member Author

Choose a reason for hiding this comment

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

I narrowed it down and am just squashing/merging without a review as it was so minor. Thanks again.

}
end

# https://www.elastic.co/guide/en/elasticsearch/reference/7.17/query-dsl-geo-distance-query.html
Copy link
Member

Choose a reason for hiding this comment

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

Weird that they haven't created a page for this type of search, but I see what you're talking about.

Copy link
Member Author

Choose a reason for hiding this comment

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

I suspect the Docs licensing didn't allow a fork so I believe OpenSearch had to throw away all the docs and start over whereas with the code they had the fork... so docs required a lot of work to build back up.

@JPrevost
Copy link
Member Author

@matt-bernhardt I agree with your comment. I added that explicitly to make the pairs of tests more clear as to what was happening in the method being tested between the two, but you are absolutely correct that the refute statement is so specific that many other things could be going wrong that it isn't testing. I'll give it peek and see if I can narrow it down in a meaningful way. If not, I may just let it slide but I appreciate pointing it out!

Relevant ticket(s):

* https://mitlibraries.atlassian.net/browse/GDT-167

How does this address that need:

* Adds an optional geo_bounding_box search to our existing opensearch
  query builder
@JPrevost JPrevost merged commit 2d200fd into main Feb 13, 2024
3 of 4 checks passed
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.

3 participants