ESQL: Fix functions emitting warnings with no source by ivancea · Pull Request #122821 · elastic/elasticsearch

ivancea · 2025-02-18T10:57:46Z

Fixes #122588

Replaced Source.EMPTY.writeTo(out) to source().writeTo(out) in functions emitting warnings
- Did the same on all aggs, as Top emits an error on type resolution. This is not a bug, as type resolution errors should only happen in the coordinator. Another option would be changing Top to not generate that error there, and make it implement instead PostAnalysisVerificationAware
In some cases, we don't even serialize an empty source. So I had to add a new TransportVersion to do so
- As an special case, ToLower and ToUpper weren't serializing a source, but they don't emit warnings. As they were the only remaining functions not serializing the source, I added it there too

…sabling serialization on broken functions

…e some code

…rialization

…in the future if we need it

…ons-warnings-fix # Conflicts: # server/src/main/java/org/elasticsearch/TransportVersions.java

# Conflicts: # server/src/main/java/org/elasticsearch/TransportVersions.java

elasticsearchmachine · 2025-02-18T10:58:12Z

Hi @ivancea, I've created a changelog YAML for you.

ivancea · 2025-02-18T11:11:49Z

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

    @Override
    public TestCase get() {
        TestCase supplied = supplier.get();
+        if (types.size() != supplied.getData().size()) {


The for below wasn't checking the case where the TestCase data had more types than the supplier. This required fixing some cases in Top and Case

ivancea · 2025-02-18T11:12:57Z

...st/java/org/elasticsearch/xpack/esql/expression/function/scalar/multivalue/MvSliceTests.java

This was one of the affected functions by the "warnings but no source" problem, but it had no tests for them. So here I added them based on the function errors

elasticsearchmachine · 2025-02-18T14:59:21Z

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

bpintea · 2025-02-20T22:06:49Z

...test/java/org/elasticsearch/xpack/esql/expression/predicate/operator/comparison/InTests.java

+    /**
+     * Returns a list with N dataType1, followed by 1 dataType2.
+     */
+    private static List<DataType> makeTypes(DataType dataType1, DataType dataType2, int n) {


Optional:

Suggested change

private static List<DataType> makeTypes(DataType dataType1, DataType dataType2, int n) {

private static List<DataType> typesList(DataType dataType1, DataType dataType2, int n) {

astefan

I was expecting to see some changes in IT tests as well, meaning warning message that started having proper line:column numbers.
Do you know why the change in this PR is not "visible" in IT tests?

# Conflicts: # server/src/main/java/org/elasticsearch/TransportVersions.java

ivancea · 2025-02-21T13:53:58Z

@astefan From what I saw, there are no CSV tests with warnings for those functions. I'll add some cases + capability for them to double-check.
Unless by "IT tests" you mean other tests I'm not aware of

astefan · 2025-02-21T13:57:52Z

I don't know exactly what we have and what don't for this particular set of functionality this PR is addressing, thus my evasiveness. I am ok with CSV IT tests. Since this involves serializations and, implicitly, type resolution on the data nodes, make sure the queries used trigger this fix properly.

astefan

LGTM

# Conflicts: # server/src/main/java/org/elasticsearch/TransportVersions.java

ivancea and others added 18 commits February 12, 2025 14:27

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

d8cdf36

Fixed Match

d277fd1

[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

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

0419e9d

Fixed RepeatStaticTests

2f48a59

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

a8f1698

…e some code

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

a65009f

Add check on test supplier types

2643e57

Add TransportVersion and fix functions with warnings and no source se…

b99e5a2

…rialization

Transport the source on missing functions, to avoid creating new TPs …

95b7937

…in the future if we need it

Updated MvSlice with extra tests, and fixed source

a7ee11b

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

5a30e5d

Merge branch 'esql-source-functions-warnings' into esql-source-functi…

c5b83ff

…ons-warnings-fix # Conflicts: # server/src/main/java/org/elasticsearch/TransportVersions.java

Fixed CaseTests

d347c14

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

92d49bc

# Conflicts: # server/src/main/java/org/elasticsearch/TransportVersions.java

ivancea added >bug Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) :Analytics/ES|QL AKA ESQL v9.1.0 labels Feb 18, 2025

Update docs/changelog/122821.yaml

075d99e

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

e30e72f

ivancea commented Feb 18, 2025

View reviewed changes

ivancea added 3 commits February 18, 2025 13:33

Update serialization tests

65bb97b

Fix InTests

048693e

Lowercase test name

79b1c4a

ivancea mentioned this pull request Feb 18, 2025

ESQL: Add ser/deser to functions tests #122498

Merged

2 tasks

ivancea marked this pull request as ready for review February 18, 2025 14:58

ivancea requested review from astefan, bpintea and nik9000 February 19, 2025 11:14

bpintea reviewed Feb 20, 2025

View reviewed changes

bpintea approved these changes Feb 20, 2025

View reviewed changes

astefan reviewed Feb 21, 2025

View reviewed changes

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

59a2218

# Conflicts: # server/src/main/java/org/elasticsearch/TransportVersions.java

Improve InTests helper methods

e85bbc1

Added CSV tests for warnings

caca630

ivancea requested a review from astefan February 21, 2025 17:27

astefan approved these changes Feb 24, 2025

View reviewed changes

ivancea added 2 commits February 24, 2025 13:34

Fixed tests

ce980d1

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

ca37e8a

# Conflicts: # server/src/main/java/org/elasticsearch/TransportVersions.java

ivancea enabled auto-merge (squash) February 24, 2025 12:36

ivancea merged commit c40c5a6 into elastic:main Feb 24, 2025
17 checks passed

ivancea deleted the esql-source-functions-warnings-fix branch February 24, 2025 13:52

wchaparro mentioned this pull request May 23, 2025

ESQL: Warnings lose their line information in multi-node cluster #112566

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ESQL: Fix functions emitting warnings with no source#122821

ESQL: Fix functions emitting warnings with no source#122821
ivancea merged 28 commits intoelastic:mainfrom
ivancea:esql-source-functions-warnings-fix

ivancea commented Feb 18, 2025 •

edited

Loading

elasticsearchmachine commented Feb 18, 2025

ivancea Feb 18, 2025

ivancea Feb 18, 2025

elasticsearchmachine commented Feb 18, 2025

bpintea Feb 20, 2025

astefan left a comment

ivancea commented Feb 21, 2025

astefan commented Feb 21, 2025

astefan left a comment

Uh oh!

Labels

4 participants

	private static List<DataType> makeTypes(DataType dataType1, DataType dataType2, int n) {
	private static List<DataType> typesList(DataType dataType1, DataType dataType2, int n) {

Conversation

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

Uh oh!

elasticsearchmachine commented Feb 18, 2025

ivancea Feb 18, 2025

Choose a reason for hiding this comment

ivancea Feb 18, 2025

Choose a reason for hiding this comment

elasticsearchmachine commented Feb 18, 2025

bpintea Feb 20, 2025

Choose a reason for hiding this comment

astefan left a comment

Choose a reason for hiding this comment

ivancea commented Feb 21, 2025

astefan commented Feb 21, 2025

astefan left a comment

Choose a reason for hiding this comment

Uh oh!

Labels

4 participants

ivancea commented Feb 18, 2025 •

edited

Loading