Skip to content

Commit 3f205f2

Browse files
iveraseiverase
authored
Fix IOOBE in TTest aggregation when using filters (#109034)
There is a corner case in the TTEst aggregation where we are throwing an IOOBE becuase we are not checking properly the bounds of the arrays. It happens when using filters on the aggregation and the aggregation is a sub-aggregation. Then one of the TTestStats can be empty which my mean that the array might have not been grown. When reading it, we don't check the current range. This commit makes sure we only read the state if it exist in the array.
1 parent 01fdeca commit 3f205f2

File tree

5 files changed

+83
-3
lines changed

5 files changed

+83
-3
lines changed

‎docs/changelog/109034.yaml

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
pr: 109034
2+
summary: Fix IOOBE in TTest aggregation when using filters
3+
area: Aggregations
4+
type: bug
5+
issues: []

‎x-pack/plugin/analytics/src/main/java/org/elasticsearch/xpack/analytics/ttest/PairedTTestAggregator.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@ protected PairedTTestState getState(long bucket) {
4848

4949
@Override
5050
protected PairedTTestState getEmptyState() {
51-
return new PairedTTestState(new TTestStats(0, 0, 0), tails);
51+
return new PairedTTestState(TTestStats.EMPTY, tails);
5252
}
5353

5454
@Override

‎x-pack/plugin/analytics/src/main/java/org/elasticsearch/xpack/analytics/ttest/TTestStats.java

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,8 @@
2020
* Collects basic stats that are needed to perform t-test
2121
*/
2222
public class TTestStats implements Writeable {
23+
static TTestStats EMPTY = new TTestStats(0, 0, 0);
24+
2325
public final long count;
2426
public final double sum;
2527
public final double sumOfSqrs;

‎x-pack/plugin/analytics/src/main/java/org/elasticsearch/xpack/analytics/ttest/UnpairedTTestAggregator.java

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -56,12 +56,14 @@ public class UnpairedTTestAggregator extends TTestAggregator<UnpairedTTestState>
5656

5757
@Override
5858
protected UnpairedTTestState getState(long bucket) {
59-
return new UnpairedTTestState(a.get(bucket), b.get(bucket), homoscedastic, tails);
59+
final TTestStats aTTestStats = a.getSize() > bucket ? a.get(bucket) : TTestStats.EMPTY;
60+
final TTestStats bTTestStats = b.getSize() > bucket ? b.get(bucket) : TTestStats.EMPTY;
61+
return new UnpairedTTestState(aTTestStats, bTTestStats, homoscedastic, tails);
6062
}
6163

6264
@Override
6365
protected UnpairedTTestState getEmptyState() {
64-
return new UnpairedTTestState(new TTestStats(0, 0, 0), new TTestStats(0, 0, 0), homoscedastic, tails);
66+
return new UnpairedTTestState(TTestStats.EMPTY, TTestStats.EMPTY, homoscedastic, tails);
6567
}
6668

6769
@Override

‎x-pack/plugin/analytics/src/test/java/org/elasticsearch/xpack/analytics/ttest/TTestAggregatorTests.java

Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -636,6 +636,77 @@ public void testFiltered() throws IOException {
636636
}
637637
}
638638

639+
public void testFilteredAsSubAgg() throws IOException {
640+
TTestType tTestType = randomFrom(TTestType.values());
641+
MappedFieldType fieldType1 = new NumberFieldMapper.NumberFieldType("h", NumberFieldMapper.NumberType.INTEGER);
642+
MappedFieldType fieldType2 = new NumberFieldMapper.NumberFieldType("a", NumberFieldMapper.NumberType.INTEGER);
643+
MappedFieldType fieldType3 = new NumberFieldMapper.NumberFieldType("b", NumberFieldMapper.NumberType.INTEGER);
644+
TTestAggregationBuilder ttestAggregationBuilder = new TTestAggregationBuilder("t_test").a(
645+
new MultiValuesSourceFieldConfig.Builder().setFieldName("a").setFilter(QueryBuilders.termQuery("b", 1)).build()
646+
)
647+
.b(new MultiValuesSourceFieldConfig.Builder().setFieldName("a").setFilter(QueryBuilders.termQuery("b", 2)).build())
648+
.testType(tTestType);
649+
int tails = randomIntBetween(1, 2);
650+
if (tails == 1 || randomBoolean()) {
651+
ttestAggregationBuilder.tails(tails);
652+
}
653+
HistogramAggregationBuilder aggregationBuilder = new HistogramAggregationBuilder("h").field("h")
654+
.interval(1)
655+
.subAggregation(ttestAggregationBuilder);
656+
int buckets = randomInt(100);
657+
CheckedConsumer<RandomIndexWriter, IOException> buildIndex = iw -> {
658+
for (int i = 0; i < buckets; i++) {
659+
iw.addDocument(asList(new NumericDocValuesField("h", i), new NumericDocValuesField("a", 102), new IntPoint("b", 1)));
660+
iw.addDocument(asList(new NumericDocValuesField("h", i), new NumericDocValuesField("a", 99), new IntPoint("b", 1)));
661+
iw.addDocument(asList(new NumericDocValuesField("h", i), new NumericDocValuesField("a", 111), new IntPoint("b", 1)));
662+
iw.addDocument(asList(new NumericDocValuesField("h", i), new NumericDocValuesField("a", 97), new IntPoint("b", 1)));
663+
iw.addDocument(asList(new NumericDocValuesField("h", i), new NumericDocValuesField("a", 101), new IntPoint("b", 1)));
664+
iw.addDocument(asList(new NumericDocValuesField("h", i), new NumericDocValuesField("a", 99), new IntPoint("b", 1)));
665+
666+
iw.addDocument(asList(new NumericDocValuesField("h", i), new NumericDocValuesField("a", 89), new IntPoint("b", 2)));
667+
iw.addDocument(asList(new NumericDocValuesField("h", i), new NumericDocValuesField("a", 93), new IntPoint("b", 2)));
668+
iw.addDocument(asList(new NumericDocValuesField("h", i), new NumericDocValuesField("a", 72), new IntPoint("b", 2)));
669+
iw.addDocument(asList(new NumericDocValuesField("h", i), new NumericDocValuesField("a", 98), new IntPoint("b", 2)));
670+
iw.addDocument(asList(new NumericDocValuesField("h", i), new NumericDocValuesField("a", 102), new IntPoint("b", 2)));
671+
iw.addDocument(asList(new NumericDocValuesField("h", i), new NumericDocValuesField("a", 98), new IntPoint("b", 2)));
672+
673+
iw.addDocument(asList(new NumericDocValuesField("h", i), new NumericDocValuesField("a", 189), new IntPoint("b", 3)));
674+
iw.addDocument(asList(new NumericDocValuesField("h", i), new NumericDocValuesField("a", 193), new IntPoint("b", 3)));
675+
iw.addDocument(asList(new NumericDocValuesField("h", i), new NumericDocValuesField("a", 172), new IntPoint("b", 3)));
676+
iw.addDocument(asList(new NumericDocValuesField("h", i), new NumericDocValuesField("a", 198), new IntPoint("b", 3)));
677+
iw.addDocument(asList(new NumericDocValuesField("h", i), new NumericDocValuesField("a", 1102), new IntPoint("b", 3)));
678+
iw.addDocument(asList(new NumericDocValuesField("h", i), new NumericDocValuesField("a", 198), new IntPoint("b", 3)));
679+
}
680+
};
681+
if (tTestType == TTestType.PAIRED) {
682+
IllegalArgumentException ex = expectThrows(
683+
IllegalArgumentException.class,
684+
() -> testCase(
685+
buildIndex,
686+
tTest -> fail("Should have thrown exception"),
687+
new AggTestConfig(aggregationBuilder, fieldType1, fieldType2, fieldType3)
688+
)
689+
);
690+
assertEquals("Paired t-test doesn't support filters", ex.getMessage());
691+
} else {
692+
testCase(buildIndex, (Consumer<InternalHistogram>) histogram -> {
693+
if (tTestType == TTestType.HOMOSCEDASTIC) {
694+
assertEquals(buckets, histogram.getBuckets().size());
695+
for (int i = 0; i < buckets; i++) {
696+
InternalTTest ttest = histogram.getBuckets().get(i).getAggregations().get("t_test");
697+
assertEquals(0.03928288693 * tails, ttest.getValue(), 0.00001);
698+
}
699+
} else {
700+
assertEquals(buckets, histogram.getBuckets().size());
701+
for (int i = 0; i < buckets; i++) {
702+
InternalTTest ttest = histogram.getBuckets().get(i).getAggregations().get("t_test");
703+
assertEquals(0.04538666214 * tails, ttest.getValue(), 0.00001);
704+
}
705+
}
706+
}, new AggTestConfig(aggregationBuilder, fieldType1, fieldType2, fieldType3));
707+
}
708+
}
709+
639710
public void testFilterByFilterOrScript() throws IOException {
640711
boolean fieldInA = randomBoolean();
641712
TTestType tTestType = randomFrom(TTestType.HOMOSCEDASTIC, TTestType.HETEROSCEDASTIC);

0 commit comments

Comments
 (0)