Skip to content

Commit b83ab0f

Browse files
ivanceaivancea
authored
ESQL: Add ser/deser to functions tests (#122498) (#122820)
# Why and what? First part of #122588 Some functions don't serialize their Source. This sometimes makes them emit wrong warnings, with -1:-1 line and without the correct source text. This PR pretends to integrate ser/deserialization before executing some (randomly chosen) functions, as well as using a non-empty Source to build them. _Note:_ It's different from the SerializationTests: Here we check that, whether serialized or not, the functionality is identical
1 parent 1370f6e commit b83ab0f

Some content is hidden

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

49 files changed

+337
-214
lines changed

‎x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/tree/Source.java‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -105,6 +105,10 @@ public String toString() {
105105
return text + location;
106106
}
107107

108+
/**
109+
* @deprecated Sources created by this can't be correctly deserialized. For use in tests only.
110+
*/
111+
@Deprecated
108112
public static Source synthetic(String text) {
109113
return new Source(Location.EMPTY, text);
110114
}

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

Lines changed: 41 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,7 @@
6464
import org.elasticsearch.xpack.esql.expression.predicate.operator.comparison.LessThan;
6565
import org.elasticsearch.xpack.esql.expression.predicate.operator.comparison.LessThanOrEqual;
6666
import org.elasticsearch.xpack.esql.expression.predicate.operator.comparison.NotEquals;
67+
import org.elasticsearch.xpack.esql.io.stream.PlanStreamOutput;
6768
import org.elasticsearch.xpack.esql.optimizer.rules.logical.FoldNull;
6869
import org.elasticsearch.xpack.esql.parser.ExpressionBuilder;
6970
import org.elasticsearch.xpack.esql.planner.Layout;
@@ -102,6 +103,7 @@
102103
import static org.elasticsearch.xpack.esql.EsqlTestUtils.randomLiteral;
103104
import static org.elasticsearch.xpack.esql.EsqlTestUtils.unboundLogicalOptimizerContext;
104105
import static org.elasticsearch.xpack.esql.SerializationTestUtils.assertSerialization;
106+
import static org.elasticsearch.xpack.esql.SerializationTestUtils.serializeDeserialize;
105107
import static org.elasticsearch.xpack.esql.expression.function.EsqlFunctionRegistry.mapParam;
106108
import static org.elasticsearch.xpack.esql.expression.function.EsqlFunctionRegistry.param;
107109
import static org.elasticsearch.xpack.esql.expression.function.EsqlFunctionRegistry.paramWithoutAnnotation;
@@ -331,7 +333,7 @@ protected static String typeErrorMessage(
331333
String ordinal = includeOrdinal ? TypeResolutions.ParamOrdinal.fromIndex(badArgPosition).name().toLowerCase(Locale.ROOT) + " " : "";
332334
String expectedTypeString = expectedTypeSupplier.apply(validPerPosition.get(badArgPosition), badArgPosition);
333335
String name = types.get(badArgPosition).typeName();
334-
return ordinal + "argument of [] must be [" + expectedTypeString + "], found value [" + name + "] type [" + name + "]";
336+
return ordinal + "argument of [source] must be [" + expectedTypeString + "], found value [" + name + "] type [" + name + "]";
335337
}
336338

337339
@FunctionalInterface
@@ -522,7 +524,7 @@ public static Expression deepCopyOfField(String name, DataType type) {
522524
* <strong>except</strong> those that have been marked with {@link TestCaseSupplier.TypedData#forceLiteral()}.
523525
*/
524526
protected final Expression buildFieldExpression(TestCaseSupplier.TestCase testCase) {
525-
return build(testCase.getSource(), testCase.getDataAsFields());
527+
return randomSerializeDeserialize(build(testCase.getSource(), testCase.getDataAsFields()));
526528
}
527529

528530
/**
@@ -531,12 +533,47 @@ protected final Expression buildFieldExpression(TestCaseSupplier.TestCase testCa
531533
* those that have been marked with {@link TestCaseSupplier.TypedData#forceLiteral()}.
532534
*/
533535
protected final Expression buildDeepCopyOfFieldExpression(TestCaseSupplier.TestCase testCase) {
536+
// We don't use `randomSerializeDeserialize()` here as the deep copied fields aren't deserializable right now
534537
return build(testCase.getSource(), testCase.getDataAsDeepCopiedFields());
535538
}
536539

540+
private Expression randomSerializeDeserialize(Expression expression) {
541+
if (randomBoolean()) {
542+
return expression;
543+
}
544+
545+
return serializeDeserializeExpression(expression);
546+
}
547+
548+
/**
549+
* Returns the expression after being serialized and deserialized.
550+
* <p>
551+
* Tests randomly go through this method to ensure that the function retains the same logic after serialization and deserialization.
552+
* </p>
553+
* <p>
554+
* Can be overridden to provide custom serialization and deserialization logic, or disable it if needed.
555+
* </p>
556+
*/
557+
protected Expression serializeDeserializeExpression(Expression expression) {
558+
Expression newExpression = serializeDeserialize(
559+
expression,
560+
PlanStreamOutput::writeNamedWriteable,
561+
in -> in.readNamedWriteable(Expression.class),
562+
testCase.getConfiguration() // The configuration query should be == to the source text of the function for this to work
563+
);
564+
565+
// Fields use synthetic sources, which can't be serialized. So we replace with the originals instead.
566+
var dummyChildren = newExpression.children()
567+
.stream()
568+
.<Expression>map(c -> new Literal(Source.EMPTY, "anything that won't match any test case", c.dataType()))
569+
.toList();
570+
// We first replace them with other unrelated expressions to force a replace, as some replaceChildren() will check for equality
571+
return newExpression.replaceChildrenSameSize(dummyChildren).replaceChildrenSameSize(expression.children());
572+
}
573+
537574
protected final Expression buildLiteralExpression(TestCaseSupplier.TestCase testCase) {
538575
assumeTrue("Data can't be converted to literals", testCase.canGetDataAsLiterals());
539-
return build(testCase.getSource(), testCase.getDataAsLiterals());
576+
return randomSerializeDeserialize(build(testCase.getSource(), testCase.getDataAsLiterals()));
540577
}
541578

542579
public static EvaluatorMapper.ToEvaluator toEvaluator() {
@@ -711,7 +748,7 @@ private static BytesRef randomizeBytesRefOffset(BytesRef bytesRef) {
711748
}
712749

713750
public void testSerializationOfSimple() {
714-
assertSerialization(buildFieldExpression(testCase));
751+
assertSerialization(buildFieldExpression(testCase), testCase.getConfiguration());
715752
}
716753

717754
/**

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -401,8 +401,8 @@ protected static TestCaseSupplier arithmeticExceptionOverflowCase(
401401
evaluator + "[lhs=Attribute[channel=0], rhs=Attribute[channel=1]]",
402402
dataType,
403403
is(nullValue())
404-
).withWarning("Line -1:-1: evaluation of [] failed, treating result as null. Only first 20 failures recorded.")
405-
.withWarning("Line -1:-1: java.lang.ArithmeticException: " + typeNameOverflow)
404+
).withWarning("Line 1:1: evaluation of [source] failed, treating result as null. Only first 20 failures recorded.")
405+
.withWarning("Line 1:1: java.lang.ArithmeticException: " + typeNameOverflow)
406406
);
407407
}
408408
}

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

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,12 +16,15 @@
1616
import org.elasticsearch.logging.LogManager;
1717
import org.elasticsearch.logging.Logger;
1818
import org.elasticsearch.test.ESTestCase;
19+
import org.elasticsearch.xpack.esql.EsqlTestUtils;
1920
import org.elasticsearch.xpack.esql.core.expression.Expression;
2021
import org.elasticsearch.xpack.esql.core.expression.Literal;
2122
import org.elasticsearch.xpack.esql.core.expression.MapExpression;
23+
import org.elasticsearch.xpack.esql.core.tree.Location;
2224
import org.elasticsearch.xpack.esql.core.tree.Source;
2325
import org.elasticsearch.xpack.esql.core.type.DataType;
2426
import org.elasticsearch.xpack.esql.core.util.NumericUtils;
27+
import org.elasticsearch.xpack.esql.session.Configuration;
2528
import org.elasticsearch.xpack.versionfield.Version;
2629
import org.hamcrest.Matcher;
2730

@@ -54,6 +57,9 @@ public record TestCaseSupplier(String name, List<DataType> types, Supplier<TestC
5457
implements
5558
Supplier<TestCaseSupplier.TestCase> {
5659

60+
public static final Source TEST_SOURCE = new Source(new Location(1, 0), "source");
61+
public static final Configuration TEST_CONFIGURATION = EsqlTestUtils.configuration(TEST_SOURCE.text());
62+
5763
private static final Logger logger = LogManager.getLogger(TestCaseSupplier.class);
5864

5965
/**
@@ -1388,6 +1394,10 @@ public static final class TestCase {
13881394
* The {@link Source} this test case should be run with
13891395
*/
13901396
private final Source source;
1397+
/**
1398+
* The {@link Configuration} this test case should use
1399+
*/
1400+
private final Configuration configuration;
13911401
/**
13921402
* The parameter values and types to pass into the function for this test run
13931403
*/
@@ -1490,7 +1500,8 @@ public static TestCase typeError(List<TypedData> data, String expectedTypeError)
14901500
Object extra,
14911501
boolean canBuildEvaluator
14921502
) {
1493-
this.source = Source.EMPTY;
1503+
this.source = TEST_SOURCE;
1504+
this.configuration = TEST_CONFIGURATION;
14941505
this.data = data;
14951506
this.evaluatorToString = evaluatorToString;
14961507
this.expectedType = expectedType == null ? null : expectedType.noText();
@@ -1510,6 +1521,10 @@ public Source getSource() {
15101521
return source;
15111522
}
15121523

1524+
public Configuration getConfiguration() {
1525+
return configuration;
1526+
}
1527+
15131528
public List<TypedData> getData() {
15141529
return data;
15151530
}

‎x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/aggregate/TopTests.java‎

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -241,7 +241,7 @@ public static Iterable<Object[]> parameters() {
241241
new TestCaseSupplier.TypedData(0, DataType.INTEGER, "limit").forceLiteral(),
242242
new TestCaseSupplier.TypedData(new BytesRef("desc"), DataType.KEYWORD, "order").forceLiteral()
243243
),
244-
"Limit must be greater than 0 in [], found [0]"
244+
"Limit must be greater than 0 in [source], found [0]"
245245
)
246246
),
247247
new TestCaseSupplier(
@@ -252,7 +252,7 @@ public static Iterable<Object[]> parameters() {
252252
new TestCaseSupplier.TypedData(2, DataType.INTEGER, "limit").forceLiteral(),
253253
new TestCaseSupplier.TypedData(new BytesRef("wrong-order"), DataType.KEYWORD, "order").forceLiteral()
254254
),
255-
"Invalid order value in [], expected [ASC, DESC] but got [wrong-order]"
255+
"Invalid order value in [source], expected [ASC, DESC] but got [wrong-order]"
256256
)
257257
),
258258
new TestCaseSupplier(
@@ -263,7 +263,7 @@ public static Iterable<Object[]> parameters() {
263263
new TestCaseSupplier.TypedData(null, DataType.INTEGER, "limit").forceLiteral(),
264264
new TestCaseSupplier.TypedData(new BytesRef("desc"), DataType.KEYWORD, "order").forceLiteral()
265265
),
266-
"second argument of [] cannot be null, received [limit]"
266+
"second argument of [source] cannot be null, received [limit]"
267267
)
268268
),
269269
new TestCaseSupplier(
@@ -274,7 +274,7 @@ public static Iterable<Object[]> parameters() {
274274
new TestCaseSupplier.TypedData(1, DataType.INTEGER, "limit").forceLiteral(),
275275
new TestCaseSupplier.TypedData(null, DataType.KEYWORD, "order").forceLiteral()
276276
),
277-
"third argument of [] cannot be null, received [order]"
277+
"third argument of [source] cannot be null, received [order]"
278278
)
279279
)
280280
)
@@ -317,4 +317,10 @@ private static TestCaseSupplier makeSupplier(
317317
);
318318
});
319319
}
320+
321+
@Override
322+
protected Expression serializeDeserializeExpression(Expression expression) {
323+
// TODO: This aggregation doesn't serialize the Source, and must be fixed.
324+
return expression;
325+
}
320326
}

