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

Composite Aggregation after key ignored #785

Open
assafcoh opened this issue Dec 27, 2023 · 7 comments
Open

Composite Aggregation after key ignored #785

assafcoh opened this issue Dec 27, 2023 · 7 comments
Labels
bug Something isn't working

Comments

@assafcoh
Copy link

assafcoh commented Dec 27, 2023

What is the bug?

when searching with composite aggregation the supplied afterKey is ignored and we always get back the same initial afterKey.
Therefore, iterating with composite aggregation until the afterKey is null, leads to an infinite loop

client version

we encountered this bug with client version 2.6.0, 2.70 and also after upgrading to the latest 2.8.1 client version

How can one reproduce the bug?

search with composite aggregation in a loop until the afterKey is null.
Since the afterKey is ignored, it will always search from the beginning and repeatedly return the same afterKey

What is the expected behavior?

the afterKey should should be considered, so that after each search we get back the "next" afterKey

workaround

Unfortunately we switched back to the rest client :(

Code sample to reproduce infinite loop

List<Map<String, CompositeAggregationSource>> compositeAggSourceList = new ArrayList<>();

compositeAggSourceList.add(Collections.singletonMap("tenant_id", new CompositeAggregationSource.Builder()
				.terms(new TermsAggregation.Builder()
						.field("tenant_id")
						.build())
		.build()));
compositeAggSourceList.add(Collections.singletonMap("user_id", new CompositeAggregationSource.Builder()
		.terms(new TermsAggregation.Builder()
				.field("user_id")
				.build())
		.build()));
					
CompositeAggregate compositeAggregate = null;
do {
	// update the composite aggregation after key
	Map<String, String> afterKey;
	if(compositeAggregate !=null){
		afterKey = compositeAggregate.afterKey().entrySet().stream().collect(Collectors.toMap(Map.Entry::getKey,
				e -> new String(e.getValue().toString())));
	} else {
		afterKey = null;
	}

	SearchRequest searchRequest = new SearchRequest.Builder()
			.index("example-index")
			.size(0)
			.aggregations("composite-agg", new Aggregation.Builder()
					.composite(c-> afterKey !=null ? c.sources(compositeAggSourceList).size(10).after(afterKey) : c.sources(compositeAggSourceList).size(10))
					.aggregations("incidents",
							a -> a.sum(t -> t.field("incidents")))
					.aggregations("activities",
							a -> a.sum(t -> t.field("activities")))
					.build())
			.build();

	SearchResponse<Map> searchResponse = openSearchClient.get().search(searchRequest, Map.class);

	compositeAggregate = searchResponse.aggregations().get("composite-agg").composite();
	
	// our internal logic...

} while (compositeAggregate.afterKey()!=null);

@assafcoh assafcoh added bug Something isn't working untriaged labels Dec 27, 2023
@dblock dblock removed the untriaged label Dec 29, 2023
@assafcoh
Copy link
Author

composite aggregation is the only way to paginate aggregations. This is a major bug,
Is there a fix estimate?

@dblock
Copy link
Member

dblock commented Jan 24, 2024

@assafcoh Nobody has picked this up yet, care to help? Maybe start by adding a (failing) test?

@assafcoh
Copy link
Author

Unfortunately, i am not able to contribute here.

However, this bug means there is no way to paginate aggregations with this java client. In my opinion, this is a central feature and should be given higher priority.

@amcgreevy-r7
Copy link

amcgreevy-r7 commented Jul 30, 2024

@dblock Has there been any movement on this? This is a core feature that is really needed.

@dblock
Copy link
Member

dblock commented Jul 30, 2024

@amcgreevy-r7 Would love your help, see above.

@assafcoh
Copy link
Author

hi guys, I am really surprised this bug is still lurking in the code.
Composite aggregation is the only way to paginate aggregations. This is a major bug,
Is there a fix estimate?

@Xtansia
Copy link
Collaborator

Xtansia commented Nov 20, 2024

So, there's a couple things going on here:
One actual issue is that CompositeAggregation.after() should be changed to accept a Map<String, JsonData> to match CompositeAggregate.afterKey().

Separately a quirk of the client is that List and Map types are never actually null, there's just a sentinel undefined map/list type. As such you should not be checking for agg.afterKey() != null but instead using ApiTypeHelepr.isDefined(agg.afterKey()) or !agg.afterKey().isEmpty().

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants