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.

Sign up for GitHub

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

Jump to bottom

[ENG-6576][ENG-6586] fix: duplicate value-search values #832

Merged
merged 2 commits into from
Nov 21, 2024
Merged

[ENG-6576][ENG-6586] fix: duplicate value-search values #832

merged 2 commits into from
Nov 21, 2024

Conversation

aaxelb
Copy link
Contributor

@aaxelb aaxelb commented Nov 20, 2024

  • when indexing iri values for value-search aggregations, drop trailing slash and ensure a single iri (not including synonyms)
  • update trovesearch_denorm mappings: single iri for each value, better synonym handling
  • update tests with fixes while leaving old _flats strategy as-is (because it'll be removed before long...)

@aaxelb
fix: drop trailing slashes for value-search
cc2e453 
update denorm mappings: single iri for each value, better synonyms

update tests with fixes while leaving old _flats strategy as-is
@aaxelb aaxelb changed the title [ENG-6576] drop trailing slashes for value-search [ENG-6576] fix: duplicate value-search values Nov 20, 2024
@aaxelb aaxelb marked this pull request as ready for review November 20, 2024 16:49
@aaxelb aaxelb requested a review from mfraezz November 20, 2024 16:49
@coveralls
Copy link

coveralls commented Nov 20, 2024 •
edited
Loading

Coverage Status

coverage: 91.202% (+0.01%) from 91.191%
when pulling 63740ca on aaxelb:fix/valuesearch-trailing-slashes
into 1db8e75 on CenterForOpenScience:develop.

mfraezz
mfraezz reviewed Nov 21, 2024
View reviewed changes
Copy link
Member

@mfraezz mfraezz left a comment

Choose a reason for hiding this comment

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

Generally looks good, however this change will require a reindex, which also means it'd be a good time to implement sharding. Could that be added to this PR, or should it be separate?

Pass complete :octocat:

@aaxelb aaxelb force-pushed the fix/valuesearch-trailing-slashes branch from 79e7c4f to 63740ca Compare November 21, 2024 18:27
@aaxelb aaxelb merged commit 39df273 into CenterForOpenScience:develop Nov 21, 2024
3 checks passed
@aaxelb aaxelb deleted the fix/valuesearch-trailing-slashes branch November 21, 2024 18:45
@aaxelb aaxelb changed the title [ENG-6576] fix: duplicate value-search values [ENG-6576][ENG-6586] fix: duplicate value-search values Nov 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mfraezz mfraezz mfraezz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@aaxelb @coveralls @mfraezz