elastic / elasticsearch

Free and Open Source, Distributed, RESTful Search Engine
https://www.elastic.co/products/elasticsearch
Other
69.9k stars 24.73k forks source link

FunctionScore query always passes `null` ExceptionHolder to sub script queries #109177

Closed bhmehdi closed 2 months ago

bhmehdi commented 4 months ago

Elasticsearch Version

8.13.4

Installed Plugins

No response

Java Version

bundled

OS Version

Windows 10

Problem Description

Hello, when setting up a score calculation script I notice that the ExplanationHolder object is always null which makes it impossible to add a description on this object. The example iused is this example you can see on lines 144 and 168 ExplanationHolder is always null and you can not add a custom description. explanation.set(myCustomdescription);

Steps to Reproduce

Logs (if relevant)

No response

elasticsearchmachine commented 4 months ago

Pinging @elastic/es-search (Team:Search)

bhmehdi commented 4 months ago

Hello, Do you have any news about this issue ?

benwtrent commented 4 months ago

Maybe I misunderstand what you are trying to do, but in the ScriptScoreQueryBuilder object, when explain is called, we do construct a holder and pass that down to the lower executors.

Our example here might be in complete.

Are you seeing a null ExceptionHolder object when running your script within a query when explain: true ?

bhmehdi commented 4 months ago

Yes ExplanationHolder is null when i call the script whith explain: true on query

elasticsearchmachine commented 3 months ago

Pinging @elastic/es-search-relevance (Team:Search Relevance)

benwtrent commented 3 months ago

@bhmehdi I don't understand how its null unless you are overwriting the query explain and bypassing the value being passed down to the script.

This holder is only ever provided when explain: true, and its being provided when the ScriptScoreQuery objects are being created. Then when the score function is called, it gets passed to explain the scoring.

When you execute your script within ScriptScoreQueryBuilder you are saying the explanation holder is null?

In what context are you executing your script?

Could you provide stack traces and logs or some repeatable steps so we can debug why you are seeing a null value?

bhmehdi commented 3 months ago

@benwtrent To reproduce the problem I added the following line in the example : explanation.set("myDescription");

Here is an example of a log retrieved from the Elasticsearch log files which indicates error when calling ExplanationHolder.set(String) methd because "explanation" is null

org.elasticsearch.action.search.SearchPhaseExecutionException: all shards failed at org.elasticsearch.action.search.AbstractSearchAsyncAction.onPhaseFailure(AbstractSearchAsyncAction.java:712) ~[elasticsearch-8.13.2.jar:?] at org.elasticsearch.action.search.AbstractSearchAsyncAction.executeNextPhase(AbstractSearchAsyncAction.java:404) ~[elasticsearch-8.13.2.jar:?] at org.elasticsearch.action.search.AbstractSearchAsyncAction.onPhaseDone(AbstractSearchAsyncAction.java:744) ~[elasticsearch-8.13.2.jar:?] at org.elasticsearch.action.search.AbstractSearchAsyncAction.onShardFailure(AbstractSearchAsyncAction.java:497) ~[elasticsearch-8.13.2.jar:?] at org.elasticsearch.action.search.AbstractSearchAsyncAction$1.onFailure(AbstractSearchAsyncAction.java:335) ~[elasticsearch-8.13.2.jar:?] at org.elasticsearch.action.ActionListenerImplementations.safeAcceptException(ActionListenerImplementations.java:62) ~[elasticsearch-8.13.2.jar:?] at org.elasticsearch.action.ActionListenerImplementations.safeOnFailure(ActionListenerImplementations.java:73) ~[elasticsearch-8.13.2.jar:?] at org.elasticsearch.action.DelegatingActionListener.onFailure(DelegatingActionListener.java:31) ~[elasticsearch-8.13.2.jar:?] at org.elasticsearch.action.ActionListenerResponseHandler.handleException(ActionListenerResponseHandler.java:53) ~[elasticsearch-8.13.2.jar:?] at org.elasticsearch.action.search.SearchTransportService$ConnectionCountingHandler.handleException(SearchTransportService.java:634) ~[elasticsearch-8.13.2.jar:?] at org.elasticsearch.transport.TransportService$UnregisterChildTransportResponseHandler.handleException(TransportService.java:1751) ~[elasticsearch-8.13.2.jar:?] at org.elasticsearch.transport.TransportService$ContextRestoreResponseHandler.handleException(TransportService.java:1475) ~[elasticsearch-8.13.2.jar:?] at org.elasticsearch.transport.TransportService$DirectResponseChannel.processException(TransportService.java:1609) ~[elasticsearch-8.13.2.jar:?] at org.elasticsearch.transport.TransportService$DirectResponseChannel.sendResponse(TransportService.java:1584) ~[elasticsearch-8.13.2.jar:?] at org.elasticsearch.transport.TaskTransportChannel.sendResponse(TaskTransportChannel.java:44) ~[elasticsearch-8.13.2.jar:?] at org.elasticsearch.action.support.ChannelActionListener.onFailure(ChannelActionListener.java:44) ~[elasticsearch-8.13.2.jar:?] at org.elasticsearch.action.ActionRunnable.onFailure(ActionRunnable.java:146) ~[elasticsearch-8.13.2.jar:?] at org.elasticsearch.common.util.concurrent.AbstractRunnable.run(AbstractRunnable.java:28) ~[elasticsearch-8.13.2.jar:?] at org.elasticsearch.common.util.concurrent.TimedRunnable.doRun(TimedRunnable.java:33) ~[elasticsearch-8.13.2.jar:?] at org.elasticsearch.common.util.concurrent.ThreadContext$ContextPreservingAbstractRunnable.doRun(ThreadContext.java:984) ~[elasticsearch-8.13.2.jar:?] at org.elasticsearch.common.util.concurrent.AbstractRunnable.run(AbstractRunnable.java:26) ~[elasticsearch-8.13.2.jar:?] at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1144) ~[?:?] at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:642) ~[?:?] at java.lang.Thread.run(Thread.java:1583) ~[?:?] Caused by: org.elasticsearch.ElasticsearchException$1: Cannot invoke "org.elasticsearch.script.ScoreScript$ExplanationHolder.set(String)" because "explanation" is null at org.elasticsearch.ElasticsearchException.guessRootCauses(ElasticsearchException.java:701) ~[elasticsearch-8.13.2.jar:?] at org.elasticsearch.action.search.AbstractSearchAsyncAction.executeNextPhase(AbstractSearchAsyncAction.java:402) ~[elasticsearch-8.13.2.jar:?] ... 22 more

