- Notifications
You must be signed in to change notification settings - Fork 25.7k
Linear retriever top level option for normalizer #129693
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
Linear retriever top level option for normalizer #129693
Conversation
6750bc8 to 4df0e99 Compare | Pinging @elastic/es-search (Team:Search) |
| Hi @mridula-s109, I've created a changelog YAML for you. |
| 🔍 Preview links for changed docs: 🔔 The preview site may take up to 3 minutes to finish building. These links will become live once it completes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Partial review, covered everything except the documentation. Looks good overall! I left a few minor comments.
...lugin/rank-rrf/src/main/java/org/elasticsearch/xpack/rank/linear/LinearRetrieverBuilder.java Outdated Show resolved Hide resolved
...lugin/rank-rrf/src/main/java/org/elasticsearch/xpack/rank/linear/LinearRetrieverBuilder.java Outdated Show resolved Hide resolved
...lugin/rank-rrf/src/main/java/org/elasticsearch/xpack/rank/linear/LinearRetrieverBuilder.java Outdated Show resolved Hide resolved
...-rrf/src/internalClusterTest/java/org/elasticsearch/xpack/rank/linear/LinearRetrieverIT.java Outdated Show resolved Hide resolved
...-rrf/src/internalClusterTest/java/org/elasticsearch/xpack/rank/linear/LinearRetrieverIT.java Outdated Show resolved Hide resolved
...rf/src/test/java/org/elasticsearch/xpack/rank/linear/LinearRetrieverBuilderParsingTests.java Outdated Show resolved Hide resolved
.../rank-rrf/src/test/java/org/elasticsearch/xpack/rank/linear/LinearRetrieverBuilderTests.java Outdated Show resolved Hide resolved
.../rank-rrf/src/test/java/org/elasticsearch/xpack/rank/linear/LinearRetrieverBuilderTests.java Outdated Show resolved Hide resolved
...plugin/rank-rrf/src/yamlRestTest/resources/rest-api-spec/test/linear/10_linear_retriever.yml Show resolved Hide resolved
kderusso left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work @mridula-s109 - I left a few minor comments, but this is very close!
docs/reference/elasticsearch/rest-apis/retrievers/linear-retriever.md Outdated Show resolved Hide resolved
docs/reference/elasticsearch/rest-apis/retrievers/linear-retriever.md Outdated Show resolved Hide resolved
x-pack/plugin/rank-rrf/src/main/java/org/elasticsearch/xpack/rank/RankRRFFeatures.java Outdated Show resolved Hide resolved
...plugin/rank-rrf/src/yamlRestTest/resources/rest-api-spec/test/linear/10_linear_retriever.yml Outdated Show resolved Hide resolved
...plugin/rank-rrf/src/yamlRestTest/resources/rest-api-spec/test/linear/10_linear_retriever.yml Outdated Show resolved Hide resolved
...plugin/rank-rrf/src/yamlRestTest/resources/rest-api-spec/test/linear/10_linear_retriever.yml Outdated Show resolved Hide resolved
Thanks for your comments @kderusso , have resolved all the cluster features comments and updated the documentation. |
kderusso left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making the changes @mridula-s109 - I've left some nitpicky comments, and you may need to update/there are some failing CI tests that look unrelated. Otherwise LGTM. Nice work, thanks for iterating!
...plugin/rank-rrf/src/yamlRestTest/resources/rest-api-spec/test/linear/10_linear_retriever.yml Outdated Show resolved Hide resolved
...plugin/rank-rrf/src/yamlRestTest/resources/rest-api-spec/test/linear/10_linear_retriever.yml Outdated Show resolved Hide resolved
...plugin/rank-rrf/src/yamlRestTest/resources/rest-api-spec/test/linear/10_linear_retriever.yml Outdated Show resolved Hide resolved
...plugin/rank-rrf/src/yamlRestTest/resources/rest-api-spec/test/linear/10_linear_retriever.yml Outdated Show resolved Hide resolved
| @Mikep86 Can you please have a look at it as you had requested changes as well? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job cleaning this up! I left a couple non-nit comments about the some missing applies_to tags in the docs. The rest is non-blocking.
...rf/src/test/java/org/elasticsearch/xpack/rank/linear/LinearRetrieverBuilderParsingTests.java Outdated Show resolved Hide resolved
...plugin/rank-rrf/src/yamlRestTest/resources/rest-api-spec/test/linear/10_linear_retriever.yml Outdated Show resolved Hide resolved
...plugin/rank-rrf/src/yamlRestTest/resources/rest-api-spec/test/linear/10_linear_retriever.yml Show resolved Hide resolved
Thanks for the comments have resolved them all. |
Summary
Adds a top-level
normalizeroption to thelinearretriever.The top-level normalizer acts as the default for all sub-retrievers, but each sub-retriever can override it if needed.
Behavior
Normalizer resolution priority:
Example