Skip to content

Commit ab4147a

Browse files
luigidellaquilaluigidellaquila
committed
ES|QL: No plain strings in Literal (elastic#129399)
1 parent 07dfcde commit ab4147a

File tree

57 files changed

+367
-276
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

57 files changed

+367
-276
lines changed

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

Lines changed: 40 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,18 +11,24 @@
1111
import org.elasticsearch.common.io.stream.NamedWriteableRegistry;
1212
import org.elasticsearch.common.io.stream.StreamInput;
1313
import org.elasticsearch.common.io.stream.StreamOutput;
14+
import org.elasticsearch.common.lucene.BytesRefs;
1415
import org.elasticsearch.xpack.esql.core.QlIllegalArgumentException;
1516
import org.elasticsearch.xpack.esql.core.tree.NodeInfo;
1617
import org.elasticsearch.xpack.esql.core.tree.Source;
1718
import org.elasticsearch.xpack.esql.core.type.DataType;
1819
import org.elasticsearch.xpack.esql.core.util.PlanStreamInput;
20+
import org.elasticsearch.xpack.versionfield.Version;
1921

2022
import java.io.IOException;
23+
import java.util.Collection;
2124
import java.util.List;
2225
import java.util.Objects;
2326

2427
import static org.elasticsearch.xpack.esql.core.type.DataType.CARTESIAN_POINT;
2528
import static org.elasticsearch.xpack.esql.core.type.DataType.GEO_POINT;
29+
import static org.elasticsearch.xpack.esql.core.type.DataType.KEYWORD;
30+
import static org.elasticsearch.xpack.esql.core.type.DataType.TEXT;
31+
import static org.elasticsearch.xpack.esql.core.type.DataType.VERSION;
2632
import static org.elasticsearch.xpack.esql.core.util.SpatialCoordinateTypes.CARTESIAN;
2733
import static org.elasticsearch.xpack.esql.core.util.SpatialCoordinateTypes.GEO;
2834

@@ -45,10 +51,25 @@ public class Literal extends LeafExpression {
4551

4652
public Literal(Source source, Object value, DataType dataType) {
4753
super(source);
54+
assert noPlainStrings(value, dataType);
4855
this.dataType = dataType;
4956
this.value = value;
5057
}
5158

59+
private boolean noPlainStrings(Object value, DataType dataType) {
60+
if (dataType == KEYWORD || dataType == TEXT || dataType == VERSION) {
61+
if (value == null) {
62+
return true;
63+
}
64+
if (value instanceof String) {
65+
return false;
66+
} else if (value instanceof Collection<?> c) {
67+
return c.stream().allMatch(x -> noPlainStrings(x, dataType));
68+
}
69+
}
70+
return true;
71+
}
72+
5273
private static Literal readFrom(StreamInput in) throws IOException {
5374
Source source = Source.readFrom((StreamInput & PlanStreamInput) in);
5475
Object value = in.readGenericValue();
@@ -122,7 +143,21 @@ public boolean equals(Object obj) {
122143

123144
@Override
124145
public String toString() {
125-
String str = String.valueOf(value);
146+
String str;
147+
if (dataType == KEYWORD || dataType == TEXT) {
148+
str = BytesRefs.toString(value);
149+
} else if (dataType == VERSION && value instanceof BytesRef br) {
150+
str = new Version(br).toString();
151+
// TODO review how we manage IPs: https://github.com/elastic/elasticsearch/issues/129605
152+
// } else if (dataType == IP && value instanceof BytesRef ip) {
153+
// str = DocValueFormat.IP.format(ip);
154+
} else {
155+
str = String.valueOf(value);
156+
}
157+
158+
if (str == null) {
159+
str = "null";
160+
}
126161
if (str.length() > 500) {
127162
return str.substring(0, 500) + "...";
128163
}
@@ -154,6 +189,10 @@ public static Literal of(Expression source, Object value) {
154189
return new Literal(source.source(), value, source.dataType());
155190
}
156191

192+
public static Literal keyword(Source source, String literal) {
193+
return new Literal(source, BytesRefs.toBytesRef(literal), KEYWORD);
194+
}
195+
157196
/**
158197
* Not all literal values are currently supported in StreamInput/StreamOutput as generic values.
159198
* This mapper allows for addition of new and interesting values without (yet) adding to StreamInput/Output.

‎x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/expression/MapExpression.java‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
import org.elasticsearch.common.io.stream.NamedWriteableRegistry;
1111
import org.elasticsearch.common.io.stream.StreamInput;
1212
import org.elasticsearch.common.io.stream.StreamOutput;
13+
import org.elasticsearch.common.lucene.BytesRefs;
1314
import org.elasticsearch.xpack.esql.core.tree.NodeInfo;
1415
import org.elasticsearch.xpack.esql.core.tree.Source;
1516
import org.elasticsearch.xpack.esql.core.type.DataType;
@@ -39,7 +40,7 @@ public class MapExpression extends Expression {
3940

4041
private final Map<Expression, Expression> map;
4142

42-
private final Map<Object, Expression> keyFoldedMap;
43+
private final Map<String, Expression> keyFoldedMap;
4344

4445
public MapExpression(Source source, List<Expression> entries) {
4546
super(source, entries);
@@ -54,7 +55,7 @@ public MapExpression(Source source, List<Expression> entries) {
5455
entryExpressions.add(new EntryExpression(key.source(), key, value));
5556
map.put(key, value);
5657
if (key instanceof Literal l) {
57-
this.keyFoldedMap.put(l.value(), value);
58+
this.keyFoldedMap.put(BytesRefs.toString(l.value()), value);
5859
}
5960
}
6061
}
@@ -95,7 +96,7 @@ public Map<Expression, Expression> map() {
9596
return map;
9697
}
9798

98-
public Map<Object, Expression> keyFoldedMap() {
99+
public Map<String, Expression> keyFoldedMap() {
99100
return keyFoldedMap;
100101
}
101102

‎x-pack/plugin/esql-core/src/test/java/org/elasticsearch/xpack/esql/core/expression/LiteralTests.java‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88

99
import joptsimple.internal.Strings;
1010

11+
import org.elasticsearch.common.lucene.BytesRefs;
1112
import org.elasticsearch.test.ESTestCase;
1213
import org.elasticsearch.xpack.esql.core.InvalidArgumentException;
1314
import org.elasticsearch.xpack.esql.core.tree.AbstractNodeTestCase;
@@ -20,7 +21,6 @@
2021
import java.util.ArrayList;
2122
import java.util.Arrays;
2223
import java.util.List;
23-
import java.util.Objects;
2424
import java.util.function.Function;
2525
import java.util.function.Supplier;
2626

@@ -62,7 +62,7 @@ static class ValueAndCompatibleTypes {
6262
new ValueAndCompatibleTypes(ESTestCase::randomLong, LONG, FLOAT, DOUBLE, BOOLEAN),
6363
new ValueAndCompatibleTypes(ESTestCase::randomFloat, FLOAT, LONG, DOUBLE, BOOLEAN),
6464
new ValueAndCompatibleTypes(ESTestCase::randomDouble, DOUBLE, LONG, FLOAT, BOOLEAN),
65-
new ValueAndCompatibleTypes(() -> randomAlphaOfLength(5), KEYWORD)
65+
new ValueAndCompatibleTypes(() -> BytesRefs.toBytesRef(randomAlphaOfLength(5)), KEYWORD)
6666
);
6767

6868
public static Literal randomLiteral() {
@@ -129,14 +129,14 @@ public void testReplaceChildren() {
129129

130130
public void testToString() {
131131
assertThat(new Literal(Source.EMPTY, 1, LONG).toString(), equalTo("1"));
132-
assertThat(new Literal(Source.EMPTY, "short", KEYWORD).toString(), equalTo("short"));
132+
assertThat(new Literal(Source.EMPTY, BytesRefs.toBytesRef("short"), KEYWORD).toString(), equalTo("short"));
133133
// toString should limit it's length
134134
String tooLong = Strings.repeat('a', 510);
135-
assertThat(new Literal(Source.EMPTY, tooLong, KEYWORD).toString(), equalTo(Strings.repeat('a', 500) + "..."));
135+
assertThat(new Literal(Source.EMPTY, BytesRefs.toBytesRef(tooLong), KEYWORD).toString(), equalTo(Strings.repeat('a', 500) + "..."));
136136

137137
for (ValueAndCompatibleTypes g : GENERATORS) {
138138
Literal lit = new Literal(Source.EMPTY, g.valueSupplier.get(), randomFrom(g.validDataTypes));
139-
assertThat(lit.toString(), equalTo(Objects.toString(lit.value())));
139+
assertThat(lit.toString(), equalTo(BytesRefs.toString(lit.value())));
140140
}
141141
}
142142

‎x-pack/plugin/esql-core/src/test/java/org/elasticsearch/xpack/esql/core/expression/function/scalar/FunctionTestUtils.java‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77

88
package org.elasticsearch.xpack.esql.core.expression.function.scalar;
99

10+
import org.elasticsearch.common.lucene.BytesRefs;
1011
import org.elasticsearch.xpack.esql.core.expression.Literal;
1112
import org.elasticsearch.xpack.esql.core.type.DataType;
1213

@@ -15,10 +16,13 @@
1516
public final class FunctionTestUtils {
1617

1718
public static Literal l(Object value) {
18-
return new Literal(EMPTY, value, DataType.fromJava(value));
19+
return l(value, DataType.fromJava(value));
1920
}
2021

2122
public static Literal l(Object value, DataType type) {
23+
if ((type == DataType.TEXT || type == DataType.KEYWORD) && value instanceof String) {
24+
value = BytesRefs.toBytesRef(value);
25+
}
2226
return new Literal(EMPTY, value, type);
2327
}
2428
}

‎x-pack/plugin/esql-core/src/test/java/org/elasticsearch/xpack/esql/core/util/TestUtils.java‎

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77

88
package org.elasticsearch.xpack.esql.core.util;
99

10+
import org.elasticsearch.common.lucene.BytesRefs;
1011
import org.elasticsearch.xpack.esql.core.expression.FieldAttribute;
1112
import org.elasticsearch.xpack.esql.core.expression.Literal;
1213
import org.elasticsearch.xpack.esql.core.tree.Source;
@@ -22,6 +23,8 @@
2223
import static org.elasticsearch.test.ESTestCase.randomFrom;
2324
import static org.elasticsearch.xpack.esql.core.tree.Source.EMPTY;
2425
import static org.elasticsearch.xpack.esql.core.type.DataType.INTEGER;
26+
import static org.elasticsearch.xpack.esql.core.type.DataType.KEYWORD;
27+
import static org.elasticsearch.xpack.esql.core.type.DataType.TEXT;
2528

2629
public final class TestUtils {
2730
private TestUtils() {}
@@ -39,6 +42,10 @@ public static Literal of(Source source, Object value) {
3942
if (value instanceof Literal) {
4043
return (Literal) value;
4144
}
45+
DataType type = DataType.fromJava(value);
46+
if ((type == TEXT || type == KEYWORD) && value instanceof String) {
47+
value = BytesRefs.toBytesRef(value);
48+
}
4249
return new Literal(source, value, DataType.fromJava(value));
4350
}
4451

@@ -58,12 +65,16 @@ public static FieldAttribute getFieldAttribute(String name, DataType dataType) {
5865
return new FieldAttribute(EMPTY, name, new EsField(name + "f", dataType, emptyMap(), true));
5966
}
6067

61-
/** Similar to {@link String#strip()}, but removes the WS throughout the entire string. */
68+
/**
69+
* Similar to {@link String#strip()}, but removes the WS throughout the entire string.
70+
*/
6271
public static String stripThrough(String input) {
6372
return WS_PATTERN.matcher(input).replaceAll(StringUtils.EMPTY);
6473
}
6574

66-
/** Returns the input string, but with parts of it having the letter casing changed. */
75+
/**
76+
* Returns the input string, but with parts of it having the letter casing changed.
77+
*/
6778
public static String randomCasing(String input) {
6879
StringBuilder sb = new StringBuilder(input.length());
6980
for (int i = 0, inputLen = input.length(), step = (int) Math.sqrt(inputLen), chunkEnd; i < inputLen; i += step) {

‎x-pack/plugin/esql/qa/testFixtures/src/main/java/org/elasticsearch/xpack/esql/EsqlTestUtils.java‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
import org.elasticsearch.common.breaker.CircuitBreaker;
2121
import org.elasticsearch.common.breaker.NoopCircuitBreaker;
2222
import org.elasticsearch.common.bytes.BytesReference;
23+
import org.elasticsearch.common.lucene.BytesRefs;
2324
import org.elasticsearch.common.regex.Regex;
2425
import org.elasticsearch.common.settings.Settings;
2526
import org.elasticsearch.common.util.BigArrays;
@@ -226,7 +227,11 @@ public static Literal of(Source source, Object value) {
226227
if (value instanceof Literal) {
227228
return (Literal) value;
228229
}
229-
return new Literal(source, value, DataType.fromJava(value));
230+
var dataType = DataType.fromJava(value);
231+
if (value instanceof String) {
232+
value = BytesRefs.toBytesRef(value);
233+
}
234+
return new Literal(source, value, dataType);
230235
}
231236

232237
public static ReferenceAttribute referenceAttribute(String name, DataType type) {

‎x-pack/plugin/esql/qa/testFixtures/src/main/resources/ip.csv-spec‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -409,7 +409,7 @@ eth0 |gamma |fe80::cae2:65ff:fece:feb9
409409
lo0 |gamma |fe80::cae2:65ff:fece:feb9 |fe81::cae2:65ff:fece:feb9
410410
;
411411

412-
cdirMatchEqualsInsOrsIPs
412+
cidrMatchEqualsInsOrsIPs
413413
required_capability: combine_disjunctive_cidrmatches
414414

415415
FROM hosts

‎x-pack/plugin/esql/qa/testFixtures/src/main/resources/match-function.csv-spec‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -860,3 +860,21 @@ c: long | scalerank: long
860860
10 | 3
861861
15 | 2
862862
;
863+
864+
865+
testMatchWithReplace
866+
required_capability: match_function
867+
required_capability: no_plain_strings_in_literals
868+
from books
869+
| keep book_no, author
870+
| where match(author, REPLACE("FaulkneX", "X", "r"))
871+
| sort book_no
872+
| limit 5;
873+
874+
book_no:keyword | author:text
875+
2378 | [Carol Faulkner, Holly Byers Ochoa, Lucretia Mott]
876+
2713 | William Faulkner
877+
2847 | Colleen Faulkner
878+
2883 | William Faulkner
879+
3293 | Danny Faulkner
880+
;

‎x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -979,7 +979,15 @@ public enum Cap {
979979
/**
980980
* Support parameters for SAMPLE command.
981981
*/
982-
PARAMETER_FOR_SAMPLE;
982+
PARAMETER_FOR_SAMPLE,
983+
984+
/**
985+
* From now, Literal only accepts strings as BytesRefs.
986+
* No java.lang.String anymore.
987+
*
988+
* https://github.com/elastic/elasticsearch/issues/129322
989+
*/
990+
NO_PLAIN_STRINGS_IN_LITERALS;
983991

984992
private final boolean enabled;
985993

‎x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/Analyzer.java‎

Lines changed: 13 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,8 @@
88
package org.elasticsearch.xpack.esql.analysis;
99

1010
import org.elasticsearch.common.logging.HeaderWarning;
11+
import org.elasticsearch.common.logging.LoggerMessageFormat;
12+
import org.elasticsearch.common.lucene.BytesRefs;
1113
import org.elasticsearch.compute.data.Block;
1214
import org.elasticsearch.index.IndexMode;
1315
import org.elasticsearch.logging.Logger;
@@ -310,7 +312,7 @@ protected LogicalPlan rule(Enrich plan, AnalyzerContext context) {
310312
// the policy does not exist
311313
return plan;
312314
}
313-
final String policyName = (String) plan.policyName().fold(FoldContext.small() /* TODO remove me */);
315+
final String policyName = BytesRefs.toString(plan.policyName().fold(FoldContext.small() /* TODO remove me */));
314316
final var resolved = context.enrichResolution().getResolvedPolicy(policyName, plan.mode());
315317
if (resolved != null) {
316318
var policy = new EnrichPolicy(resolved.matchType(), null, List.of(), resolved.matchField(), resolved.enrichFields());
@@ -393,7 +395,7 @@ private static class ResolveInference extends ParameterizedAnalyzerRule<Inferenc
393395
protected LogicalPlan rule(InferencePlan<?> plan, AnalyzerContext context) {
394396
assert plan.inferenceId().resolved() && plan.inferenceId().foldable();
395397

396-
String inferenceId = plan.inferenceId().fold(FoldContext.small()).toString();
398+
String inferenceId = BytesRefs.toString(plan.inferenceId().fold(FoldContext.small()));
397399
ResolvedInference resolvedInference = context.inferenceResolution().getResolvedInference(inferenceId);
398400

399401
if (resolvedInference != null && resolvedInference.taskType() == plan.taskType()) {
@@ -423,7 +425,7 @@ protected LogicalPlan rule(Lookup lookup, AnalyzerContext context) {
423425
// the parser passes the string wrapped in a literal
424426
Source source = lookup.source();
425427
Expression tableNameExpression = lookup.tableName();
426-
String tableName = lookup.tableName().toString();
428+
String tableName = BytesRefs.toString(tableNameExpression.fold(FoldContext.small() /* TODO remove me */));
427429
Map<String, Map<String, Column>> tables = context.configuration().tables();
428430
LocalRelation localRelation = null;
429431

@@ -1361,18 +1363,22 @@ private static boolean supportsStringImplicitCasting(DataType type) {
13611363
}
13621364

13631365
private static UnresolvedAttribute unresolvedAttribute(Expression value, String type, Exception e) {
1364-
String message = format(
1366+
String name = BytesRefs.toString(value.fold(FoldContext.small()) /* TODO remove me */);
1367+
String message = LoggerMessageFormat.format(
1368+
null,
13651369
"Cannot convert string [{}] to [{}], error [{}]",
1366-
value.fold(FoldContext.small() /* TODO remove me */),
1370+
name,
13671371
type,
13681372
(e instanceof ParsingException pe) ? pe.getErrorMessage() : e.getMessage()
13691373
);
1370-
return new UnresolvedAttribute(value.source(), String.valueOf(value.fold(FoldContext.small() /* TODO remove me */)), message);
1374+
return new UnresolvedAttribute(value.source(), name, message);
13711375
}
13721376

13731377
private static Expression castStringLiteralToTemporalAmount(Expression from) {
13741378
try {
1375-
TemporalAmount result = maybeParseTemporalAmount(from.fold(FoldContext.small() /* TODO remove me */).toString().strip());
1379+
TemporalAmount result = maybeParseTemporalAmount(
1380+
BytesRefs.toString(from.fold(FoldContext.small() /* TODO remove me */)).strip()
1381+
);
13761382
if (result == null) {
13771383
return from;
13781384
}

0 commit comments

Comments
 (0)