and this is the return of the elasticsearch request { "error": { "root_cause": [ { "type": "null_pointer_exception", "reason": """Cannot invoke "org.elasticsearch.script.ScoreScript$ExplanationHolder.set(String)" because "explanation" is null""" } ], "type": "search_phase_execution_exception", "reason": "all shards failed", "phase": "query", "grouped": true, "failed_shards": [ { "shard": 0, "index": "myindex_name", "node": "11HfTjw_T_uUJxZdMbT4hg", "reason": { "type": "null_pointer_exception", "reason": """Cannot invoke "org.elasticsearch.script.ScoreScript$ExplanationHolder.set(String)" because "explanation" is null""" } } ], "caused_by": { "type": "null_pointer_exception", "reason": """Cannot invoke "org.elasticsearch.script.ScoreScript$ExplanationHolder.set(String)" because "explanation" is null""", "caused_by": { "type": "null_pointer_exception", "reason": """Cannot invoke "org.elasticsearch.script.ScoreScript$ExplanationHolder.set(String)" because "explanation" is null""" } } }, "status": 500 }

benwtrent commented 3 months ago

@bhmehdi could you provide the API call that caused this failure?

jdconrad commented 3 months ago

@bhmehdi If you guard the explanation with a null check does it fix the issue?

if (explanation != null) {
    explanation.set("my explanation");
}

My guess is this guard may be necessary to avoid an NPE due to how we run explain after the actual query.

bhmehdi commented 3 months ago

@benwtrent this is the aoi call `POST /myindex_name/_search?typed_keys=true { "_source": { "includes": ["field1", "field2", "field3"] }, "explain": true, "fields": [{ "field": "field1" }, { "field": "field2" }, { "field": "field3" }

],
"from": 0,
"query": {
    "function_score": {
        "boost_mode": "replace",
        "functions": [{
                "script_score": {
                    "script": {
                        "source": "pure_df",
                        "lang": "expert_scripts",
                        "params": {
                            "field": "field1",
                            "term": "test"
                        }
                    }
                }
            }
        ],
        "query": {
            "constant_score": {
                "filter": {
                    "bool": {
                        "filter": [{
                                "bool": {}
                            }
                        ],
                        "must": [{
                                "bool": {
                                    "must": [{
                                            "bool": {
                                                "minimum_should_match": "1",
                                                "should": [{
                                                        "term": {
                                                            "field1.raw": {
                                                                "value": "test"
                                                            }
                                                        }
                                                    }, {
                                                        "match": {
                                                            "field1": {
                                                                "operator": "and",
                                                                "query": "test"
                                                            }
                                                        }
                                                    }, {
                                                        "match": {
                                                            "field1": {
                                                                "fuzziness": "0",
                                                                "operator": "or",
                                                                "query": "test"
                                                            }
                                                        }
                                                    }
                                                ]
                                            }
                                        }
                                    ]
                                }
                            }
                        ]
                    }
                }
            }
        }
    }
},
"size": 20,
"sort": [{
        "_score": {
            "order": "desc"
        }
    }, {
        "myid": {
            "order": "desc"
        }
    }
],
"timeout": "15000ms"

}`

@jdconrad yes i use

if (explanation != null) { explanation.set("my explanation"); } to avoid NPE, i just removed it to get error log, but because explanaition is always null i can set my custom explanation

https://www.elastic.co/guide/en/elasticsearch/reference/current/modules-scripting-engine.html

jdconrad commented 3 months ago

It looks like ExplanationHolder is specifically supported only for a script score query, but not as part of a function score query.

benwtrent commented 3 months ago

@jdconrad yea, I see it here:

https://github.com/elastic/elasticsearch/blob/12ad399c488f0cc60e19b5e1b29c6d569cb4351a/server/src/main/java/org/elasticsearch/common/lucene/search/function/ScriptScoreFunction.java#L70

I am not sure how complicated the change would be, probably not too bad. I will rewrite the title of this issue to better capture the bug.

john-wagster commented 2 months ago

@bhmehdi I posted a PR that I believe fixes the issue. Take a look at the description on that PR for what a returned hit would look like when setting an explanation and if that's not what you expect let me know and we can adjust accordingly.

bhmehdi commented 2 months ago

@john-wagster I took a look at the PR and it seems consistent and corresponds well to what is expected

john-wagster commented 2 months ago

This should be fixed now in 8.15.1 and later. @bhmehdi let me know if you run into any additional issues and we can re-open this issue or open a new issue as appropriate.

bhmehdi commented 2 months ago

Thanks i will test on release of the version 8.15.1