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

[ENG-6189] trovesearch denormalized #828

Merged
merged 14 commits into from
Nov 12, 2024

Conversation

aaxelb
Copy link
Contributor

@aaxelb aaxelb commented Oct 25, 2024

  • add trovesearch_denorm index strategy to speed up /trove/index-card-search and /trove/index-value-search requests
    • used by default when the TROVESEARCH_DENORMILY feature flag is up
    • stop using nested fields (what a wild choice that was)
      • to support indexing values at arbitrary paths, use dynamic templates
      • to support value-search with cardSearchFilter/cardSearchText params, index additional docs for each iri value in an indexcard (each with a full copy of the card's filterable fields -- this is the denormalization part)
    • include integer values (for sorting only, so far)
  • allow sorting by property-path (not only by a single property)

and assorted improvements from meanderings in #824

  • move the definition of available index-strategies from settings to static code (to fix having code that depends on settings being just so)
  • replace static methods on IndexStrategy with functions in share.search.index_strategy
  • add admin view to view mappings for an index
  • update abstract Elastic8IndexStrategy to support indexing multiple docs per index-card (required for denormalization above)
  • make tests reusable for index-strategies supporting trove-search
  • consolidate some logic shared across trove-y index strategies to reusable utils in share.search.index_strategy._trovesearch_util
  • more accurate type annotations

@coveralls
Copy link

coveralls commented Oct 25, 2024

Coverage Status

coverage: 91.14% (+0.5%) from 90.646%
when pulling 46ccc83 on aaxelb:feature/6189-denormal
into ec39058 on CenterForOpenScience:develop.

@aaxelb aaxelb marked this pull request as ready for review October 25, 2024 21:08
@aaxelb aaxelb changed the title [wip][ENG-6189] trovesearch denormalized [ENG-6189] trovesearch denormalized Oct 25, 2024
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 pretty good. A couple questions and some potential tidying, but no obvious bugs. Good test coverage.

Pass complete :octocat:

project/settings.py Show resolved Hide resolved
trove/vocab/trove.py Show resolved Hide resolved
index=self.indexname,
)
except elasticsearch8.TransportError as error:
raise exceptions.IndexStrategyError() from error # TODO: error messaging
Copy link
Member

Choose a reason for hiding this comment

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

Minor: how annoying would it be to add error messaging instead of the TODOs here and above?

share/search/index_strategy/trovesearch_denorm.py Outdated Show resolved Hide resolved
share/search/index_strategy/trovesearch_denorm.py Outdated Show resolved Hide resolved
@aaxelb aaxelb merged commit d6d5774 into CenterForOpenScience:develop Nov 12, 2024
3 checks passed
@aaxelb aaxelb deleted the feature/6189-denormal branch November 15, 2024 15:25
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