‎x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/fulltext/MatchTests.java‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,11 +19,13 @@
1919
import org.elasticsearch.xpack.esql.core.type.DataType;
2020
import org.elasticsearch.xpack.esql.expression.function.FunctionName;
2121
import org.elasticsearch.xpack.esql.expression.function.TestCaseSupplier;
22+
import org.elasticsearch.xpack.esql.io.stream.PlanStreamOutput;
2223

2324
import java.util.ArrayList;
2425
import java.util.List;
2526
import java.util.function.Supplier;
2627

28+
import static org.elasticsearch.xpack.esql.SerializationTestUtils.serializeDeserialize;
2729
import static org.elasticsearch.xpack.esql.core.type.DataType.BOOLEAN;
2830
import static org.elasticsearch.xpack.esql.core.type.DataType.KEYWORD;
2931
import static org.elasticsearch.xpack.esql.core.type.DataType.UNSUPPORTED;
@@ -83,4 +85,19 @@ protected Expression build(Source source, List<Expression> args) {
8385
}
8486
return match;
8587
}
88+
89+
/**
90+
* Copy of the overridden method that doesn't check for children size, as the {@code options} child isn't serialized in Match.
91+
*/
92+
@Override
93+
protected Expression serializeDeserializeExpression(Expression expression) {
94+
Expression newExpression = serializeDeserialize(
95+
expression,
96+
PlanStreamOutput::writeNamedWriteable,
97+
in -> in.readNamedWriteable(Expression.class),
98+
testCase.getConfiguration() // The configuration query should be == to the source text of the function for this to work
99+
);
100+
// Fields use synthetic sources, which can't be serialized. So we use the originals instead.
101+
return newExpression.replaceChildren(expression.children());
102+
}
86103
}

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

