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

Presearch Code Refactor #2112

Merged
merged 6 commits into from
Dec 17, 2024
Merged

Presearch Code Refactor #2112

merged 6 commits into from
Dec 17, 2024

Conversation

CascadingRadium
Copy link
Member

@CascadingRadium CascadingRadium commented Dec 6, 2024

  • Refactor the presearch code path to make it more generic and extensible.
  • Add an ExtractFields API for obtaining the set of fields applicable to a generic query.
  • Add support for a new API to set the index mapping for an alias.
    This is useful when an alias contains partitions of the same index, as the index
    mapping would be consistent across all indexes and can be inferred directly from
    the alias.

- Refactor the presearch code path to make
   it more generic and extensible.
@Thejas-bhat
Copy link
Member

my bad, rebased (to the latest master) but pushed on the wrong branch :p

@Thejas-bhat
Copy link
Member

Thejas-bhat commented Dec 6, 2024

@CascadingRadium, so I looked into the bm25 code and realized that we are going to aggregate a field specific stat as well.

in your synonyms code, i see you've written a query tree parser

// ExtractSynonyms extracts synonyms from the query tree and returns a map of
// field-term pairs to their synonyms. The input query tree is traversed and
// for each term query, the synonyms are extracted from the synonym source
// associated with the field. The synonyms are then added to the provided map.
// The map is returned and may be nil if no synonyms were found.

can you refactor that to two sets of APIs one generic one which returns all the fields in the query and synonyms can use that API in ExtractSynonyms and bm25 can also use this API to fetch those field specific stat

and that API you can push it to this refactor branch

index_alias_impl.go Outdated Show resolved Hide resolved
@abhinavdangeti
Copy link
Member

Put a do not merge label here as this is already included within #2090 .

@abhinavdangeti
Copy link
Member

-or- if you think we merge this first and rebase #2090 that's fine too.

@CascadingRadium
Copy link
Member Author

i think we can merge this and rebase the #2090 on top

@abhinavdangeti abhinavdangeti added this to the v2.5.0 milestone Dec 17, 2024
index_alias_impl.go Show resolved Hide resolved
search.go Show resolved Hide resolved
Copy link
Member

@abhinavdangeti abhinavdangeti left a comment

Choose a reason for hiding this comment

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

Wait for one more review perhaps. Also @CascadingRadium hope you've made sure all the knn tests still succeed with this change.

@CascadingRadium CascadingRadium merged commit ff6ba91 into master Dec 17, 2024
9 checks passed
@CascadingRadium CascadingRadium deleted the presearchRefactor branch December 17, 2024 08:52
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.

4 participants