From 7b3142e1d71188ab2422219ddd02731097ce042c Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Mon, 2 Dec 2024 14:19:41 -0500 Subject: [PATCH] ESQL: Limit size of `Literal#toString` (#117842) This `toString` is rendered in task output and progress. Let's make sure it's not massive. --- docs/changelog/117842.yaml | 5 ++ .../xpack/esql/heap_attack/HeapAttackIT.java | 85 ++++++++++++++++--- .../xpack/esql/core/expression/Literal.java | 6 +- .../esql/core/expression/LiteralTests.java | 20 +++++ 4 files changed, 103 insertions(+), 13 deletions(-) create mode 100644 docs/changelog/117842.yaml diff --git a/docs/changelog/117842.yaml b/docs/changelog/117842.yaml new file mode 100644 index 0000000000000..9b528a158288c --- /dev/null +++ b/docs/changelog/117842.yaml @@ -0,0 +1,5 @@ +pr: 117842 +summary: Limit size of `Literal#toString` +area: ES|QL +type: bug +issues: [] diff --git a/test/external-modules/esql-heap-attack/src/javaRestTest/java/org/elasticsearch/xpack/esql/heap_attack/HeapAttackIT.java b/test/external-modules/esql-heap-attack/src/javaRestTest/java/org/elasticsearch/xpack/esql/heap_attack/HeapAttackIT.java index 008a056e87901..8b9176a346e30 100644 --- a/test/external-modules/esql-heap-attack/src/javaRestTest/java/org/elasticsearch/xpack/esql/heap_attack/HeapAttackIT.java +++ b/test/external-modules/esql-heap-attack/src/javaRestTest/java/org/elasticsearch/xpack/esql/heap_attack/HeapAttackIT.java @@ -295,15 +295,10 @@ private Response concat(int evals) throws IOException { * Returns many moderately long strings. */ public void testManyConcat() throws IOException { + int strings = 300; initManyLongs(); - Response resp = manyConcat(300); - Map map = responseAsMap(resp); - ListMatcher columns = matchesList(); - for (int s = 0; s < 300; s++) { - columns = columns.item(matchesMap().entry("name", "str" + s).entry("type", "keyword")); - } - MapMatcher mapMatcher = matchesMap(); - assertMap(map, mapMatcher.entry("columns", columns).entry("values", any(List.class)).entry("took", greaterThanOrEqualTo(0))); + Response resp = manyConcat("FROM manylongs", strings); + assertManyStrings(resp, strings); } /** @@ -311,15 +306,24 @@ public void testManyConcat() throws IOException { */ public void testHugeManyConcat() throws IOException { initManyLongs(); - assertCircuitBreaks(() -> manyConcat(2000)); + assertCircuitBreaks(() -> manyConcat("FROM manylongs", 2000)); + } + + /** + * Returns many moderately long strings. + */ + public void testManyConcatFromRow() throws IOException { + int strings = 2000; + Response resp = manyConcat("ROW a=9999, b=9999, c=9999, d=9999, e=9999", strings); + assertManyStrings(resp, strings); } /** * Tests that generate many moderately long strings. */ - private Response manyConcat(int strings) throws IOException { + private Response manyConcat(String init, int strings) throws IOException { StringBuilder query = startQuery(); - query.append("FROM manylongs | EVAL str = CONCAT("); + query.append(init).append(" | EVAL str = CONCAT("); query.append( Arrays.stream(new String[] { "a", "b", "c", "d", "e" }) .map(f -> "TO_STRING(" + f + ")") @@ -344,7 +348,64 @@ private Response manyConcat(int strings) throws IOException { query.append("str").append(s); } query.append("\"}"); - return query(query.toString(), null); + return query(query.toString(), "columns"); + } + + /** + * Returns many moderately long strings. + */ + public void testManyRepeat() throws IOException { + int strings = 30; + initManyLongs(); + Response resp = manyRepeat("FROM manylongs", strings); + assertManyStrings(resp, 30); + } + + /** + * Hits a circuit breaker by building many moderately long strings. + */ + public void testHugeManyRepeat() throws IOException { + initManyLongs(); + assertCircuitBreaks(() -> manyRepeat("FROM manylongs", 75)); + } + + /** + * Returns many moderately long strings. + */ + public void testManyRepeatFromRow() throws IOException { + int strings = 10000; + Response resp = manyRepeat("ROW a = 99", strings); + assertManyStrings(resp, strings); + } + + /** + * Tests that generate many moderately long strings. + */ + private Response manyRepeat(String init, int strings) throws IOException { + StringBuilder query = startQuery(); + query.append(init).append(" | EVAL str = TO_STRING(a)"); + for (int s = 0; s < strings; s++) { + query.append(",\nstr").append(s).append("=REPEAT(str, 10000)"); + } + query.append("\n|KEEP "); + for (int s = 0; s < strings; s++) { + if (s != 0) { + query.append(", "); + } + query.append("str").append(s); + } + query.append("\"}"); + return query(query.toString(), "columns"); + } + + private void assertManyStrings(Response resp, int strings) throws IOException { + Map map = responseAsMap(resp); + ListMatcher columns = matchesList(); + for (int s = 0; s < strings; s++) { + columns = columns.item(matchesMap().entry("name", "str" + s).entry("type", "keyword")); + } + MapMatcher mapMatcher = matchesMap(); + assertMap(map, mapMatcher.entry("columns", columns)); } public void testManyEval() throws IOException { diff --git a/x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/expression/Literal.java b/x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/expression/Literal.java index 20cdbaf6acdbf..53f559c5c82fe 100644 --- a/x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/expression/Literal.java +++ b/x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/expression/Literal.java @@ -122,7 +122,11 @@ public boolean equals(Object obj) { @Override public String toString() { - return String.valueOf(value); + String str = String.valueOf(value); + if (str.length() > 500) { + return str.substring(0, 500) + "..."; + } + return str; } @Override diff --git a/x-pack/plugin/esql-core/src/test/java/org/elasticsearch/xpack/esql/core/expression/LiteralTests.java b/x-pack/plugin/esql-core/src/test/java/org/elasticsearch/xpack/esql/core/expression/LiteralTests.java index a4c67a8076479..a628916e67746 100644 --- a/x-pack/plugin/esql-core/src/test/java/org/elasticsearch/xpack/esql/core/expression/LiteralTests.java +++ b/x-pack/plugin/esql-core/src/test/java/org/elasticsearch/xpack/esql/core/expression/LiteralTests.java @@ -6,9 +6,12 @@ */ package org.elasticsearch.xpack.esql.core.expression; +import joptsimple.internal.Strings; + import org.elasticsearch.test.ESTestCase; import org.elasticsearch.xpack.esql.core.InvalidArgumentException; import org.elasticsearch.xpack.esql.core.tree.AbstractNodeTestCase; +import org.elasticsearch.xpack.esql.core.tree.Source; import org.elasticsearch.xpack.esql.core.tree.SourceTests; import org.elasticsearch.xpack.esql.core.type.Converter; import org.elasticsearch.xpack.esql.core.type.DataType; @@ -17,6 +20,7 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.List; +import java.util.Objects; import java.util.function.Function; import java.util.function.Supplier; @@ -29,9 +33,12 @@ import static org.elasticsearch.xpack.esql.core.type.DataType.KEYWORD; import static org.elasticsearch.xpack.esql.core.type.DataType.LONG; import static org.elasticsearch.xpack.esql.core.type.DataType.SHORT; +import static org.hamcrest.Matchers.equalTo; public class LiteralTests extends AbstractNodeTestCase { + static class ValueAndCompatibleTypes { + final Supplier valueSupplier; final List validDataTypes; @@ -120,6 +127,19 @@ public void testReplaceChildren() { assertEquals("this type of node doesn't have any children to replace", e.getMessage()); } + public void testToString() { + assertThat(new Literal(Source.EMPTY, 1, LONG).toString(), equalTo("1")); + assertThat(new Literal(Source.EMPTY, "short", KEYWORD).toString(), equalTo("short")); + // toString should limit it's length + String tooLong = Strings.repeat('a', 510); + assertThat(new Literal(Source.EMPTY, tooLong, KEYWORD).toString(), equalTo(Strings.repeat('a', 500) + "...")); + + for (ValueAndCompatibleTypes g : GENERATORS) { + Literal lit = new Literal(Source.EMPTY, g.valueSupplier.get(), randomFrom(g.validDataTypes)); + assertThat(lit.toString(), equalTo(Objects.toString(lit.value()))); + } + } + private static Object randomValueOfTypeOtherThan(Object original, DataType type) { for (ValueAndCompatibleTypes gen : GENERATORS) { if (gen.validDataTypes.get(0) == type) {