Skip to content

ESQL: Fix functions emitting warnings with no source #122821

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 28 commits into from
Feb 24, 2025
Merged

ESQL: Fix functions emitting warnings with no source #122821

merged 28 commits into from
Feb 24, 2025

Conversation

@ivancea
Copy link
Contributor

@ivancea ivancea commented Feb 18, 2025 •
edited
Loading

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

ivancea and others added 18 commits February 12, 2025 14:27
@ivancea
Use ser/deserialized functions in function tests, only Match failing
d8cdf36
@ivancea
Fixed Match
d277fd1
[CI] Auto commit changes from spotless
1afc3ea
@ivancea
Use the real Source, and implement for DateDiff
1b26239
@ivancea
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
Merge branch 'main' into esql-source-functions-warnings
0419e9d
@ivancea
Fixed RepeatStaticTests
2f48a59
@ivancea
Restore replaceChildren(), fix condition on serialization, and improv…
a8f1698 
…e some code
@ivancea
Merge branch 'main' into esql-source-functions-warnings
a65009f
@ivancea
Add check on test supplier types
2643e57
@ivancea
Add TransportVersion and fix functions with warnings and no source se…
b99e5a2 
…rialization
@ivancea
Transport the source on missing functions, to avoid creating new TPs …
95b7937 
…in the future if we need it
@ivancea
Updated MvSlice with extra tests, and fixed source
a7ee11b
@ivancea
Merge branch 'main' into esql-source-functions-warnings
5a30e5d
@ivancea
Merge branch 'esql-source-functions-warnings' into esql-source-functi…
c5b83ff 
…ons-warnings-fix

# Conflicts:
#	server/src/main/java/org/elasticsearch/TransportVersions.java
@ivancea
Fixed CaseTests
d347c14
@ivancea
Merge branch 'main' into esql-source-functions-warnings-fix
92d49bc 
# Conflicts:
#	server/src/main/java/org/elasticsearch/TransportVersions.java
@ivancea 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
@elasticsearchmachine
Copy link
Collaborator

elasticsearchmachine commented Feb 18, 2025

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

ivancea
ivancea commented Feb 18, 2025
View reviewed changes
...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()) {
Copy link
Contributor Author

@ivancea ivancea Feb 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
ivancea commented Feb 18, 2025
View reviewed changes
...st/java/org/elasticsearch/xpack/esql/expression/function/scalar/multivalue/MvSliceTests.java
Copy link
Contributor Author

@ivancea ivancea Feb 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@ivancea ivancea mentioned this pull request Feb 18, 2025
Merged
2 tasks
@ivancea ivancea marked this pull request as ready for review February 18, 2025 14:58
@elasticsearchmachine
Copy link
Collaborator

elasticsearchmachine commented Feb 18, 2025

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

bpintea
bpintea reviewed Feb 20, 2025
View reviewed changes
...test/java/org/elasticsearch/xpack/esql/expression/predicate/operator/comparison/InTests.java Outdated
/**
* Returns a list with N dataType1, followed by 1 dataType2.
*/
private static List<DataType> makeTypes(DataType dataType1, DataType dataType2, int n) {
Copy link
Contributor

@bpintea bpintea Feb 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
astefan reviewed Feb 21, 2025
View reviewed changes
Copy link
Contributor

@astefan astefan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@ivancea
Merge branch 'main' into esql-source-functions-warnings-fix
59a2218 
# Conflicts:
#	server/src/main/java/org/elasticsearch/TransportVersions.java
@ivancea
Copy link
Contributor Author

ivancea commented Feb 21, 2025

@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
Copy link
Contributor

astefan commented Feb 21, 2025

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.

@ivancea ivancea requested a review from astefan February 21, 2025 17:27
astefan
astefan approved these changes Feb 24, 2025
View reviewed changes
Copy link
Contributor

@astefan astefan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ivancea
Fixed tests
ce980d1
@ivancea
Merge branch 'main' into esql-source-functions-warnings-fix
ca37e8a 
# Conflicts:
#	server/src/main/java/org/elasticsearch/TransportVersions.java
@ivancea ivancea enabled auto-merge (squash) February 24, 2025 12:36
@ivancea ivancea merged commit c40c5a6 into elastic:main Feb 24, 2025
17 checks passed
@ivancea ivancea deleted the esql-source-functions-warnings-fix branch February 24, 2025 13:52
@wchaparro wchaparro mentioned this pull request May 23, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@astefan astefan astefan approved these changes

@bpintea bpintea bpintea approved these changes

@nik9000 nik9000 Awaiting requested review from nik9000

Assignees

No one assigned

Labels

:Analytics/ES|QL AKA ESQL >bug Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) v9.1.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

ESQL: Functions not serializing their Source and emitting warnings

4 participants

@ivancea @elasticsearchmachine @astefan @bpintea