ESQL: Add ser/deser to functions tests #122498

ivancea · 2025-02-13T12:03:03Z

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

To-Do

Make the source "realistic", using the fields names, so they can also be deserialized. This may be more complicated, as fields currently use a synthetic source. May be discarded or moved to another PR
From a quick investigation, some functions don't serialize the source but emit warnings. ESQL: Fix functions emitting warnings with no source #122821

…sabling serialization on broken functions

ivancea · 2025-02-13T15:33:20Z

.../java/org/elasticsearch/xpack/esql/core/expression/function/scalar/BinaryScalarFunction.java

        Expression newRight = newChildren.get(1);

-        return left.equals(newLeft) && right.equals(newRight) ? this : replaceChildren(newLeft, newRight);
+        return replaceChildren(newLeft, newRight);


I'm not sure what to think about this, I may try to do... something around it, as the test code currently needs replaceChildren() to actually replace them even if only the Source changed.
This is the only ESQL expression I found doing this, so I need to check if this was made for some specific reason

I can do a expression.replaceChildren(dummies).replaceChildren(new). A bit weird, but it is what it is!

Finally replaced with that, and restored the original code here

ivancea · 2025-02-13T15:34:49Z

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

    }

+    /**
+     * @deprecated Sources created by this can't be correctly deserialized. For use in tests only.


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

elasticsearchmachine · 2025-02-13T17:18:37Z

Pinging @elastic/es-analytical-engine (Team:Analytics)

nik9000

I think it's a good idea.

nik9000 · 2025-02-13T21:32:02Z

...src/test/java/org/elasticsearch/xpack/esql/expression/function/AbstractFunctionTestCase.java

    }

+    private Expression randomSerializeDeserialize(Expression expression) {
+        if (false && randomBoolean()) {


false && maybe a leftover

nik9000 · 2025-02-13T21:32:42Z

...src/test/java/org/elasticsearch/xpack/esql/expression/function/AbstractFunctionTestCase.java

     */
    protected final Expression buildFieldExpression(TestCaseSupplier.TestCase testCase) {
-        return build(testCase.getSource(), testCase.getDataAsFields());
+        return randomSerializeDeserialize(build(testCase.getSource(), testCase.getDataAsFields()));


I see! So you are asserting that everything works the same after a round trip of serialization/deserialization. That sounds nice.

nik9000 · 2025-02-13T21:33:23Z

...in/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/TestCaseSupplier.java

    implements
        Supplier<TestCaseSupplier.TestCase> {

+    public static final Source TEST_SOURCE = new Source(new Location(1, 0), "source");


Do you think it'd be better to randomize this in the places we call it?

Actually probably not - we want it to be consistent in the error message.

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.
There's some minor problems I have to handle first to do that, so I'll try in another PR

nik9000 · 2025-02-13T21:35:06Z

.../esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/aggregate/TopTests.java

                            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]"


I wonder if source should be TOP or something. source feels sort of surprising here even though it's easy to make it the test.

You could call functionName() to get the function I think.

It's currently a static Source. Making it dynamic is possible, but requires also changing the Configuration.
As commented in other thread here, I'll try that later, but it requires some extra refactor, and I feel like it's better in a separate PR.
I thoguht about changing that "source" to something more localizable like "$source$" or even "function()". But at this point, I'm not sure it adds much

nik9000 · 2025-02-13T21:37:30Z

...in/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/TestCaseSupplier.java

        Supplier<TestCaseSupplier.TestCase> {

+    public static final Source TEST_SOURCE = new Source(new Location(1, 0), "source");
+    public static final Configuration TEST_CONFIGURATION = EsqlTestUtils.configuration(TEST_SOURCE.text());


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.

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

idegtiarenko · 2025-02-14T07:19:23Z

.../esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/aggregate/TopTests.java

+
+    @Override
+    protected Expression serializeDeserializeExpression(Expression expression) {
+        // TODO: This aggregation doesn't serialize the Source, and must be fixed.


It would be really nice to open an issue about it and reference it here from the todo.
This way we could track its progress and know if todos are resolved or accidentally left after the feature was implemented.

Sure! That's the issue I was tracking actually, so I should have had one to begin with :hehe:

idegtiarenko · 2025-02-14T07:21:07Z

...asticsearch/xpack/esql/expression/function/scalar/AbstractConfigurationFunctionTestCase.java

+        Configuration differentConfig;
+        do {
+            differentConfig = randomConfiguration();
+        } while (config.equals(differentConfig));


NIT: This could be simplified with Configuration differentConfig = randomValueOtherThan(config, () -> randomConfiguration());

…e some code

elasticsearchmachine · 2025-02-18T10:48:50Z

💚 Backport successful

Status	Branch	Result
✅	8.18
✅	9.0

# Why and what? First part of elastic#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

# 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

Manual 8.x backport of #122498

ivancea added 2 commits February 12, 2025 14:27

Use ser/deserialized functions in function tests, only Match failing

d8cdf36

Fixed Match

d277fd1

ivancea added >test Issues or PRs that are addressing/adding tests Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) :Analytics/ES|QL AKA ESQL v9.1.0 labels Feb 13, 2025

elasticsearchmachine and others added 4 commits February 13, 2025 12:13

[CI] Auto commit changes from spotless

1afc3ea

Use the real Source, and implement for DateDiff

1b26239

Updated tests to use the non-empty source, fixing the warnings and di…

b8f315b

…sabling serialization on broken functions

[CI] Auto commit changes from spotless

5350252

ivancea commented Feb 13, 2025

View reviewed changes

ivancea and others added 2 commits February 13, 2025 16:42

Merge branch 'main' into esql-source-functions-warnings

0419e9d

Fixed RepeatStaticTests

2f48a59

ivancea marked this pull request as ready for review February 13, 2025 17:18

nik9000 approved these changes Feb 13, 2025

View reviewed changes

idegtiarenko reviewed Feb 14, 2025

View reviewed changes

Restore replaceChildren(), fix condition on serialization, and improv…

a8f1698

…e some code

ivancea requested a review from idegtiarenko February 14, 2025 12:46

ivancea added auto-backport Automatically create backport pull requests when merged v8.18.0 v9.0.1 labels Feb 14, 2025

ivancea added 2 commits February 14, 2025 16:09

Merge branch 'main' into esql-source-functions-warnings

a65009f

Merge branch 'main' into esql-source-functions-warnings

5a30e5d

idegtiarenko approved these changes Feb 17, 2025

View reviewed changes

Merge branch 'main' into esql-source-functions-warnings

7e94e73

ivancea merged commit a1b16d9 into elastic:main Feb 18, 2025
17 checks passed

ivancea deleted the esql-source-functions-warnings branch February 18, 2025 10:47

This was referenced Feb 18, 2025

[8.18] ESQL: Add ser/deser to functions tests (#122498) #122819

Closed

[9.0] ESQL: Add ser/deser to functions tests (#122498) #122820

Merged

ivancea removed the v8.18.0 label Feb 18, 2025

ivancea mentioned this pull request Feb 18, 2025

[8.x] ESQL: Add ser/deser to functions tests (#122498) #122827

Merged

ivancea added the v8.19.0 label Feb 18, 2025

ivancea added a commit that referenced this pull request Feb 18, 2025

ESQL: Add ser/deser to functions tests (#122498) (#122827)

4ae8210

Manual 8.x backport of #122498

ESQL: Add ser/deser to functions tests #122498

ESQL: Add ser/deser to functions tests #122498

Uh oh!

Conversation

ivancea commented Feb 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why and what?

To-Do

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticsearchmachine commented Feb 13, 2025

Uh oh!

nik9000 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

elasticsearchmachine commented Feb 18, 2025

💚 Backport successful

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ivancea commented Feb 13, 2025 •

edited

Loading