Skip to content
Merged
Show file tree
Hide file tree
Changes from 20 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
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,7 @@ static TransportVersion def(int id) {
public static final TransportVersion ESQL_SUPPORT_PARTIAL_RESULTS = def(9_011_0_00);
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 ESQL_SERIALIZE_SOURCE_FUNCTIONS_WARNINGS = def(9_014_0_00);

/*
* STOP! READ THIS FIRST! No, really,
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 @@ -126,7 +126,7 @@ public static Iterable<Object[]> parameters() {
)
),
new TestCaseSupplier(
List.of(DataType.IP),
List.of(DataType.IP, DataType.INTEGER, DataType.KEYWORD),
() -> new TestCaseSupplier.TestCase(
List.of(
TestCaseSupplier.TypedData.multiRow(
Expand Down Expand Up @@ -215,7 +215,7 @@ public static Iterable<Object[]> parameters() {
)
),
new TestCaseSupplier(
List.of(DataType.IP),
List.of(DataType.IP, DataType.INTEGER, DataType.KEYWORD),
() -> new TestCaseSupplier.TestCase(
List.of(
TestCaseSupplier.TypedData.multiRow(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -532,7 +532,7 @@ private static void fourAndFiveArgs(
suppliers.add(
new TestCaseSupplier(
"partial foldable 1 " + TestCaseSupplier.nameFrom(Arrays.asList(cond1, type, cond2, type)),
List.of(DataType.BOOLEAN, type, DataType.BOOLEAN, type),
List.of(DataType.BOOLEAN, type, DataType.BOOLEAN, type, type),
() -> {
Object r1 = randomLiteral(type).value();
Object r2 = randomLiteral(type).value();
Expand Down
Copy link
Contributor Author

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

Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
import org.apache.lucene.util.BytesRef;
import org.elasticsearch.geo.GeometryTestUtils;
import org.elasticsearch.geo.ShapeTestUtils;
import org.elasticsearch.xpack.esql.core.InvalidArgumentException;
import org.elasticsearch.xpack.esql.core.expression.Expression;
import org.elasticsearch.xpack.esql.core.tree.Source;
import org.elasticsearch.xpack.esql.core.type.DataType;
Expand Down Expand Up @@ -44,6 +45,64 @@ public static Iterable<Object[]> parameters() {
longs(suppliers);
doubles(suppliers);
bytesRefs(suppliers);

// Warnings cases
suppliers.stream().toList().forEach(supplier -> {
DataType firstArgumentType = supplier.types().get(0);
String evaluatorTypePart = switch (firstArgumentType) {
case BOOLEAN -> "Boolean";
case INTEGER -> "Int";
case LONG, DATE_NANOS, DATETIME, UNSIGNED_LONG -> "Long";
case DOUBLE -> "Double";
case KEYWORD, TEXT, SEMANTIC_TEXT, IP, VERSION, GEO_POINT, CARTESIAN_POINT, GEO_SHAPE, CARTESIAN_SHAPE -> "BytesRef";
default -> throw new IllegalArgumentException("Unsupported type: " + firstArgumentType);
};

// Start offset greater than end offset
suppliers.add(new TestCaseSupplier(List.of(firstArgumentType, DataType.INTEGER, DataType.INTEGER), () -> {
int end = randomIntBetween(0, 10);
int start = randomIntBetween(end + 1, end + 10);
return new TestCaseSupplier.TestCase(
List.of(
new TestCaseSupplier.TypedData(List.of(randomLiteral(firstArgumentType).value()), firstArgumentType, "field"),
new TestCaseSupplier.TypedData(start, DataType.INTEGER, "start"),
new TestCaseSupplier.TypedData(end, DataType.INTEGER, "end")
),
"MvSlice"
+ evaluatorTypePart
+ "Evaluator[field=Attribute[channel=0], start=Attribute[channel=1], end=Attribute[channel=2]]",
firstArgumentType,
nullValue()
).withFoldingException(InvalidArgumentException.class, "Start offset is greater than end offset")
.withWarning("Line 1:1: evaluation of [source] failed, treating result as null. Only first 20 failures recorded.")
.withWarning(
"Line 1:1: org.elasticsearch.xpack.esql.core.InvalidArgumentException: Start offset is greater than end offset"
);
}));

// Negative start with positive end
suppliers.add(new TestCaseSupplier(List.of(firstArgumentType, DataType.INTEGER, DataType.INTEGER), () -> {
int start = randomIntBetween(-10, -1);
int end = randomIntBetween(0, 10);
return new TestCaseSupplier.TestCase(
List.of(
new TestCaseSupplier.TypedData(List.of(randomLiteral(firstArgumentType).value()), firstArgumentType, "field"),
new TestCaseSupplier.TypedData(start, DataType.INTEGER, "start"),
new TestCaseSupplier.TypedData(end, DataType.INTEGER, "end")
),
"MvSlice"
+ evaluatorTypePart
+ "Evaluator[field=Attribute[channel=0], start=Attribute[channel=1], end=Attribute[channel=2]]",
firstArgumentType,
nullValue()
).withFoldingException(InvalidArgumentException.class, "Start and end offset have different signs")
.withWarning("Line 1:1: evaluation of [source] failed, treating result as null. Only first 20 failures recorded.")
.withWarning(
"Line 1:1: org.elasticsearch.xpack.esql.core.InvalidArgumentException: Start and end offset have different signs"
);
}));
});

return parameterSuppliersFromTypedData(
anyNullIsNull(
suppliers,
Expand Down