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

[BUG] NPE when queryResults.isEmpty() but hasAggs is true #5785

Closed
janheise opened this issue Jan 10, 2023 · 13 comments
Closed

[BUG] NPE when queryResults.isEmpty() but hasAggs is true #5785

janheise opened this issue Jan 10, 2023 · 13 comments
Labels
bug Something isn't working distributed framework good first issue Good for newcomers v2.9.0 'Issues and PRs related to version v2.9.0'

Comments

@janheise
Copy link

janheise commented Jan 10, 2023

Describe the bug
We have the following occuring;

If there is an empty result:

if (queryResults.isEmpty()) { // early terminate we have nothing to reduce
final TotalHits totalHits = topDocsStats.getTotalHits();
return new ReducedQueryPhase(

returns with an almost empty ReducedQueryPhase instance, which further on in

if (hasAggs) {
// Update the circuit breaker to replace the estimation with the serialized size of the newly reduced result
long finalSize = reducePhase.aggregations.getSerializedSize() - breakerSize;

creates an NPE if the query had aggregations.

It is probably related to the multi_terms aggregation.

To Reproduce
Steps to reproduce the behavior:

  • Start a new/empty OpenSearch 2.4 instance
  • create an index PUT http://localhost:9200/test_jh
  • run the following query:
{
	"aggregations": {
		"agg": {
			"date_histogram": {
				"field": "timestamp",
				"format": "date_time",
				"fixed_interval": "30m"
			},
			"aggregations": {
				"agg": {
					"multi_terms": {
						"terms": [
							{
								"field": "field1"
							},
							{
								"field": "field2"
							}
						]
					}
				}
			}
		}
	}
}

The following error occurs:

{
	"error": {
		"root_cause": [
			{
				"type": "response_handler_failure_transport_exception",
				"reason": "java.lang.NullPointerException: Cannot invoke \"org.opensearch.common.io.stream.NamedWriteable.getWriteableName()\" because \"namedWriteable\" is null"
			}
		],
		"type": "search_phase_execution_exception",
		"reason": "",
		"phase": "fetch",
		"grouped": true,
		"failed_shards": [
			{
				"shard": 0,
				"index": "test_jh",
				"node": "NbG0fKofQ5OOkIagnOIXvA",
				"reason": {
					"type": "response_handler_failure_transport_exception",
					"reason": "java.lang.NullPointerException: Cannot invoke \"org.opensearch.common.io.stream.NamedWriteable.getWriteableName()\" because \"namedWriteable\" is null",
					"caused_by": {
						"type": "null_pointer_exception",
						"reason": "Cannot invoke \"org.opensearch.common.io.stream.NamedWriteable.getWriteableName()\" because \"namedWriteable\" is null"
					}
				}
			}
		],
		"caused_by": {
			"type": "null_pointer_exception",
			"reason": "Cannot invoke \"org.opensearch.search.aggregations.InternalAggregations.getSerializedSize()\" because \"reducePhase.aggregations\" is null"
		}
	},
	"status": 500
}

Expected behavior
There should be no NPE, the empty result should be propagated as is.

Host/Environment (please complete the following information):

  • OS: macOS, Ubuntu 20.04
  • Version 2.4
@janheise janheise added bug Something isn't working untriaged labels Jan 10, 2023
@janheise janheise changed the title [BUG] NPE when queryResults.isEmpty() but hasAggs is true (maybe only with "multi_terms") [BUG] NPE when queryResults.isEmpty() but hasAggs is true Jan 11, 2023
@janheise
Copy link
Author

Added example to reproduce problem.

@dblock
Copy link
Member

dblock commented Jan 17, 2023

@janheise You did most of the work to debug this! Thank you. Want to try to implement a unit test and maybe even a fix?

@anasalkouz anasalkouz added good first issue Good for newcomers and removed untriaged labels Jan 17, 2023
@janheise
Copy link
Author

@dblock I am currently too busy to tackle it myself - and am surprised about the good first issue label. From what I've seen (but I did not look into it too deeply), you need a lot of context to decide where the problem actually starts and how a bugfix should look.

@dblock
Copy link
Member

dblock commented Jan 20, 2023

@janheise Thanks! @anasalkouz triaged it as such, I imagine because there's a request/response repro (thank you). I think a unit test is not too difficult based on this, but I agree that fixing the problem may be much more involved. Would value a PR that just has the unit test, it will get whomever wants to fix this one step further.

@sarvajeetp
Copy link

/assign

@sarvajeetp
Copy link

sarvajeetp commented Jan 29, 2023

When I tried to debug, it turned out that queryResults has size of 1 and hence it doesn't go into below condition

if (queryResults.isEmpty()) { // early terminate we have nothing to reduce
final TotalHits totalHits = topDocsStats.getTotalHits();
return new ReducedQueryPhase(

Instead below aggregation variable evaluates to null, because bufferedAggs list is empty.

final InternalAggregations aggregations = reduceAggs(aggReduceContextBuilder, performFinalReduce, bufferedAggs);

@anasalkouz
Copy link
Member

anasalkouz commented Jan 31, 2023

am surprised about the good first issue label. From what I've seen (but I did not look into it too deeply), you need a lot of context to decide where the problem actually starts and how a bugfix should look.

Agreed, will keep the label for the unit test

@sarvajeetp sarvajeetp removed their assignment Feb 4, 2023
@geetptl
Copy link

geetptl commented Feb 6, 2023

Hey @anasalkouz @janheise @dblock, I am going to pick this up since the previous assignee has left. Will reply here if I need any help

@anasalkouz
Copy link
Member

Hi @geetptl, it has been a while, are you actively working on this?

@dblock
Copy link
Member

dblock commented Apr 13, 2023

There may be a repro in #7089. Help us turn this into a failing unit test?

@z002Holpp
Copy link

Could you give a rough estimate about when the issue could be solved or if someone actively works on it? We have some business decisions depending on that.

@dblock
Copy link
Member

dblock commented Apr 21, 2023

@z002Holpp I would say nobody is working on it unless it clearly says so, and looks like @geetptl isn't around to confirm they still are :( maybe you can help with some hands?

@geetptl
Copy link

geetptl commented Apr 22, 2023

Het @dblock, I'm not working on this, feel free to take it up!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working distributed framework good first issue Good for newcomers v2.9.0 'Issues and PRs related to version v2.9.0'
Projects
None yet
Development

No branches or pull requests

8 participants