Lines changed: 14 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@
88
package org.elasticsearch.xpack.esql.expression.function.scalar;
99

1010
import org.elasticsearch.common.settings.Settings;
11-
import org.elasticsearch.xpack.esql.EsqlTestUtils;
1211
import org.elasticsearch.xpack.esql.core.expression.Expression;
1312
import org.elasticsearch.xpack.esql.core.tree.Source;
1413
import org.elasticsearch.xpack.esql.core.util.StringUtils;
@@ -27,10 +26,22 @@ public abstract class AbstractConfigurationFunctionTestCase extends AbstractScal
2726

2827
@Override
2928
protected Expression build(Source source, List<Expression> args) {
30-
return buildWithConfiguration(source, args, EsqlTestUtils.TEST_CFG);
29+
return buildWithConfiguration(source, args, testCase.getConfiguration());
3130
}
3231

33-
static Configuration randomConfiguration() {
32+
public void testSerializationWithConfiguration() {
33+
Configuration config = randomConfiguration();
34+
Expression expr = buildWithConfiguration(testCase.getSource(), testCase.getDataAsFields(), config);
35+
36+
assertSerialization(expr, config);
37+
38+
Configuration differentConfig = randomValueOtherThan(config, AbstractConfigurationFunctionTestCase::randomConfiguration);
39+
40+
Expression differentExpr = buildWithConfiguration(testCase.getSource(), testCase.getDataAsFields(), differentConfig);
41+
assertNotEquals(expr, differentExpr);
42+
}
43+
44+
private static Configuration randomConfiguration() {
3445
// TODO: Randomize the query and maybe the pragmas.
3546
return new Configuration(
3647
randomZone(),
@@ -46,19 +57,4 @@ static Configuration randomConfiguration() {
4657
System.nanoTime()
4758
);
4859
}
49-
50-
public void testSerializationWithConfiguration() {
51-
Configuration config = randomConfiguration();
52-
Expression expr = buildWithConfiguration(testCase.getSource(), testCase.getDataAsFields(), config);
53-
54-
assertSerialization(expr, config);
55-
56-
Configuration differentConfig;
57-
do {
58-
differentConfig = randomConfiguration();
59-
} while (config.equals(differentConfig));
60-
61-
Expression differentExpr = buildWithConfiguration(testCase.getSource(), testCase.getDataAsFields(), differentConfig);
62-
assertFalse(expr.equals(differentExpr));
63-
}
6460
}

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

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -75,4 +75,10 @@ public static Iterable<Object[]> parameters() {
7575
)
7676
);
7777
}
78+
79+
@Override
80+
protected Expression serializeDeserializeExpression(Expression expression) {
81+
// AggregateMetricDoubleLiteral can't be serialized when it's a literal
82+
return expression;
83+
}
7884
}

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -49,8 +49,8 @@ public static Iterable<Object[]> parameters() {
4949
bytesRef -> {
5050
var exception = expectThrows(Exception.class, () -> CARTESIAN.wktToWkb(bytesRef.utf8ToString()));
5151
return List.of(
52-
"Line -1:-1: evaluation of [] failed, treating result as null. Only first 20 failures recorded.",
53-
"Line -1:-1: " + exception
52+
"Line 1:1: evaluation of [source] failed, treating result as null. Only first 20 failures recorded.",
53+
"Line 1:1: " + exception
5454
);
5555
}
5656
);

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -50,8 +50,8 @@ public static Iterable<Object[]> parameters() {
5050
bytesRef -> {
5151
var exception = expectThrows(Exception.class, () -> CARTESIAN.wktToWkb(bytesRef.utf8ToString()));
5252
return List.of(
53-
"Line -1:-1: evaluation of [] failed, treating result as null. Only first 20 failures recorded.",
54-
"Line -1:-1: " + exception
53+
"Line 1:1: evaluation of [source] failed, treating result as null. Only first 20 failures recorded.",
54+
"Line 1:1: " + exception
5555
);
5656
}
5757
);

0 commit comments

Comments
 (0)