Linear retriever top level option for normalizer #129693
Conversation
elasticsearchmachine
added
needs:triage
mridula-s109
changed the title
mridula-s109
force-pushed
the
SEARCH-1027-linear-retriever-top-level-option-for-normalizer
branch
from
6750bc8 to
4df0e99
Compare
mridula-s109
added
>enhancement
:Search/Search
elasticsearchmachine
removed
the
needs:triage
|
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.
Hi @mridula-s109 thanks for this work. I think we need to keep backwards compatibility for when the top level normalizer was not added. Let's generate some examples of how this should work. Yaml tests - which I see we haven't added any - are a good way to go through this exercise. Thanks!
docs/changelog/129693.yaml
Outdated
| @@ -0,0 +1,5 @@ | |||
| pr: 129693 | |||
| summary: Linear retriever top level option for normalizer | |||
There was a problem hiding this comment.
Nitpick:
| summary: Linear retriever top level option for normalizer | |
| summary: Add top level normalizer for linear retriever |
| @@ -290,6 +296,12 @@ Each entry specifies the following parameters: | |||
|
|
|||
| * `l2_norm` : An `L2ScoreNormalizer` that normalizes scores using the L2 norm of the score values. | |||
|
|
|||
| ::::{note} | |||
| Since 9.0 the `normalizer` field must be provided at the top level of the | |||
There was a problem hiding this comment.
This describes a breaking change (and also I don't think the version number is correct here)? We need to discuss this, we need to have backward compatibility.
| ScoreNormalizer normalizer = args[3] == null ? null : ScoreNormalizer.valueOf((String) args[3]); | ||
| String normalizerStr = (String) args[3]; | ||
| boolean explicitNormalizer = normalizerStr != null; | ||
| ScoreNormalizer normalizer = normalizerStr == null ? IdentityScoreNormalizer.INSTANCE : ScoreNormalizer.valueOf(normalizerStr); |
There was a problem hiding this comment.
Why did we change the logic of how to compute default normalizers here?
| } | ||
|
|
||
| @Override | ||
| public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { | ||
| builder.field(RETRIEVER_FIELD.getPreferredName(), retriever); | ||
| builder.field(WEIGHT_FIELD.getPreferredName(), weight); | ||
| builder.field(NORMALIZER_FIELD.getPreferredName(), normalizer.getName()); | ||
| if (normalizer != null) { |
There was a problem hiding this comment.
Why did we add this null check?
| NORMALIZER_FIELD, | ||
| ObjectParser.ValueType.STRING | ||
| ); | ||
| PARSER.declareString(optionalConstructorArg(), NORMALIZER_FIELD); |
There was a problem hiding this comment.
Why did we change the serialization here?
There was a problem hiding this comment.
First off, introducing a breaking change with this is not ok. I think there's a simpler way to do this with less invasive changes and no breaking changes. I see some complications with duplicate serialization of normalizer in the sub-retrievers, but we can deal with that secondarily.
...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
mridula-s109
force-pushed
the
SEARCH-1027-linear-retriever-top-level-option-for-normalizer
branch
from
07965d5 to
d4b1ced
Compare
Summary
This PR extends the rank-rrf plugin’s Linear Retriever with:
normalizerparameter in the simplified syntax.Key Changes
Parsing & Builder
boolean explicitNormalizerto distinguish default vs. user-supplied normalizers.identity) and records explicitness.queryis provided and none supplied.retrieversare present.