Fix: nested neural_sparse search throws error if model_id not supplied and default_model_id is configured #872
Conversation
jdnvn
requested review from
heemin32,
navneet1v,
VijayanB,
vamshin,
jmazanec15,
naveentatikonda,
junqiu-lei,
martin-gaievski,
sean-zheng-amazon,
model-collapse,
zane-neo,
ylwu-amzn,
jngz-es,
vibrantvarun and
zhichao-aws
as code owners
|
Not sure if this solution is correct, but figured I'd give it a shot. Let me know and I can add some tests. I also wasn't sure how to test this in my local OpenSearch container, so any guidance would be much appreciated. |
@jdnvn thanks for raising the PR. Adding few maintainers who can help in neural sparse. @zane-neo , @yuye-aws, @model-collapse |
| @@ -376,16 +376,24 @@ protected Query doToQuery(QueryShardContext context) throws IOException { | |||
| } | |||
|
|
|||
| private static void validateForRewrite(String queryText, String modelId) { | |||
| if (StringUtils.isBlank(queryText) || StringUtils.isBlank(modelId)) { | |||
| if (StringUtils.isBlank(modelId) && !isClusterOnOrAfterMinReqVersionForDefaultModelIdSupport()) { | |||
There was a problem hiding this comment.
isClusterOnOrAfterMinReqVersionForDefaultModelIdSupport only checks whether the default model id feature is supported in the current cluster by OpenSearch version. Have you checked that default model id has been configured?
|
Please don't fix this issue via this approach, it won't fix the problem, the ml-commons will still throw exception because the passed in model id is null. The root cause is the compound query visitor logics in OpenSearch core, example: opensearch-project/OpenSearch#14739. And for your case here I think it's the missing of sub-visitor in function_score query. Let's discuss the problem in #871 |
You can follow this guide: https://github.com/opensearch-project/OpenSearch/blob/main/TESTING.md |
|
@zhichao-aws and @conggguan has more context on this PR. Can you help review? |
The bug is not fixed from neural-search. It's fixed by modifying the compound query visitor in core |
I see. We need a PR in core to fix the visitor. |
|
Thanks guys, I opened up this PR in OpenSearch core opensearch-project/OpenSearch#15404 |
Description
Adds a conditional for default model id support when validating model id presence in NeuralSparseQueryBuilder. This will prevent the
query_text and model_id cannot be nullerror. Also splits up the errors to make it more clear which is null.Related Issues
Resolves #871
Check List
--signoff.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.