Skip to content
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ public final BinaryScalarFunction replaceChildren(List<Expression> newChildren)
Expression newLeft = newChildren.get(0);
Expression newRight = newChildren.get(1);

return left.equals(newLeft) && right.equals(newRight) ? this : replaceChildren(newLeft, newRight);
return replaceChildren(newLeft, newRight);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Finally replaced with that, and restored the original code here

}

protected abstract BinaryScalarFunction replaceChildren(Expression newLeft, Expression newRight);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,10 @@ public String toString() {
return text + location;
}

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

Choose a reason for hiding this comment

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

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

*/
@Deprecated
public static Source synthetic(String text) {
return new Source(Location.EMPTY, text);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@
import org.elasticsearch.xpack.esql.expression.predicate.operator.comparison.LessThan;
import org.elasticsearch.xpack.esql.expression.predicate.operator.comparison.LessThanOrEqual;
import org.elasticsearch.xpack.esql.expression.predicate.operator.comparison.NotEquals;
import org.elasticsearch.xpack.esql.io.stream.PlanStreamOutput;
import org.elasticsearch.xpack.esql.optimizer.rules.logical.FoldNull;
import org.elasticsearch.xpack.esql.parser.ExpressionBuilder;
import org.elasticsearch.xpack.esql.planner.Layout;
Expand Down Expand Up @@ -102,6 +103,7 @@
import static org.elasticsearch.xpack.esql.EsqlTestUtils.randomLiteral;
import static org.elasticsearch.xpack.esql.EsqlTestUtils.unboundLogicalOptimizerContext;
import static org.elasticsearch.xpack.esql.SerializationTestUtils.assertSerialization;
import static org.elasticsearch.xpack.esql.SerializationTestUtils.serializeDeserialize;
import static org.elasticsearch.xpack.esql.expression.function.EsqlFunctionRegistry.mapParam;
import static org.elasticsearch.xpack.esql.expression.function.EsqlFunctionRegistry.param;
import static org.elasticsearch.xpack.esql.expression.function.EsqlFunctionRegistry.paramWithoutAnnotation;
Expand Down Expand Up @@ -331,7 +333,7 @@ protected static String typeErrorMessage(
String ordinal = includeOrdinal ? TypeResolutions.ParamOrdinal.fromIndex(badArgPosition).name().toLowerCase(Locale.ROOT) + " " : "";
String expectedTypeString = expectedTypeSupplier.apply(validPerPosition.get(badArgPosition), badArgPosition);
String name = types.get(badArgPosition).typeName();
return ordinal + "argument of [] must be [" + expectedTypeString + "], found value [" + name + "] type [" + name + "]";
return ordinal + "argument of [source] must be [" + expectedTypeString + "], found value [" + name + "] type [" + name + "]";
}

@FunctionalInterface
Expand Down Expand Up @@ -522,7 +524,7 @@ public static Expression deepCopyOfField(String name, DataType type) {
* <strong>except</strong> those that have been marked with {@link TestCaseSupplier.TypedData#forceLiteral()}.
*/
protected final Expression buildFieldExpression(TestCaseSupplier.TestCase testCase) {
return build(testCase.getSource(), testCase.getDataAsFields());
return randomSerializeDeserialize(build(testCase.getSource(), testCase.getDataAsFields()));
Copy link
Member

Choose a reason for hiding this comment

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

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

}

/**
Expand All @@ -531,12 +533,41 @@ protected final Expression buildFieldExpression(TestCaseSupplier.TestCase testCa
* those that have been marked with {@link TestCaseSupplier.TypedData#forceLiteral()}.
*/
protected final Expression buildDeepCopyOfFieldExpression(TestCaseSupplier.TestCase testCase) {
// We don't use `randomSerializeDeserialize()` here as the deep copied fields aren't deserializable right now
return build(testCase.getSource(), testCase.getDataAsDeepCopiedFields());
}

private Expression randomSerializeDeserialize(Expression expression) {
if (false && randomBoolean()) {
Copy link
Member

Choose a reason for hiding this comment

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

false && maybe a leftover

return expression;
}

return serializeDeserializeExpression(expression);
}

/**
* Returns the expression after being serialized and deserialized.
* <p>
* Tests randomly go through this method to ensure that the function retains the same logic after serialization and deserialization.
* </p>
* <p>
* Can be overridden to provide custom serialization and deserialization logic, or disable it if needed.
* </p>
*/
protected Expression serializeDeserializeExpression(Expression expression) {
Expression newExpression = serializeDeserialize(
expression,
PlanStreamOutput::writeNamedWriteable,
in -> in.readNamedWriteable(Expression.class),
testCase.getConfiguration() // The configuration query should be == to the source text of the function for this to work
);
// Fields use synthetic sources, which can't be serialized. So we use the originals instead.
return newExpression.replaceChildrenSameSize(expression.children());
}

protected final Expression buildLiteralExpression(TestCaseSupplier.TestCase testCase) {
assumeTrue("Data can't be converted to literals", testCase.canGetDataAsLiterals());
return build(testCase.getSource(), testCase.getDataAsLiterals());
return randomSerializeDeserialize(build(testCase.getSource(), testCase.getDataAsLiterals()));
}

public static EvaluatorMapper.ToEvaluator toEvaluator() {
Expand Down Expand Up @@ -711,7 +742,7 @@ private static BytesRef randomizeBytesRefOffset(BytesRef bytesRef) {
}

public void testSerializationOfSimple() {
assertSerialization(buildFieldExpression(testCase));
assertSerialization(buildFieldExpression(testCase), testCase.getConfiguration());
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -401,8 +401,8 @@ protected static TestCaseSupplier arithmeticExceptionOverflowCase(
evaluator + "[lhs=Attribute[channel=0], rhs=Attribute[channel=1]]",
dataType,
is(nullValue())
).withWarning("Line -1:-1: evaluation of [] failed, treating result as null. Only first 20 failures recorded.")
.withWarning("Line -1:-1: java.lang.ArithmeticException: " + typeNameOverflow)
).withWarning("Line 1:1: evaluation of [source] failed, treating result as null. Only first 20 failures recorded.")
.withWarning("Line 1:1: java.lang.ArithmeticException: " + typeNameOverflow)
);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,15 @@
import org.elasticsearch.logging.LogManager;
import org.elasticsearch.logging.Logger;
import org.elasticsearch.test.ESTestCase;
import org.elasticsearch.xpack.esql.EsqlTestUtils;
import org.elasticsearch.xpack.esql.core.expression.Expression;
import org.elasticsearch.xpack.esql.core.expression.Literal;
import org.elasticsearch.xpack.esql.core.expression.MapExpression;
import org.elasticsearch.xpack.esql.core.tree.Location;
import org.elasticsearch.xpack.esql.core.tree.Source;
import org.elasticsearch.xpack.esql.core.type.DataType;
import org.elasticsearch.xpack.esql.core.util.NumericUtils;
import org.elasticsearch.xpack.esql.session.Configuration;
import org.elasticsearch.xpack.versionfield.Version;
import org.hamcrest.Matcher;

Expand Down Expand Up @@ -54,6 +57,9 @@ public record TestCaseSupplier(String name, List<DataType> types, Supplier<TestC
implements
Supplier<TestCaseSupplier.TestCase> {

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

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

public static final Configuration TEST_CONFIGURATION = EsqlTestUtils.configuration(TEST_SOURCE.text());
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


private static final Logger logger = LogManager.getLogger(TestCaseSupplier.class);

/**
Expand Down Expand Up @@ -1388,6 +1394,10 @@ public static final class TestCase {
* The {@link Source} this test case should be run with
*/
private final Source source;
/**
* The {@link Configuration} this test case should use
*/
private final Configuration configuration;
/**
* The parameter values and types to pass into the function for this test run
*/
Expand Down Expand Up @@ -1490,7 +1500,8 @@ public static TestCase typeError(List<TypedData> data, String expectedTypeError)
Object extra,
boolean canBuildEvaluator
) {
this.source = Source.EMPTY;
this.source = TEST_SOURCE;
this.configuration = TEST_CONFIGURATION;
this.data = data;
this.evaluatorToString = evaluatorToString;
this.expectedType = expectedType == null ? null : expectedType.noText();
Expand All @@ -1510,6 +1521,10 @@ public Source getSource() {
return source;
}

public Configuration getConfiguration() {
return configuration;
}

public List<TypedData> getData() {
return data;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,7 @@ public static Iterable<Object[]> parameters() {
new TestCaseSupplier.TypedData(0, DataType.INTEGER, "limit").forceLiteral(),
new TestCaseSupplier.TypedData(new BytesRef("desc"), DataType.KEYWORD, "order").forceLiteral()
),
"Limit must be greater than 0 in [], found [0]"
"Limit must be greater than 0 in [source], found [0]"
)
),
new TestCaseSupplier(
Expand All @@ -252,7 +252,7 @@ public static Iterable<Object[]> parameters() {
new TestCaseSupplier.TypedData(2, DataType.INTEGER, "limit").forceLiteral(),
new TestCaseSupplier.TypedData(new BytesRef("wrong-order"), DataType.KEYWORD, "order").forceLiteral()
),
"Invalid order value in [], expected [ASC, DESC] but got [wrong-order]"
"Invalid order value in [source], expected [ASC, DESC] but got [wrong-order]"
)
),
new TestCaseSupplier(
Expand All @@ -263,7 +263,7 @@ public static Iterable<Object[]> parameters() {
new TestCaseSupplier.TypedData(null, DataType.INTEGER, "limit").forceLiteral(),
new TestCaseSupplier.TypedData(new BytesRef("desc"), DataType.KEYWORD, "order").forceLiteral()
),
"second argument of [] cannot be null, received [limit]"
"second argument of [source] cannot be null, received [limit]"
)
),
new TestCaseSupplier(
Expand All @@ -274,7 +274,7 @@ public static Iterable<Object[]> parameters() {
new TestCaseSupplier.TypedData(1, DataType.INTEGER, "limit").forceLiteral(),
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]"
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

)
)
)
Expand Down Expand Up @@ -317,4 +317,10 @@ private static TestCaseSupplier makeSupplier(
);
});
}

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

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

