From 1837133d2138ca73307f919d8dd02e1faf83de69 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Iv=C3=A1n=20Cea=20Fontenla?= Date: Wed, 31 Jul 2024 17:29:05 +0200 Subject: [PATCH] Unmute ValuesTests and reduce strings memory usage (#111455) Strings tests were taking too much memory in the worst cases. And VALUES() store all the values. Fixes https://github.com/elastic/elasticsearch/issues/111428 Fixes https://github.com/elastic/elasticsearch/issues/111429 --- muted-tests.yml | 6 -- .../function/MultiRowTestCaseSupplier.java | 83 ++++++++++++------- .../aggregate/CountDistinctTests.java | 5 +- .../function/aggregate/CountTests.java | 5 +- .../function/aggregate/ValuesTests.java | 4 +- 5 files changed, 60 insertions(+), 43 deletions(-) diff --git a/muted-tests.yml b/muted-tests.yml index fe26427aaca76..282e7be99142f 100644 --- a/muted-tests.yml +++ b/muted-tests.yml @@ -126,12 +126,6 @@ tests: issue: https://github.com/elastic/elasticsearch/issues/111280 - class: org.elasticsearch.xpack.repositories.metering.azure.AzureRepositoriesMeteringIT issue: https://github.com/elastic/elasticsearch/issues/111307 -- class: org.elasticsearch.xpack.esql.expression.function.aggregate.ValuesTests - method: testGroupingAggregate {TestCase=} - issue: https://github.com/elastic/elasticsearch/issues/111429 -- class: org.elasticsearch.xpack.esql.expression.function.aggregate.ValuesTests - method: testGroupingAggregate {TestCase=} - issue: https://github.com/elastic/elasticsearch/issues/111428 - class: org.elasticsearch.xpack.restart.FullClusterRestartIT method: testSingleDoc {cluster=UPGRADED} issue: https://github.com/elastic/elasticsearch/issues/111434 diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/MultiRowTestCaseSupplier.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/MultiRowTestCaseSupplier.java index 973249e4a743c..e740533462746 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/MultiRowTestCaseSupplier.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/MultiRowTestCaseSupplier.java @@ -469,36 +469,61 @@ public static List cartesianPointCases(int minRows, int maxRo } public static List stringCases(int minRows, int maxRows, DataType type) { - return List.of( - new TypedDataSupplier("", () -> randomList(minRows, maxRows, () -> new BytesRef("")), type, false, true), - new TypedDataSupplier( - "", - () -> randomList(minRows, maxRows, () -> new BytesRef(ESTestCase.randomAlphaOfLengthBetween(1, 30))), - type, - false, - true - ), - new TypedDataSupplier( - "", - () -> randomList(minRows, maxRows, () -> new BytesRef(ESTestCase.randomAlphaOfLengthBetween(300, 3000))), - type, - false, - true - ), - new TypedDataSupplier( - "", - () -> randomList(minRows, maxRows, () -> new BytesRef(ESTestCase.randomRealisticUnicodeOfLengthBetween(1, 30))), - type, - false, - true - ), - new TypedDataSupplier( - "", - () -> randomList(minRows, maxRows, () -> new BytesRef(ESTestCase.randomRealisticUnicodeOfLengthBetween(300, 3000))), - type, - false, - true + List cases = new ArrayList<>(); + + cases.addAll( + List.of( + new TypedDataSupplier( + "", + () -> randomList(minRows, maxRows, () -> new BytesRef("")), + type, + false, + true + ), + new TypedDataSupplier( + "", + () -> randomList(minRows, maxRows, () -> new BytesRef(ESTestCase.randomAlphaOfLengthBetween(1, 30))), + type, + false, + true + ), + new TypedDataSupplier( + "", + () -> randomList(minRows, maxRows, () -> new BytesRef(ESTestCase.randomRealisticUnicodeOfLengthBetween(1, 30))), + type, + false, + true + ) ) ); + + if (minRows <= 100) { + var longStringsMaxRows = Math.min(maxRows, 100); + + cases.addAll( + List.of( + new TypedDataSupplier( + "", + () -> randomList(minRows, longStringsMaxRows, () -> new BytesRef(ESTestCase.randomAlphaOfLengthBetween(300, 1000))), + type, + false, + true + ), + new TypedDataSupplier( + "", + () -> randomList( + minRows, + longStringsMaxRows, + () -> new BytesRef(ESTestCase.randomRealisticUnicodeOfLengthBetween(300, 1000)) + ), + type, + false, + true + ) + ) + ); + } + + return cases; } } diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/aggregate/CountDistinctTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/aggregate/CountDistinctTests.java index c2638e8da9196..5e23083d7c810 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/aggregate/CountDistinctTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/aggregate/CountDistinctTests.java @@ -55,9 +55,8 @@ public static Iterable parameters() { MultiRowTestCaseSupplier.booleanCases(1, 1000), MultiRowTestCaseSupplier.ipCases(1, 1000), MultiRowTestCaseSupplier.versionCases(1, 1000), - // Lower values for strings, as they take more space and may trigger the circuit breaker - MultiRowTestCaseSupplier.stringCases(1, 100, DataType.KEYWORD), - MultiRowTestCaseSupplier.stringCases(1, 100, DataType.TEXT) + MultiRowTestCaseSupplier.stringCases(1, 1000, DataType.KEYWORD), + MultiRowTestCaseSupplier.stringCases(1, 1000, DataType.TEXT) ).flatMap(List::stream).forEach(fieldCaseSupplier -> { // With precision for (var precisionCaseSupplier : precisionSuppliers) { diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/aggregate/CountTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/aggregate/CountTests.java index 09076f2d70fd9..979048534edbf 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/aggregate/CountTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/aggregate/CountTests.java @@ -46,9 +46,8 @@ public static Iterable parameters() { MultiRowTestCaseSupplier.versionCases(1, 1000), MultiRowTestCaseSupplier.geoPointCases(1, 1000, true), MultiRowTestCaseSupplier.cartesianPointCases(1, 1000, true), - // Lower values for strings, as they take more space and may trigger the circuit breaker - MultiRowTestCaseSupplier.stringCases(1, 100, DataType.KEYWORD), - MultiRowTestCaseSupplier.stringCases(1, 100, DataType.TEXT) + MultiRowTestCaseSupplier.stringCases(1, 1000, DataType.KEYWORD), + MultiRowTestCaseSupplier.stringCases(1, 1000, DataType.TEXT) ).flatMap(List::stream).map(CountTests::makeSupplier).collect(Collectors.toCollection(() -> suppliers)); // No rows diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/aggregate/ValuesTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/aggregate/ValuesTests.java index 704bd3ab204a3..23b70b94d0d7f 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/aggregate/ValuesTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/aggregate/ValuesTests.java @@ -49,8 +49,8 @@ public static Iterable parameters() { MultiRowTestCaseSupplier.ipCases(1, 1000), MultiRowTestCaseSupplier.versionCases(1, 1000), // Lower values for strings, as they take more space and may trigger the circuit breaker - MultiRowTestCaseSupplier.stringCases(1, 100, DataType.KEYWORD), - MultiRowTestCaseSupplier.stringCases(1, 100, DataType.TEXT) + MultiRowTestCaseSupplier.stringCases(1, 20, DataType.KEYWORD), + MultiRowTestCaseSupplier.stringCases(1, 20, DataType.TEXT) ).flatMap(List::stream).map(ValuesTests::makeSupplier).collect(Collectors.toCollection(() -> suppliers)); return parameterSuppliersFromTypedDataWithDefaultChecks(suppliers);