Skip to content

Commit 85c42bc

Browse files
authored
Fix dimension values aggregator (elastic#137410)
This change fixes a mistake in the dimension values aggregator. The bug prevented the shortcut in production (not correctness issue) and caused incorrect results in tests when using a block hash that reserves 0 for nulls. This change corrects the issue and randomly uses two types of block hash in tests: one that reserves group 0 for nulls and one that does not. Closes elastic#137378
1 parent 09f5349 commit 85c42bc

File tree

3 files changed

+13
-12
lines changed

3 files changed

+13
-12
lines changed

muted-tests.yml

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -501,9 +501,6 @@ tests:
501501
- class: org.elasticsearch.readiness.ReadinessClusterIT
502502
method: testReadinessDuringRestartsNormalOrder
503503
issue: https://github.com/elastic/elasticsearch/issues/136955
504-
- class: org.elasticsearch.xpack.esql.expression.function.aggregate.DimensionValuesByteRefGroupingAggregatorFunctionTests
505-
method: testSimple
506-
issue: https://github.com/elastic/elasticsearch/issues/137378
507504
- class: org.elasticsearch.xpack.ilm.TimeSeriesDataStreamsIT
508505
method: testSearchableSnapshotAction
509506
issue: https://github.com/elastic/elasticsearch/issues/137167

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

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -237,16 +237,17 @@ public void addIntermediateInput(int positionOffset, IntVector groups, Page page
237237
@Override
238238
public void evaluateIntermediate(Block[] blocks, int offset, IntVector selected) {
239239
int positionCount = selected.getPositionCount();
240-
boolean allSelected = positionCount == maxGroupId + 1;
240+
boolean allSelected = positionCount > maxGroupId;
241241
if (allSelected) {
242242
for (int i = 0; i < selected.getPositionCount(); i++) {
243-
if (selected.getInt(i) == i) {
243+
if (selected.getInt(i) != i) {
244244
allSelected = false;
245245
break;
246246
}
247247
}
248248
}
249249
if (allSelected) {
250+
fillNullsUpTo(positionCount);
250251
blocks[offset] = builder.build();
251252
return;
252253
}

x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/aggregate/DimensionValuesByteRefGroupingAggregatorFunctionTests.java

Lines changed: 10 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -77,14 +77,17 @@ public void testSimple() {
7777
randomFrom(AggregatorMode.INITIAL, AggregatorMode.SINGLE),
7878
List.of(0)
7979
);
80+
final List<BlockHash.GroupSpec> groupSpecs;
81+
if (randomBoolean()) {
82+
// Use IntBlockHash; reserves 0 for null values
83+
groupSpecs = List.of(new BlockHash.GroupSpec(1, ElementType.INT));
84+
} else {
85+
// Use PackedValuesBlockHash; does not reserve 0 for null values
86+
groupSpecs = List.of(new BlockHash.GroupSpec(1, ElementType.INT), new BlockHash.GroupSpec(1, ElementType.INT));
87+
}
8088
HashAggregationOperator hashAggregationOperator = new HashAggregationOperator(
8189
List.of(aggregatorFactory),
82-
() -> BlockHash.build(
83-
List.of(new BlockHash.GroupSpec(1, ElementType.INT)),
84-
driverContext.blockFactory(),
85-
randomIntBetween(1, 1024),
86-
randomBoolean()
87-
),
90+
() -> BlockHash.build(groupSpecs, driverContext.blockFactory(), randomIntBetween(1, 1024), randomBoolean()),
8891
driverContext
8992
);
9093
List<Page> outputPages = new ArrayList<>();
@@ -99,7 +102,7 @@ public void testSimple() {
99102
Map<Integer, List<BytesRef>> actualValues = new HashMap<>();
100103
for (Page out : outputPages) {
101104
IntBlock groups = out.getBlock(0);
102-
BytesRefBlock valuesBlock = out.getBlock(1);
105+
BytesRefBlock valuesBlock = out.getBlock(groupSpecs.size());
103106
for (int p = 0; p < out.getPositionCount(); p++) {
104107
int group = groups.getInt(p);
105108
int valueCount = valuesBlock.getValueCount(p);

0 commit comments

Comments
 (0)