From db601d74424cb332ffa4461b472581f12a5ba119 Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Mon, 11 Aug 2025 21:56:12 -0400 Subject: [PATCH 1/2] ESQL: Check a few toStrings for aggs Adds `toString` checking for aggregators to the generic aggs test cases so we can make sure they spit out sensible looking results. We have this for scalar functions but it isn't plugged in for aggs and I noticed it while working on #132603 where I stuck `asdf` for the toString thinking I'd fix it when the test failed. It didn't. There's to many changes to grab this in one go so I've made a hook that tests can opt into. We'll drop the hook once everything has opted into it. --- .../function/AbstractAggregationTestCase.java | 43 +++++++++++++++++++ .../function/aggregate/MaxTests.java | 29 +++++++------ .../function/aggregate/MedianTests.java | 13 ++++-- .../function/aggregate/MinTests.java | 29 +++++++------ 4 files changed, 86 insertions(+), 28 deletions(-) diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/AbstractAggregationTestCase.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/AbstractAggregationTestCase.java index e625aa2c0970f..cf9d8e965c5fc 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/AbstractAggregationTestCase.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/AbstractAggregationTestCase.java @@ -42,6 +42,7 @@ import java.util.stream.IntStream; import static org.elasticsearch.xpack.esql.EsqlTestUtils.unboundLogicalOptimizerContext; +import static org.hamcrest.Matchers.endsWith; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.instanceOf; import static org.hamcrest.Matchers.is; @@ -49,6 +50,7 @@ import static org.hamcrest.Matchers.not; import static org.hamcrest.Matchers.nullValue; import static org.hamcrest.Matchers.oneOf; +import static org.hamcrest.Matchers.startsWith; /** * Base class for aggregation tests. @@ -164,6 +166,7 @@ public void testFold() { private void aggregateSingleMode(Expression expression) { Object result; try (var aggregator = aggregator(expression, initialInputChannels(), AggregatorMode.SINGLE)) { + assertAggregatorToString(aggregator); for (Page inputPage : rows(testCase.getMultiRowFields())) { try ( BooleanVector noMasking = driverContext().blockFactory().newConstantBooleanVector(true, inputPage.getPositionCount()) @@ -187,6 +190,7 @@ private void aggregateGroupingSingleMode(Expression expression) { assumeFalse("Grouping aggregations must receive data to check results", pages.isEmpty()); try (var aggregator = groupingAggregator(expression, initialInputChannels(), AggregatorMode.SINGLE)) { + assertAggregatorToString(aggregator); var groupCount = randomIntBetween(1, 1000); for (Page inputPage : pages) { processPageGrouping(aggregator, inputPage, groupCount); @@ -482,4 +486,43 @@ private void processPageGrouping(GroupingAggregator aggregator, Page inputPage, } } } + + private void assertAggregatorToString(Object aggregator) { + if (optIntoToAggregatorToStringChecks() == false) { + return; + } + String expectedStart = switch (aggregator) { + case Aggregator a -> "Aggregator[aggregatorFunction="; + case GroupingAggregator a -> "GroupingAggregator[aggregatorFunction="; + default -> throw new UnsupportedOperationException("can't check toString for [" + aggregator.getClass() + "]"); + }; + String expectedEnd = switch (aggregator) { + case Aggregator a -> "AggregatorFunction[channels=[0]], mode=SINGLE]"; + case GroupingAggregator a -> "GroupingAggregatorFunction[channels=[0]], mode=SINGLE]"; + default -> throw new UnsupportedOperationException("can't check toString for [" + aggregator.getClass() + "]"); + }; + + String toString = aggregator.toString(); + assertThat(toString, startsWith(expectedStart)); + assertThat(toString.substring(expectedStart.length(), toString.length() - expectedEnd.length()), testCase.evaluatorToString()); + assertThat(toString, endsWith(expectedEnd)); + } + + protected boolean optIntoToAggregatorToStringChecks() { + // TODO remove this when everyone has opted in + return false; + } + + protected static String standardAggregatorName(String prefix, DataType type) { + String typeName = switch (type) { + case BOOLEAN -> "Boolean"; + case KEYWORD, TEXT, VERSION -> "BytesRef"; + case DOUBLE -> "Double"; + case INTEGER -> "Int"; + case IP -> "Ip"; + case DATETIME, DATE_NANOS, LONG, UNSIGNED_LONG -> "Long"; + default -> throw new UnsupportedOperationException("name for [" + type + "]"); + }; + return prefix + typeName; + } } diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/aggregate/MaxTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/aggregate/MaxTests.java index 6bc93fbea598f..08636c6fcce2c 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/aggregate/MaxTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/aggregate/MaxTests.java @@ -60,7 +60,7 @@ public static Iterable parameters() { List.of(DataType.INTEGER), () -> new TestCaseSupplier.TestCase( List.of(TestCaseSupplier.TypedData.multiRow(List.of(200), DataType.INTEGER, "field")), - "Max[field=Attribute[channel=0]]", + standardAggregatorName("Max", DataType.INTEGER), DataType.INTEGER, equalTo(200) ) @@ -69,7 +69,7 @@ public static Iterable parameters() { List.of(DataType.LONG), () -> new TestCaseSupplier.TestCase( List.of(TestCaseSupplier.TypedData.multiRow(List.of(200L), DataType.LONG, "field")), - "Max[field=Attribute[channel=0]]", + standardAggregatorName("Max", DataType.LONG), DataType.LONG, equalTo(200L) ) @@ -78,7 +78,7 @@ public static Iterable parameters() { List.of(DataType.UNSIGNED_LONG), () -> new TestCaseSupplier.TestCase( List.of(TestCaseSupplier.TypedData.multiRow(List.of(new BigInteger("200")), DataType.UNSIGNED_LONG, "field")), - "Max[field=Attribute[channel=0]]", + standardAggregatorName("Max", DataType.UNSIGNED_LONG), DataType.UNSIGNED_LONG, equalTo(new BigInteger("200")) ) @@ -87,7 +87,7 @@ public static Iterable parameters() { List.of(DataType.DOUBLE), () -> new TestCaseSupplier.TestCase( List.of(TestCaseSupplier.TypedData.multiRow(List.of(200.), DataType.DOUBLE, "field")), - "Max[field=Attribute[channel=0]]", + standardAggregatorName("Max", DataType.DOUBLE), DataType.DOUBLE, equalTo(200.) ) @@ -96,7 +96,7 @@ public static Iterable parameters() { List.of(DataType.DATETIME), () -> new TestCaseSupplier.TestCase( List.of(TestCaseSupplier.TypedData.multiRow(List.of(200L), DataType.DATETIME, "field")), - "Max[field=Attribute[channel=0]]", + standardAggregatorName("Max", DataType.DATETIME), DataType.DATETIME, equalTo(200L) ) @@ -105,7 +105,7 @@ public static Iterable parameters() { List.of(DataType.DATE_NANOS), () -> new TestCaseSupplier.TestCase( List.of(TestCaseSupplier.TypedData.multiRow(List.of(200L), DataType.DATE_NANOS, "field")), - "Max[field=Attribute[channel=0]]", + standardAggregatorName("Max", DataType.DATE_NANOS), DataType.DATE_NANOS, equalTo(200L) ) @@ -114,7 +114,7 @@ public static Iterable parameters() { List.of(DataType.BOOLEAN), () -> new TestCaseSupplier.TestCase( List.of(TestCaseSupplier.TypedData.multiRow(List.of(true), DataType.BOOLEAN, "field")), - "Max[field=Attribute[channel=0]]", + standardAggregatorName("Max", DataType.BOOLEAN), DataType.BOOLEAN, equalTo(true) ) @@ -129,7 +129,7 @@ public static Iterable parameters() { "field" ) ), - "Max[field=Attribute[channel=0]]", + standardAggregatorName("Max", DataType.IP), DataType.IP, equalTo(new BytesRef(InetAddressPoint.encode(InetAddresses.forString("127.0.0.1")))) ) @@ -138,7 +138,7 @@ public static Iterable parameters() { var value = new BytesRef(randomAlphaOfLengthBetween(0, 50)); return new TestCaseSupplier.TestCase( List.of(TestCaseSupplier.TypedData.multiRow(List.of(value), DataType.KEYWORD, "field")), - "Max[field=Attribute[channel=0]]", + standardAggregatorName("Max", DataType.KEYWORD), DataType.KEYWORD, equalTo(value) ); @@ -147,7 +147,7 @@ public static Iterable parameters() { var value = new BytesRef(randomAlphaOfLengthBetween(0, 50)); return new TestCaseSupplier.TestCase( List.of(TestCaseSupplier.TypedData.multiRow(List.of(value), DataType.TEXT, "field")), - "Max[field=Attribute[channel=0]]", + standardAggregatorName("Max", DataType.TEXT), DataType.KEYWORD, equalTo(value) ); @@ -159,7 +159,7 @@ public static Iterable parameters() { .toBytesRef(); return new TestCaseSupplier.TestCase( List.of(TestCaseSupplier.TypedData.multiRow(List.of(value), DataType.VERSION, "field")), - "Max[field=Attribute[channel=0]]", + standardAggregatorName("Max", DataType.VERSION), DataType.VERSION, equalTo(value) ); @@ -187,10 +187,15 @@ private static TestCaseSupplier makeSupplier(TestCaseSupplier.TypedDataSupplier return new TestCaseSupplier.TestCase( List.of(fieldTypedData), - "Max[field=Attribute[channel=0]]", + standardAggregatorName("Max", fieldSupplier.type()), fieldSupplier.type(), equalTo(expected) ); }); } + + @Override + protected boolean optIntoToAggregatorToStringChecks() { + return true; + } } diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/aggregate/MedianTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/aggregate/MedianTests.java index 1c2c06c1ede94..02a1d26932bab 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/aggregate/MedianTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/aggregate/MedianTests.java @@ -47,7 +47,7 @@ public static Iterable parameters() { List.of(DataType.INTEGER), () -> new TestCaseSupplier.TestCase( List.of(TestCaseSupplier.TypedData.multiRow(List.of(200), DataType.INTEGER, "number")), - "Median[field=Attribute[channel=0]]", + standardAggregatorName("Percentile", DataType.INTEGER), DataType.DOUBLE, equalTo(200.) ) @@ -56,7 +56,7 @@ public static Iterable parameters() { List.of(DataType.LONG), () -> new TestCaseSupplier.TestCase( List.of(TestCaseSupplier.TypedData.multiRow(List.of(200L), DataType.LONG, "number")), - "Median[field=Attribute[channel=0]]", + standardAggregatorName("Percentile", DataType.LONG), DataType.DOUBLE, equalTo(200.) ) @@ -65,7 +65,7 @@ public static Iterable parameters() { List.of(DataType.DOUBLE), () -> new TestCaseSupplier.TestCase( List.of(TestCaseSupplier.TypedData.multiRow(List.of(200.), DataType.DOUBLE, "number")), - "Median[field=Attribute[channel=0]]", + standardAggregatorName("Percentile", DataType.DOUBLE), DataType.DOUBLE, equalTo(200.) ) @@ -94,11 +94,16 @@ private static TestCaseSupplier makeSupplier(TestCaseSupplier.TypedDataSupplier return new TestCaseSupplier.TestCase( List.of(fieldTypedData), - "Median[number=Attribute[channel=0]]", + standardAggregatorName("Percentile", fieldSupplier.type()), DataType.DOUBLE, equalTo(expected) ); } }); } + + @Override + protected boolean optIntoToAggregatorToStringChecks() { + return true; + } } diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/aggregate/MinTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/aggregate/MinTests.java index 391d130350a90..ea75982b627c5 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/aggregate/MinTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/aggregate/MinTests.java @@ -60,7 +60,7 @@ public static Iterable parameters() { List.of(DataType.INTEGER), () -> new TestCaseSupplier.TestCase( List.of(TestCaseSupplier.TypedData.multiRow(List.of(200), DataType.INTEGER, "field")), - "Min[field=Attribute[channel=0]]", + standardAggregatorName("Min", DataType.INTEGER), DataType.INTEGER, equalTo(200) ) @@ -69,7 +69,7 @@ public static Iterable parameters() { List.of(DataType.LONG), () -> new TestCaseSupplier.TestCase( List.of(TestCaseSupplier.TypedData.multiRow(List.of(200L), DataType.LONG, "field")), - "Min[field=Attribute[channel=0]]", + standardAggregatorName("Min", DataType.LONG), DataType.LONG, equalTo(200L) ) @@ -78,7 +78,7 @@ public static Iterable parameters() { List.of(DataType.UNSIGNED_LONG), () -> new TestCaseSupplier.TestCase( List.of(TestCaseSupplier.TypedData.multiRow(List.of(new BigInteger("200")), DataType.UNSIGNED_LONG, "field")), - "Max[field=Attribute[channel=0]]", + standardAggregatorName("Min", DataType.UNSIGNED_LONG), DataType.UNSIGNED_LONG, equalTo(new BigInteger("200")) ) @@ -87,7 +87,7 @@ public static Iterable parameters() { List.of(DataType.DOUBLE), () -> new TestCaseSupplier.TestCase( List.of(TestCaseSupplier.TypedData.multiRow(List.of(200.), DataType.DOUBLE, "field")), - "Min[field=Attribute[channel=0]]", + standardAggregatorName("Min", DataType.DOUBLE), DataType.DOUBLE, equalTo(200.) ) @@ -96,7 +96,7 @@ public static Iterable parameters() { List.of(DataType.DATETIME), () -> new TestCaseSupplier.TestCase( List.of(TestCaseSupplier.TypedData.multiRow(List.of(200L), DataType.DATETIME, "field")), - "Min[field=Attribute[channel=0]]", + standardAggregatorName("Min", DataType.DATETIME), DataType.DATETIME, equalTo(200L) ) @@ -105,7 +105,7 @@ public static Iterable parameters() { List.of(DataType.DATE_NANOS), () -> new TestCaseSupplier.TestCase( List.of(TestCaseSupplier.TypedData.multiRow(List.of(200L), DataType.DATE_NANOS, "field")), - "Min[field=Attribute[channel=0]]", + standardAggregatorName("Min", DataType.DATE_NANOS), DataType.DATE_NANOS, equalTo(200L) ) @@ -114,7 +114,7 @@ public static Iterable parameters() { List.of(DataType.BOOLEAN), () -> new TestCaseSupplier.TestCase( List.of(TestCaseSupplier.TypedData.multiRow(List.of(true), DataType.BOOLEAN, "field")), - "Min[field=Attribute[channel=0]]", + standardAggregatorName("Min", DataType.BOOLEAN), DataType.BOOLEAN, equalTo(true) ) @@ -129,7 +129,7 @@ public static Iterable parameters() { "field" ) ), - "Min[field=Attribute[channel=0]]", + standardAggregatorName("Min", DataType.IP), DataType.IP, equalTo(new BytesRef(InetAddressPoint.encode(InetAddresses.forString("127.0.0.1")))) ) @@ -138,7 +138,7 @@ public static Iterable parameters() { var value = new BytesRef(randomAlphaOfLengthBetween(0, 50)); return new TestCaseSupplier.TestCase( List.of(TestCaseSupplier.TypedData.multiRow(List.of(value), DataType.KEYWORD, "field")), - "Min[field=Attribute[channel=0]]", + standardAggregatorName("Min", DataType.KEYWORD), DataType.KEYWORD, equalTo(value) ); @@ -147,7 +147,7 @@ public static Iterable parameters() { var value = new BytesRef(randomAlphaOfLengthBetween(0, 50)); return new TestCaseSupplier.TestCase( List.of(TestCaseSupplier.TypedData.multiRow(List.of(value), DataType.TEXT, "field")), - "Min[field=Attribute[channel=0]]", + standardAggregatorName("Min", DataType.TEXT), DataType.KEYWORD, equalTo(value) ); @@ -159,7 +159,7 @@ public static Iterable parameters() { .toBytesRef(); return new TestCaseSupplier.TestCase( List.of(TestCaseSupplier.TypedData.multiRow(List.of(value), DataType.VERSION, "field")), - "Min[field=Attribute[channel=0]]", + standardAggregatorName("Min", DataType.VERSION), DataType.VERSION, equalTo(value) ); @@ -187,10 +187,15 @@ private static TestCaseSupplier makeSupplier(TestCaseSupplier.TypedDataSupplier return new TestCaseSupplier.TestCase( List.of(fieldTypedData), - "Min[field=Attribute[channel=0]]", + standardAggregatorName("Min", fieldSupplier.type()), fieldSupplier.type(), equalTo(expected) ); }); } + + @Override + protected boolean optIntoToAggregatorToStringChecks() { + return true; + } } From 348f740c4def496e437f10e7ab8feb85edd49ee6 Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Mon, 18 Aug 2025 14:24:14 -0400 Subject: [PATCH 2/2] Fixup --- .../function/AbstractAggregationTestCase.java | 18 ++++++++++++++++++ .../function/aggregate/RateTests.java | 5 +++++ 2 files changed, 23 insertions(+) diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/AbstractAggregationTestCase.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/AbstractAggregationTestCase.java index cf9d8e965c5fc..5fff84f0cf021 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/AbstractAggregationTestCase.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/AbstractAggregationTestCase.java @@ -142,12 +142,30 @@ public void testAggregate() { resolveExpression(expression, this::aggregateSingleMode, this::evaluate); } + public void testAggregateToString() { + Expression expression = randomBoolean() ? buildDeepCopyOfFieldExpression(testCase) : buildFieldExpression(testCase); + resolveExpression(expression, e -> { + try (var aggregator = aggregator(e, initialInputChannels(), AggregatorMode.SINGLE)) { + assertAggregatorToString(aggregator); + } + }, this::evaluate); + } + public void testGroupingAggregate() { Expression expression = randomBoolean() ? buildDeepCopyOfFieldExpression(testCase) : buildFieldExpression(testCase); resolveExpression(expression, this::aggregateGroupingSingleMode, this::evaluate); } + public void testGroupingAggregateToString() { + Expression expression = randomBoolean() ? buildDeepCopyOfFieldExpression(testCase) : buildFieldExpression(testCase); + resolveExpression(expression, e -> { + try (var aggregator = groupingAggregator(e, initialInputChannels(), AggregatorMode.SINGLE)) { + assertAggregatorToString(aggregator); + } + }, this::evaluate); + } + public void testAggregateIntermediate() { Expression expression = randomBoolean() ? buildDeepCopyOfFieldExpression(testCase) : buildFieldExpression(testCase); diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/aggregate/RateTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/aggregate/RateTests.java index 7da479ea28dba..ff911d215e613 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/aggregate/RateTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/aggregate/RateTests.java @@ -60,6 +60,11 @@ public void testAggregate() { assumeTrue("time-series aggregation doesn't support ungrouped", false); } + @Override + public void testAggregateToString() { + assumeTrue("time-series aggregation doesn't support ungrouped", false); + } + @Override public void testAggregateIntermediate() { assumeTrue("time-series aggregation doesn't support ungrouped", false);