-
Notifications
You must be signed in to change notification settings - Fork 25.6k
ESQL: Add ser/deser to functions tests #122498
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
d8cdf36
d277fd1
1afc3ea
1b26239
b8f315b
5350252
0419e9d
2f48a59
a8f1698
a65009f
5a30e5d
7e94e73
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -64,6 +64,7 @@ | |
| import org.elasticsearch.xpack.esql.expression.predicate.operator.comparison.LessThan; | ||
| import org.elasticsearch.xpack.esql.expression.predicate.operator.comparison.LessThanOrEqual; | ||
| import org.elasticsearch.xpack.esql.expression.predicate.operator.comparison.NotEquals; | ||
| import org.elasticsearch.xpack.esql.io.stream.PlanStreamOutput; | ||
| import org.elasticsearch.xpack.esql.optimizer.rules.logical.FoldNull; | ||
| import org.elasticsearch.xpack.esql.parser.ExpressionBuilder; | ||
| import org.elasticsearch.xpack.esql.planner.Layout; | ||
|
|
@@ -102,6 +103,7 @@ | |
| import static org.elasticsearch.xpack.esql.EsqlTestUtils.randomLiteral; | ||
| import static org.elasticsearch.xpack.esql.EsqlTestUtils.unboundLogicalOptimizerContext; | ||
| import static org.elasticsearch.xpack.esql.SerializationTestUtils.assertSerialization; | ||
| import static org.elasticsearch.xpack.esql.SerializationTestUtils.serializeDeserialize; | ||
| import static org.elasticsearch.xpack.esql.expression.function.EsqlFunctionRegistry.mapParam; | ||
| import static org.elasticsearch.xpack.esql.expression.function.EsqlFunctionRegistry.param; | ||
| import static org.elasticsearch.xpack.esql.expression.function.EsqlFunctionRegistry.paramWithoutAnnotation; | ||
|
|
@@ -331,7 +333,7 @@ protected static String typeErrorMessage( | |
| String ordinal = includeOrdinal ? TypeResolutions.ParamOrdinal.fromIndex(badArgPosition).name().toLowerCase(Locale.ROOT) + " " : ""; | ||
| String expectedTypeString = expectedTypeSupplier.apply(validPerPosition.get(badArgPosition), badArgPosition); | ||
| String name = types.get(badArgPosition).typeName(); | ||
| return ordinal + "argument of [] must be [" + expectedTypeString + "], found value [" + name + "] type [" + name + "]"; | ||
| return ordinal + "argument of [source] must be [" + expectedTypeString + "], found value [" + name + "] type [" + name + "]"; | ||
| } | ||
|
|
||
| @FunctionalInterface | ||
|
|
@@ -522,7 +524,7 @@ public static Expression deepCopyOfField(String name, DataType type) { | |
| * <strong>except</strong> those that have been marked with {@link TestCaseSupplier.TypedData#forceLiteral()}. | ||
| */ | ||
| protected final Expression buildFieldExpression(TestCaseSupplier.TestCase testCase) { | ||
| return build(testCase.getSource(), testCase.getDataAsFields()); | ||
| return randomSerializeDeserialize(build(testCase.getSource(), testCase.getDataAsFields())); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see! So you are asserting that everything works the same after a round trip of serialization/deserialization. That sounds nice. |
||
| } | ||
|
|
||
| /** | ||
|
|
@@ -531,12 +533,47 @@ protected final Expression buildFieldExpression(TestCaseSupplier.TestCase testCa | |
| * those that have been marked with {@link TestCaseSupplier.TypedData#forceLiteral()}. | ||
| */ | ||
| protected final Expression buildDeepCopyOfFieldExpression(TestCaseSupplier.TestCase testCase) { | ||
| // We don't use `randomSerializeDeserialize()` here as the deep copied fields aren't deserializable right now | ||
| return build(testCase.getSource(), testCase.getDataAsDeepCopiedFields()); | ||
| } | ||
|
|
||
| private Expression randomSerializeDeserialize(Expression expression) { | ||
| if (randomBoolean()) { | ||
| return expression; | ||
| } | ||
|
|
||
| return serializeDeserializeExpression(expression); | ||
| } | ||
|
|
||
| /** | ||
| * Returns the expression after being serialized and deserialized. | ||
| * <p> | ||
| * Tests randomly go through this method to ensure that the function retains the same logic after serialization and deserialization. | ||
| * </p> | ||
| * <p> | ||
| * Can be overridden to provide custom serialization and deserialization logic, or disable it if needed. | ||
| * </p> | ||
| */ | ||
| protected Expression serializeDeserializeExpression(Expression expression) { | ||
| Expression newExpression = serializeDeserialize( | ||
| expression, | ||
| PlanStreamOutput::writeNamedWriteable, | ||
| in -> in.readNamedWriteable(Expression.class), | ||
| testCase.getConfiguration() // The configuration query should be == to the source text of the function for this to work | ||
| ); | ||
|
|
||
| // Fields use synthetic sources, which can't be serialized. So we replace with the originals instead. | ||
| var dummyChildren = newExpression.children() | ||
| .stream() | ||
| .<Expression>map(c -> new Literal(Source.EMPTY, "anything that won't match any test case", c.dataType())) | ||
| .toList(); | ||
| // We first replace them with other unrelated expressions to force a replace, as some replaceChildren() will check for equality | ||
| return newExpression.replaceChildrenSameSize(dummyChildren).replaceChildrenSameSize(expression.children()); | ||
| } | ||
|
|
||
| protected final Expression buildLiteralExpression(TestCaseSupplier.TestCase testCase) { | ||
| assumeTrue("Data can't be converted to literals", testCase.canGetDataAsLiterals()); | ||
| return build(testCase.getSource(), testCase.getDataAsLiterals()); | ||
| return randomSerializeDeserialize(build(testCase.getSource(), testCase.getDataAsLiterals())); | ||
| } | ||
|
|
||
| public static EvaluatorMapper.ToEvaluator toEvaluator() { | ||
|
|
@@ -711,7 +748,7 @@ private static BytesRef randomizeBytesRefOffset(BytesRef bytesRef) { | |
| } | ||
|
|
||
| public void testSerializationOfSimple() { | ||
| assertSerialization(buildFieldExpression(testCase)); | ||
| assertSerialization(buildFieldExpression(testCase), testCase.getConfiguration()); | ||
| } | ||
|
|
||
| /** | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,12 +16,15 @@ | |
| import org.elasticsearch.logging.LogManager; | ||
| import org.elasticsearch.logging.Logger; | ||
| import org.elasticsearch.test.ESTestCase; | ||
| import org.elasticsearch.xpack.esql.EsqlTestUtils; | ||
| import org.elasticsearch.xpack.esql.core.expression.Expression; | ||
| import org.elasticsearch.xpack.esql.core.expression.Literal; | ||
| import org.elasticsearch.xpack.esql.core.expression.MapExpression; | ||
| import org.elasticsearch.xpack.esql.core.tree.Location; | ||
| import org.elasticsearch.xpack.esql.core.tree.Source; | ||
| import org.elasticsearch.xpack.esql.core.type.DataType; | ||
| import org.elasticsearch.xpack.esql.core.util.NumericUtils; | ||
| import org.elasticsearch.xpack.esql.session.Configuration; | ||
| import org.elasticsearch.xpack.versionfield.Version; | ||
| import org.hamcrest.Matcher; | ||
|
|
||
|
|
@@ -54,6 +57,9 @@ public record TestCaseSupplier(String name, List<DataType> types, Supplier<TestC | |
| implements | ||
| Supplier<TestCaseSupplier.TestCase> { | ||
|
|
||
| public static final Source TEST_SOURCE = new Source(new Location(1, 0), "source"); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you think it'd be better to randomize this in the places we call it?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually probably not - we want it to be consistent in the error message.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would like to autogenerate it, so we can also deserialize the fields with a real source, with real names, and remove synthetic Sources from tests if possible. |
||
| public static final Configuration TEST_CONFIGURATION = EsqlTestUtils.configuration(TEST_SOURCE.text()); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Warning - these are very slow to randomly generate. What you are doing is fine here - but a while back I had some tests that generated a random configuration in a loop - and, like 1000 or them took some time. Anyway, you shouldn't have a problem with this one.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. At least with what we have here, I didn't see a notable difference in times. I'll consider if I try to generate a realistic source, as the config query would have to change per-test. Something to check if I open that other PR |
||
|
|
||
| private static final Logger logger = LogManager.getLogger(TestCaseSupplier.class); | ||
|
|
||
| /** | ||
|
|
@@ -1388,6 +1394,10 @@ public static final class TestCase { | |
| * The {@link Source} this test case should be run with | ||
| */ | ||
| private final Source source; | ||
| /** | ||
| * The {@link Configuration} this test case should use | ||
| */ | ||
| private final Configuration configuration; | ||
| /** | ||
| * The parameter values and types to pass into the function for this test run | ||
| */ | ||
|
|
@@ -1490,7 +1500,8 @@ public static TestCase typeError(List<TypedData> data, String expectedTypeError) | |
| Object extra, | ||
| boolean canBuildEvaluator | ||
| ) { | ||
| this.source = Source.EMPTY; | ||
| this.source = TEST_SOURCE; | ||
| this.configuration = TEST_CONFIGURATION; | ||
| this.data = data; | ||
| this.evaluatorToString = evaluatorToString; | ||
| this.expectedType = expectedType == null ? null : expectedType.noText(); | ||
|
|
@@ -1510,6 +1521,10 @@ public Source getSource() { | |
| return source; | ||
| } | ||
|
|
||
| public Configuration getConfiguration() { | ||
| return configuration; | ||
| } | ||
|
|
||
| public List<TypedData> getData() { | ||
| return data; | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -241,7 +241,7 @@ public static Iterable<Object[]> parameters() { | |
| new TestCaseSupplier.TypedData(0, DataType.INTEGER, "limit").forceLiteral(), | ||
| new TestCaseSupplier.TypedData(new BytesRef("desc"), DataType.KEYWORD, "order").forceLiteral() | ||
| ), | ||
| "Limit must be greater than 0 in [], found [0]" | ||
| "Limit must be greater than 0 in [source], found [0]" | ||
| ) | ||
| ), | ||
| new TestCaseSupplier( | ||
|
|
@@ -252,7 +252,7 @@ public static Iterable<Object[]> parameters() { | |
| new TestCaseSupplier.TypedData(2, DataType.INTEGER, "limit").forceLiteral(), | ||
| new TestCaseSupplier.TypedData(new BytesRef("wrong-order"), DataType.KEYWORD, "order").forceLiteral() | ||
| ), | ||
| "Invalid order value in [], expected [ASC, DESC] but got [wrong-order]" | ||
| "Invalid order value in [source], expected [ASC, DESC] but got [wrong-order]" | ||
| ) | ||
| ), | ||
| new TestCaseSupplier( | ||
|
|
@@ -263,7 +263,7 @@ public static Iterable<Object[]> parameters() { | |
| new TestCaseSupplier.TypedData(null, DataType.INTEGER, "limit").forceLiteral(), | ||
| new TestCaseSupplier.TypedData(new BytesRef("desc"), DataType.KEYWORD, "order").forceLiteral() | ||
| ), | ||
| "second argument of [] cannot be null, received [limit]" | ||
| "second argument of [source] cannot be null, received [limit]" | ||
| ) | ||
| ), | ||
| new TestCaseSupplier( | ||
|
|
@@ -274,7 +274,7 @@ public static Iterable<Object[]> parameters() { | |
| new TestCaseSupplier.TypedData(1, DataType.INTEGER, "limit").forceLiteral(), | ||
| new TestCaseSupplier.TypedData(null, DataType.KEYWORD, "order").forceLiteral() | ||
| ), | ||
| "third argument of [] cannot be null, received [order]" | ||
| "third argument of [source] cannot be null, received [order]" | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You could call
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's currently a static Source. Making it dynamic is possible, but requires also changing the Configuration. |
||
| ) | ||
| ) | ||
| ) | ||
|
|
@@ -317,4 +317,10 @@ private static TestCaseSupplier makeSupplier( | |
| ); | ||
| }); | ||
| } | ||
|
|
||
| @Override | ||
| protected Expression serializeDeserializeExpression(Expression expression) { | ||
| // TODO: This aggregation doesn't serialize the Source, and must be fixed. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would be really nice to open an issue about it and reference it here from the todo.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure! That's the issue I was tracking actually, so I should have had one to begin with :hehe: |
||
| return expression; | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if deprecated is the way, but this is used only in tests, and can't be serialized. So I preferred to make it explicit that this is dangerous