Skip to content
Merged
Show file tree
Hide file tree
Changes from 26 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
d8cdf36
Use ser/deserialized functions in function tests, only Match failing
ivancea Feb 12, 2025
d277fd1
Fixed Match
ivancea Feb 13, 2025
1afc3ea
[CI] Auto commit changes from spotless
Feb 13, 2025
1b26239
Use the real Source, and implement for DateDiff
ivancea Feb 13, 2025
b8f315b
Updated tests to use the non-empty source, fixing the warnings and di…
ivancea Feb 13, 2025
5350252
[CI] Auto commit changes from spotless
Feb 13, 2025
0419e9d
Merge branch 'main' into esql-source-functions-warnings
ivancea Feb 13, 2025
2f48a59
Fixed RepeatStaticTests
ivancea Feb 13, 2025
a8f1698
Restore replaceChildren(), fix condition on serialization, and improv…
ivancea Feb 14, 2025
a65009f
Merge branch 'main' into esql-source-functions-warnings
ivancea Feb 14, 2025
2643e57
Add check on test supplier types
ivancea Feb 14, 2025
b99e5a2
Add TransportVersion and fix functions with warnings and no source se…
ivancea Feb 14, 2025
95b7937
Transport the source on missing functions, to avoid creating new TPs …
ivancea Feb 14, 2025
a7ee11b
Updated MvSlice with extra tests, and fixed source
ivancea Feb 14, 2025
5a30e5d
Merge branch 'main' into esql-source-functions-warnings
ivancea Feb 17, 2025
c5b83ff
Merge branch 'esql-source-functions-warnings' into esql-source-functi…
ivancea Feb 17, 2025
d347c14
Fixed CaseTests
ivancea Feb 17, 2025
92d49bc
Merge branch 'main' into esql-source-functions-warnings-fix
ivancea Feb 18, 2025
075d99e
Update docs/changelog/122821.yaml
ivancea Feb 18, 2025
e30e72f
Merge branch 'main' into esql-source-functions-warnings-fix
ivancea Feb 18, 2025
65bb97b
Update serialization tests
ivancea Feb 18, 2025
048693e
Fix InTests
ivancea Feb 18, 2025
79b1c4a
Lowercase test name
ivancea Feb 18, 2025
59a2218
Merge branch 'main' into esql-source-functions-warnings-fix
ivancea Feb 21, 2025
e85bbc1
Improve InTests helper methods
ivancea Feb 21, 2025
caca630
Added CSV tests for warnings
ivancea Feb 21, 2025
ce980d1
Fixed tests
ivancea Feb 24, 2025
ca37e8a
Merge branch 'main' into esql-source-functions-warnings-fix
ivancea Feb 24, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions docs/changelog/122821.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
pr: 122821
summary: Fix functions emitting warnings with no source
area: ES|QL
type: bug
issues:
- 122588
2 changes: 2 additions & 0 deletions server/src/main/java/org/elasticsearch/TransportVersions.java
Original file line number Diff line number Diff line change
Expand Up @@ -201,6 +201,8 @@ static TransportVersion def(int id) {
public static final TransportVersion REMOVE_REPOSITORY_CONFLICT_MESSAGE = def(9_012_0_00);
public static final TransportVersion RERANKER_FAILURES_ALLOWED = def(9_013_0_00);
public static final TransportVersion VOYAGE_AI_INTEGRATION_ADDED = def(9_014_0_00);
public static final TransportVersion ESQL_SERIALIZE_SOURCE_FUNCTIONS_WARNINGS = def(9_015_0_00);

/*
* STOP! READ THIS FIRST! No, really,
* ____ _____ ___ ____ _ ____ _____ _ ____ _____ _ _ ___ ____ _____ ___ ____ ____ _____ _
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -287,6 +287,26 @@ emp_no:integer | is_rehired:boolean | a1:boolean
10005 | [false,false,false,true] | false
;

mvSliceWarnings
required_capability: functions_source_serialization_warnings

FROM employees
| SORT first_name ASC
| EVAL
start = CASE(first_name == "Alejandro", 1, 0),
end = CASE(first_name == "Alejandro", 0, 1),
result = MV_SLICE(is_rehired, start, end)
| KEEP first_name, result
| LIMIT 1
;

warning:Line 6:10: evaluation of [MV_SLICE(is_rehired, start, end)] failed, treating result as null. Only first 20 failures recorded.
warning:Line 6:10: org.elasticsearch.xpack.esql.core.InvalidArgumentException: Start offset is greater than end offset

first_name:keyword | result:boolean
Alejandro | null
;

values
required_capability: agg_values

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -366,6 +366,24 @@ date1:date | dd_ms:integer
2023-12-02T11:00:00.000Z | 1000
;

dateDiffTestWarnings
required_capability: functions_source_serialization_warnings

FROM employees
| SORT first_name ASC
| EVAL date1 = TO_DATETIME("2023-12-02T11:00:00.000Z")
| EVAL dd_ms = DATE_DIFF(first_name, date1, date1)
| KEEP date1, dd_ms
| LIMIT 1
;

warning:Line 4:16: evaluation of [DATE_DIFF(first_name, date1, date1)] failed, treating result as null. Only first 20 failures recorded.
warning:Line 4:16: java.lang.IllegalArgumentException: A value of [YEAR, QUARTER, MONTH, DAYOFYEAR, DAY, WEEK, WEEKDAY, HOUR, MINUTE, SECOND, MILLISECOND, MICROSECOND, NANOSECOND] or their aliases is required; received [Alejandro]

date1:date | dd_ms:integer
2023-12-02T11:00:00.000Z | null
;

evalDateDiffMonthAsWhole0Months#[skip:-8.14.1, reason:omitting millis/timezone not allowed before 8.14]

ROW from=TO_DATETIME("2023-12-31T23:59:59.999Z"), to=TO_DATETIME("2024-01-01T00:00:00")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1009,6 +1009,33 @@ mvsum:unsigned_long
null
;

mvSumOverflowOnNode
required_capability: functions_source_serialization_warnings

FROM employees
| SORT first_name ASC
| EVAL
ints = CASE(first_name == "Alejandro", [0, 1, 2147483647], 0),
longs = CASE(first_name == "Alejandro", [0, 1, 9223372036854775807], 0),
ulongs = CASE(first_name == "Alejandro", [0, 1, 18446744073709551615], 0),
intsResult = mv_sum(ints),
longsResult = mv_sum(longs),
ulongsResult = mv_sum(ulongs)
| KEEP first_name, intsResult, longsResult, ulongsResult
| LIMIT 1
;

warning:Line 7:14: evaluation of [mv_sum(ints)] failed, treating result as null. Only first 20 failures recorded.
warning:Line 7:14: java.lang.ArithmeticException: integer overflow
warning:Line 8:15: evaluation of [mv_sum(longs)] failed, treating result as null. Only first 20 failures recorded.
warning:Line 8:15: java.lang.ArithmeticException: long overflow
warning:Line 9:16: evaluation of [mv_sum(ulongs)] failed, treating result as null. Only first 20 failures recorded.
warning:Line 9:16: java.lang.ArithmeticException: unsigned_long overflow

first_name:keyword | intsResult:integer | longsResult:long | ulongsResult:unsigned_long
Alejandro | null | null | null
;

e
// tag::e[]
ROW E()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1068,6 +1068,25 @@ Gatewayinstances|Gateway instances
null |null
;

replaceWarnings
required_capability: functions_source_serialization_warnings

FROM employees
| SORT first_name ASC
| EVAL
regex = CASE(first_name == "Alejandro", "(", ""),
result = replace(first_name, regex, "")
| KEEP first_name, result
| LIMIT 1
;

warning:Line 5:10: evaluation of [replace(first_name, regex, \"\")] failed, treating result as null. Only first 20 failures recorded.
warning:Line 5:10: java.util.regex.PatternSyntaxException: Unclosed group near index 1%0D%0A(

first_name:keyword | result:keyword
Alejandro | null
;

left
// tag::left[]
FROM employees
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,11 @@ public enum Cap {
*/
FN_ROUND_UL_FIXES,

/**
* Fixes for multiple functions not serializing their source, and emitting warnings with wrong line number and text.
*/
FUNCTIONS_SOURCE_SERIALIZATION_WARNINGS,

/**
* All functions that take TEXT should never emit TEXT, only KEYWORD. #114334
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ protected AggregateFunction(StreamInput in) throws IOException {

@Override
public final void writeTo(StreamOutput out) throws IOException {
Source.EMPTY.writeTo(out);
source().writeTo(out);
out.writeNamedWriteable(field);
if (out.getTransportVersion().onOrAfter(TransportVersions.V_8_16_0)) {
out.writeNamedWriteable(filter);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,7 @@ private DateDiff(StreamInput in) throws IOException {

@Override
public void writeTo(StreamOutput out) throws IOException {
Source.EMPTY.writeTo(out);
source().writeTo(out);
out.writeNamedWriteable(unit);
out.writeNamedWriteable(startTimestamp);
out.writeNamedWriteable(endTimestamp);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ protected AbstractMultivalueFunction(StreamInput in) throws IOException {

@Override
public final void writeTo(StreamOutput out) throws IOException {
Source.EMPTY.writeTo(out);
source().writeTo(out);
out.writeNamedWriteable(field);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ private MvSlice(StreamInput in) throws IOException {

@Override
public void writeTo(StreamOutput out) throws IOException {
Source.EMPTY.writeTo(out);
source().writeTo(out);
out.writeNamedWriteable(field);
out.writeNamedWriteable(start);
out.writeOptionalNamedWriteable(end);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
package org.elasticsearch.xpack.esql.expression.function.scalar.spatial;

import org.apache.lucene.util.BytesRef;
import org.elasticsearch.TransportVersions;
import org.elasticsearch.common.io.stream.StreamInput;
import org.elasticsearch.common.io.stream.StreamOutput;
import org.elasticsearch.geometry.Geometry;
Expand All @@ -20,6 +21,7 @@
import org.elasticsearch.xpack.esql.core.type.DataType;
import org.elasticsearch.xpack.esql.core.util.SpatialCoordinateTypes;
import org.elasticsearch.xpack.esql.expression.EsqlTypeResolutions;
import org.elasticsearch.xpack.esql.io.stream.PlanStreamInput;
import org.elasticsearch.xpack.esql.optimizer.rules.physical.local.LucenePushdownPredicates;

import java.io.IOException;
Expand Down Expand Up @@ -63,7 +65,9 @@ protected BinarySpatialFunction(
protected BinarySpatialFunction(StreamInput in, boolean leftDocValues, boolean rightDocValues, boolean pointsOnly) throws IOException {
// The doc-values fields are only used on data nodes local planning, and therefor never serialized
this(
Source.EMPTY,
in.getTransportVersion().onOrAfter(TransportVersions.ESQL_SERIALIZE_SOURCE_FUNCTIONS_WARNINGS)
? Source.readFrom((PlanStreamInput) in)
: Source.EMPTY,
in.readNamedWriteable(Expression.class),
in.readNamedWriteable(Expression.class),
leftDocValues,
Expand All @@ -74,6 +78,9 @@ protected BinarySpatialFunction(StreamInput in, boolean leftDocValues, boolean r

@Override
public void writeTo(StreamOutput out) throws IOException {
if (out.getTransportVersion().onOrAfter(TransportVersions.ESQL_SERIALIZE_SOURCE_FUNCTIONS_WARNINGS)) {
source().writeTo(out);
}
out.writeNamedWriteable(left());
out.writeNamedWriteable(right());
// The doc-values fields are only used on data nodes local planning, and therefor never serialized
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
package org.elasticsearch.xpack.esql.expression.function.scalar.string;

import org.apache.lucene.util.BytesRef;
import org.elasticsearch.TransportVersions;
import org.elasticsearch.common.io.stream.NamedWriteableRegistry;
import org.elasticsearch.common.io.stream.StreamInput;
import org.elasticsearch.common.io.stream.StreamOutput;
Expand All @@ -22,6 +23,7 @@
import org.elasticsearch.xpack.esql.expression.function.FunctionInfo;
import org.elasticsearch.xpack.esql.expression.function.Param;
import org.elasticsearch.xpack.esql.expression.function.scalar.EsqlScalarFunction;
import org.elasticsearch.xpack.esql.io.stream.PlanStreamInput;

import java.io.IOException;
import java.util.Arrays;
Expand Down Expand Up @@ -66,7 +68,9 @@ public Replace(

private Replace(StreamInput in) throws IOException {
this(
Source.EMPTY,
in.getTransportVersion().onOrAfter(TransportVersions.ESQL_SERIALIZE_SOURCE_FUNCTIONS_WARNINGS)
? Source.readFrom((PlanStreamInput) in)
: Source.EMPTY,
in.readNamedWriteable(Expression.class),
in.readNamedWriteable(Expression.class),
in.readNamedWriteable(Expression.class)
Expand All @@ -75,6 +79,9 @@ private Replace(StreamInput in) throws IOException {

@Override
public void writeTo(StreamOutput out) throws IOException {
if (out.getTransportVersion().onOrAfter(TransportVersions.ESQL_SERIALIZE_SOURCE_FUNCTIONS_WARNINGS)) {
source().writeTo(out);
}
out.writeNamedWriteable(str);
out.writeNamedWriteable(regex);
out.writeNamedWriteable(newStr);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

package org.elasticsearch.xpack.esql.expression.function.scalar.string;

import org.elasticsearch.TransportVersions;
import org.elasticsearch.common.io.stream.NamedWriteableRegistry;
import org.elasticsearch.common.io.stream.StreamInput;
import org.elasticsearch.common.io.stream.StreamOutput;
Expand Down Expand Up @@ -42,11 +43,20 @@ public ToLower(
}

private ToLower(StreamInput in) throws IOException {
this(Source.EMPTY, in.readNamedWriteable(Expression.class), ((PlanStreamInput) in).configuration());
this(
in.getTransportVersion().onOrAfter(TransportVersions.ESQL_SERIALIZE_SOURCE_FUNCTIONS_WARNINGS)
? Source.readFrom((PlanStreamInput) in)
: Source.EMPTY,
in.readNamedWriteable(Expression.class),
((PlanStreamInput) in).configuration()
);
}

@Override
public void writeTo(StreamOutput out) throws IOException {
if (out.getTransportVersion().onOrAfter(TransportVersions.ESQL_SERIALIZE_SOURCE_FUNCTIONS_WARNINGS)) {
source().writeTo(out);
}
out.writeNamedWriteable(field());
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

package org.elasticsearch.xpack.esql.expression.function.scalar.string;

import org.elasticsearch.TransportVersions;
import org.elasticsearch.common.io.stream.NamedWriteableRegistry;
import org.elasticsearch.common.io.stream.StreamInput;
import org.elasticsearch.common.io.stream.StreamOutput;
Expand Down Expand Up @@ -42,11 +43,20 @@ public ToUpper(
}

private ToUpper(StreamInput in) throws IOException {
this(Source.EMPTY, in.readNamedWriteable(Expression.class), ((PlanStreamInput) in).configuration());
this(
in.getTransportVersion().onOrAfter(TransportVersions.ESQL_SERIALIZE_SOURCE_FUNCTIONS_WARNINGS)
? Source.readFrom((PlanStreamInput) in)
: Source.EMPTY,
in.readNamedWriteable(Expression.class),
((PlanStreamInput) in).configuration()
);
}

@Override
public void writeTo(StreamOutput out) throws IOException {
if (out.getTransportVersion().onOrAfter(TransportVersions.ESQL_SERIALIZE_SOURCE_FUNCTIONS_WARNINGS)) {
source().writeTo(out);
}
out.writeNamedWriteable(field());
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,9 @@ public static List<TestCaseSupplier> stringCases(
@Override
public TestCase get() {
TestCase supplied = supplier.get();
if (types.size() != supplied.getData().size()) {
Copy link
Contributor Author

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

throw new IllegalStateException(name + ": type/data size mismatch " + types.size() + "/" + supplied.getData().size());
}
for (int i = 0; i < types.size(); i++) {
if (supplied.getData().get(i).type() != types.get(i)) {
throw new IllegalStateException(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,4 @@ protected Avg createTestInstance() {
protected Avg mutateInstance(Avg instance) throws IOException {
return new Avg(instance.source(), randomValueOtherThan(instance.field(), AbstractExpressionSerializationTests::randomChild));
}

@Override
protected boolean alwaysEmptySource() {
return true;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,4 @@ protected CountDistinct mutateInstance(CountDistinct instance) throws IOExceptio
}
return new CountDistinct(source, field, precision);
}

@Override
protected boolean alwaysEmptySource() {
return true;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,4 @@ protected Count createTestInstance() {
protected Count mutateInstance(Count instance) throws IOException {
return new Count(instance.source(), randomValueOtherThan(instance.field(), AbstractExpressionSerializationTests::randomChild));
}

@Override
protected boolean alwaysEmptySource() {
return true;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,4 @@ protected Max createTestInstance() {
protected Max mutateInstance(Max instance) throws IOException {
return new Max(instance.source(), randomValueOtherThan(instance.field(), AbstractExpressionSerializationTests::randomChild));
}

@Override
protected boolean alwaysEmptySource() {
return true;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,4 @@ protected MedianAbsoluteDeviation mutateInstance(MedianAbsoluteDeviation instanc
randomValueOtherThan(instance.field(), AbstractExpressionSerializationTests::randomChild)
);
}

@Override
protected boolean alwaysEmptySource() {
return true;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,4 @@ protected Median createTestInstance() {
protected Median mutateInstance(Median instance) throws IOException {
return new Median(instance.source(), randomValueOtherThan(instance.field(), AbstractExpressionSerializationTests::randomChild));
}

@Override
protected boolean alwaysEmptySource() {
return true;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,4 @@ protected Min createTestInstance() {
protected Min mutateInstance(Min instance) throws IOException {
return new Min(instance.source(), randomValueOtherThan(instance.field(), AbstractExpressionSerializationTests::randomChild));
}

@Override
protected boolean alwaysEmptySource() {
return true;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,4 @@ protected Percentile mutateInstance(Percentile instance) throws IOException {
}
return new Percentile(source, field, percentile);
}

@Override
protected boolean alwaysEmptySource() {
return true;
}
}
Loading