ESQL: Limit size of Literal#toString (elastic#117842)

This `toString` is rendered in task output and progress. Let's make sure it's not massive.
nik9000 · Dec 2, 2024 · 7b3142e · 7b3142e
1 parent 205675d
commit 7b3142e
Show file tree

Hide file tree

Showing 4 changed files with 103 additions and 13 deletions.
diff --git a/docs/changelog/117842.yaml 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/...p-attack/src/javaRestTest/java/org/elasticsearch/xpack/esql/heap_attack/HeapAttackIT.java b/...p-attack/src/javaRestTest/java/org/elasticsearch/xpack/esql/heap_attack/HeapAttackIT.java
@@ -295,31 +295,35 @@ 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);
     }
 
     /**
      * Hits a circuit breaker by building many moderately long strings.
      */
     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/.../plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/expression/Literal.java b/.../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/...in/esql-core/src/test/java/org/elasticsearch/xpack/esql/core/expression/LiteralTests.java b/...in/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<Literal, Expression> {
+
     static class ValueAndCompatibleTypes {
+
         final Supplier<Object> valueSupplier;
         final List<DataType> 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) {