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

feat!: Add substring search support for more fields #3028

Merged
merged 8 commits into from
Nov 18, 2024

Conversation

fhennig
Copy link
Contributor

@fhennig fhennig commented Oct 21, 2024

resolves #2684

preview URL: https://feat-2684-substring-searc.loculus.org

Summary

Add support for regex search for more fields, by making regex-searchability configurable in the Helm chart values.yaml.
Support is enabled for submissionId, authors, authorAffiliations, specimenCollectorSampleId

Screenshot

TODO

PR Checklist

  • All necessary documentation has been adapted.
  • The implemented feature is covered by an appropriate test.

@fhennig fhennig self-assigned this Oct 21, 2024
@fhennig fhennig added the preview Triggers a deployment to argocd label Oct 21, 2024
@fhennig
Copy link
Contributor Author

fhennig commented Oct 21, 2024

From the preview it looks like it's working.

@theosanderson
Copy link
Member

Cool - code looks nice to me. This is a config-breaking change so we should include ! in the PR name and maybe [CFG] or whatever we decided to ensure we change pathoplexus in the same way at rollout

@theosanderson
Copy link
Member

Ah, we should potentially call this "substring" search, at least from the POV of the website

@theosanderson
Copy link
Member

I think @corneliusroemer will have stronger view than me on calling it substring search and might have suggestions for how he thinks the config should look (sometimes it is regex - i.e. on the LAPIS side)

@corneliusroemer corneliusroemer changed the title feat: Add regex search for more fields feat!: Add regex search for more fields Oct 23, 2024
@corneliusroemer
Copy link
Contributor

Cool - code looks nice to me. This is a config-breaking change so we should include ! in the PR name and maybe [CFG] or whatever we decided to ensure we change pathoplexus in the same way at rollout

I edited the PR title

Copy link
Contributor

@corneliusroemer corneliusroemer left a comment

Choose a reason for hiding this comment

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

As @theosanderson said, this is substring search not regex search (we're using the LAPIS regex feature but this feature here crafts a regex that does substring search only, not general regex)

@fhennig fhennig force-pushed the feat/2684-substring-search-for-more-fields branch from a544419 to d87f112 Compare October 23, 2024 14:13
@fhennig
Copy link
Contributor Author

fhennig commented Oct 23, 2024

Ahh, I didn't understand it at first, but now I got it. I made a change, that should fix what you meant right?

In the website config the property is now called 'substringSearch' (akin to rangeSearch) and in the values.yaml it's 'enableSubstringSearch' (like generateIndex, hideOnSequenceDetailsPage)

@fhennig fhennig changed the title feat!: Add regex search for more fields feat!: Add substring search support for more fields Oct 23, 2024
@fhennig fhennig marked this pull request as ready for review October 24, 2024 06:42
kubernetes/loculus/values.yaml Outdated Show resolved Hide resolved
@corneliusroemer
Copy link
Contributor

corneliusroemer commented Oct 29, 2024

Another possible bug
https://feat-2327-better-download.loculus.org/cchf/search?visibility_submissionId=true

Brave Browser 2024-10-29 14 42 07

I tested on main.loculus.org and there it seems to not show the error

Ignore, seems to be general instability of previews right now due to too much load

@corneliusroemer corneliusroemer removed the preview Triggers a deployment to argocd label Oct 31, 2024
@fhennig fhennig force-pushed the feat/2684-substring-search-for-more-fields branch from d87f112 to 17c0f91 Compare November 5, 2024 12:22
@fhennig fhennig added the preview Triggers a deployment to argocd label Nov 5, 2024
@fhennig
Copy link
Contributor Author

fhennig commented Nov 5, 2024

made the changes. TODO: check if it works in the preview

Copy link
Member

@chaoran-chen chaoran-chen left a comment

Choose a reason for hiding this comment

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

Works and looks good to me!

@fhennig fhennig force-pushed the feat/2684-substring-search-for-more-fields branch from 0a1909f to 6a47777 Compare November 12, 2024 23:07
@emmahodcroft
Copy link
Member

If possible please do ping somewhere on slack when this goes live so I can be sure to add it to our running list of new items under technical improvements :)

@chaoran-chen
Copy link
Member

@emmahodcroft, this PR only adds the feature to support more substring fields to Loculus. Afterwards, we still have to define the fields for Pathoplexus: pathoplexus/pathoplexus#79 - You and Cornelius named already a few fields but do you have more in mind? Would be good to have a complete list.

@fhennig fhennig enabled auto-merge (squash) November 18, 2024 09:10
@chaoran-chen chaoran-chen dismissed corneliusroemer’s stale review November 18, 2024 09:46

I think that the review is now outdated.

@fhennig fhennig merged commit f6224fe into main Nov 18, 2024
20 checks passed
@fhennig fhennig deleted the feat/2684-substring-search-for-more-fields branch November 18, 2024 09:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
preview Triggers a deployment to argocd
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add substring search for more fields
5 participants