Skip to content

Commit 8682c18

Browse files
authored
[ESQL] Fix TopN grouping aggregates bug with non-qualifying intermediate states (elastic#129633)
Continuation of elastic#127148 When datanodes send the STATS intermediate states to the coordinator, it aggregates them. Now, however, the TopN groups sent by a datanode may not be acceptable in the coordinator (Because it has better values already), so it will discard such values. However, the engine wasn't handling intermediate groups with nulls (TopNBlockHash uses nulls to discard unused groups). See https://github.com/elastic/elasticsearch/blob/main/x-pack/plugin/esql/compute/src/main/java/org/elasticsearch/compute/aggregation/GroupingAggregator.java#L47 _This code isn't connected with the query yet, so there's no bug in production_
1 parent 9f22533 commit 8682c18

File tree

87 files changed

+5229
-411
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

87 files changed

+5229
-411
lines changed

x-pack/plugin/esql/compute/gen/src/main/java/org/elasticsearch/compute/gen/GroupingAggregatorImplementer.java

Lines changed: 19 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -204,9 +204,9 @@ private TypeSpec type() {
204204
for (ClassName groupIdClass : GROUP_IDS_CLASSES) {
205205
builder.addMethod(addRawInputLoop(groupIdClass, blockType(aggParam.type())));
206206
builder.addMethod(addRawInputLoop(groupIdClass, vectorType(aggParam.type())));
207+
builder.addMethod(addIntermediateInput(groupIdClass));
207208
}
208209
builder.addMethod(selectedMayContainUnseenGroups());
209-
builder.addMethod(addIntermediateInput());
210210
builder.addMethod(evaluateIntermediate());
211211
builder.addMethod(evaluateFinal());
212212
builder.addMethod(toStringMethod());
@@ -583,11 +583,12 @@ private MethodSpec selectedMayContainUnseenGroups() {
583583
return builder.build();
584584
}
585585

586-
private MethodSpec addIntermediateInput() {
586+
private MethodSpec addIntermediateInput(TypeName groupsType) {
587+
boolean groupsIsBlock = groupsType.toString().endsWith("Block");
587588
MethodSpec.Builder builder = MethodSpec.methodBuilder("addIntermediateInput");
588589
builder.addAnnotation(Override.class).addModifiers(Modifier.PUBLIC);
589590
builder.addParameter(TypeName.INT, "positionOffset");
590-
builder.addParameter(INT_VECTOR, "groups");
591+
builder.addParameter(groupsType, "groups");
591592
builder.addParameter(PAGE, "page");
592593

593594
builder.addStatement("state.enableGroupIdTracking(new $T.Empty())", SEEN_GROUP_IDS);
@@ -613,7 +614,18 @@ private MethodSpec addIntermediateInput() {
613614
}
614615
builder.beginControlFlow("for (int groupPosition = 0; groupPosition < groups.getPositionCount(); groupPosition++)");
615616
{
616-
builder.addStatement("int groupId = groups.getInt(groupPosition)");
617+
if (groupsIsBlock) {
618+
builder.beginControlFlow("if (groups.isNull(groupPosition))");
619+
builder.addStatement("continue");
620+
builder.endControlFlow();
621+
builder.addStatement("int groupStart = groups.getFirstValueIndex(groupPosition)");
622+
builder.addStatement("int groupEnd = groupStart + groups.getValueCount(groupPosition)");
623+
builder.beginControlFlow("for (int g = groupStart; g < groupEnd; g++)");
624+
builder.addStatement("int groupId = groups.getInt(g)");
625+
} else {
626+
builder.addStatement("int groupId = groups.getInt(groupPosition)");
627+
}
628+
617629
if (aggState.declaredType().isPrimitive()) {
618630
if (warnExceptions.isEmpty()) {
619631
assert intermediateState.size() == 2;
@@ -664,6 +676,9 @@ private MethodSpec addIntermediateInput() {
664676
declarationType
665677
);
666678
}
679+
if (groupsIsBlock) {
680+
builder.endControlFlow();
681+
}
667682
builder.endControlFlow();
668683
}
669684
return builder.build();

x-pack/plugin/esql/compute/src/main/generated/org/elasticsearch/compute/aggregation/CountDistinctBooleanGroupingAggregatorFunction.java

Lines changed: 61 additions & 5 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

x-pack/plugin/esql/compute/src/main/generated/org/elasticsearch/compute/aggregation/CountDistinctBytesRefGroupingAggregatorFunction.java

Lines changed: 51 additions & 5 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

x-pack/plugin/esql/compute/src/main/generated/org/elasticsearch/compute/aggregation/CountDistinctDoubleGroupingAggregatorFunction.java

Lines changed: 51 additions & 5 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)