Browse Source

ESQL: Limit size of `Literal#toString` (#117842) (#117850)

This `toString` is rendered in task output and progress. Let's make sure it's not massive.
Nik Everett 10 months ago
parent
commit
7ffbb7e27d

+ 5 - 0
docs/changelog/117842.yaml

@@ -0,0 +1,5 @@
+pr: 117842
+summary: Limit size of `Literal#toString`
+area: ES|QL
+type: bug
+issues: []

+ 73 - 12
test/external-modules/esql-heap-attack/src/javaRestTest/java/org/elasticsearch/xpack/esql/heap_attack/HeapAttackIT.java

@@ -295,15 +295,10 @@ public class HeapAttackIT extends ESRestTestCase {
      * 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 class HeapAttackIT extends ESRestTestCase {
      */
     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 @@ public class HeapAttackIT extends ESRestTestCase {
             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 {

+ 5 - 1
x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/expression/Literal.java

@@ -122,7 +122,11 @@ public class Literal extends LeafExpression {
 
     @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

+ 20 - 0
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 org.elasticsearch.xpack.esql.core.type.DataTypeConverter;
 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.INTEGER;
 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<Literal, Expression> {
+
     static class ValueAndCompatibleTypes {
+
         final Supplier<Object> valueSupplier;
         final List<DataType> validDataTypes;
 
@@ -120,6 +127,19 @@ public class LiteralTests extends AbstractNodeTestCase<Literal, Expression> {
         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) {