return expression;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,13 @@
import org.elasticsearch.xpack.esql.core.type.DataType;
import org.elasticsearch.xpack.esql.expression.function.FunctionName;
import org.elasticsearch.xpack.esql.expression.function.TestCaseSupplier;
import org.elasticsearch.xpack.esql.io.stream.PlanStreamOutput;

import java.util.ArrayList;
import java.util.List;
import java.util.function.Supplier;

import static org.elasticsearch.xpack.esql.SerializationTestUtils.serializeDeserialize;
import static org.elasticsearch.xpack.esql.core.type.DataType.BOOLEAN;
import static org.elasticsearch.xpack.esql.core.type.DataType.KEYWORD;
import static org.elasticsearch.xpack.esql.core.type.DataType.UNSUPPORTED;
Expand Down Expand Up @@ -83,4 +85,19 @@ protected Expression build(Source source, List<Expression> args) {
}
return match;
}

/**
* Copy of the overridden method that doesn't check for children size, as the {@code options} child isn't serialized in Match.
*/
@Override
protected Expression serializeDeserializeExpression(Expression expression) {
Expression newExpression = serializeDeserialize(
expression,
PlanStreamOutput::writeNamedWriteable,
in -> in.readNamedWriteable(Expression.class),
testCase.getConfiguration() // The configuration query should be == to the source text of the function for this to work
);
// Fields use synthetic sources, which can't be serialized. So we use the originals instead.
return newExpression.replaceChildren(expression.children());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
package org.elasticsearch.xpack.esql.expression.function.scalar;

import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.xpack.esql.EsqlTestUtils;
import org.elasticsearch.xpack.esql.core.expression.Expression;
import org.elasticsearch.xpack.esql.core.tree.Source;
import org.elasticsearch.xpack.esql.core.util.StringUtils;
Expand All @@ -27,10 +26,25 @@ public abstract class AbstractConfigurationFunctionTestCase extends AbstractScal

@Override
protected Expression build(Source source, List<Expression> args) {
return buildWithConfiguration(source, args, EsqlTestUtils.TEST_CFG);
return buildWithConfiguration(source, args, testCase.getConfiguration());
}

static Configuration randomConfiguration() {
public void testSerializationWithConfiguration() {
Configuration config = randomConfiguration();
Expression expr = buildWithConfiguration(testCase.getSource(), testCase.getDataAsFields(), config);

assertSerialization(expr, config);

Configuration differentConfig;
do {
differentConfig = randomConfiguration();
} while (config.equals(differentConfig));
Copy link
Contributor

Choose a reason for hiding this comment

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

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


Expression differentExpr = buildWithConfiguration(testCase.getSource(), testCase.getDataAsFields(), differentConfig);
assertFalse(expr.equals(differentExpr));
}

private static Configuration randomConfiguration() {
// TODO: Randomize the query and maybe the pragmas.
return new Configuration(
randomZone(),
Expand All @@ -46,19 +60,4 @@ static Configuration randomConfiguration() {
System.nanoTime()
);
}

public void testSerializationWithConfiguration() {
Configuration config = randomConfiguration();
Expression expr = buildWithConfiguration(testCase.getSource(), testCase.getDataAsFields(), config);

assertSerialization(expr, config);

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

Expression differentExpr = buildWithConfiguration(testCase.getSource(), testCase.getDataAsFields(), differentConfig);
assertFalse(expr.equals(differentExpr));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -75,4 +75,10 @@ public static Iterable<Object[]> parameters() {
)
);
}

@Override
protected Expression serializeDeserializeExpression(Expression expression) {
// AggregateMetricDoubleLiteral can't be serialized when it's a literal
return expression;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,8 @@ public static Iterable<Object[]> parameters() {
bytesRef -> {
var exception = expectThrows(Exception.class, () -> CARTESIAN.wktToWkb(bytesRef.utf8ToString()));
return List.of(
"Line -1:-1: evaluation of [] failed, treating result as null. Only first 20 failures recorded.",
"Line -1:-1: " + exception
"Line 1:1: evaluation of [source] failed, treating result as null. Only first 20 failures recorded.",
"Line 1:1: " + exception
);
}
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,8 @@ public static Iterable<Object[]> parameters() {
bytesRef -> {
var exception = expectThrows(Exception.class, () -> CARTESIAN.wktToWkb(bytesRef.utf8ToString()));
return List.of(
"Line -1:-1: evaluation of [] failed, treating result as null. Only first 20 failures recorded.",
"Line -1:-1: " + exception
"Line 1:1: evaluation of [source] failed, treating result as null. Only first 20 failures recorded.",
"Line 1:1: " + exception
);
}
);
Expand Down
Loading