Skip to content

Commit 55141a2

Browse files
Remove deprecated function isNotNullAndFoldable (#130944)
This PR removes the deprecated function isNotNullAndFoldable. It was getting called in the TypeResolution for various functions. With the change, we still check the isNotNull part during TypeResolution. However, a lot of the other checks are moved to postOptimizationVerification(), that happens after the constant have been folded during normal logical planning. Resolves #119756 However, it seems that there are still a few places we do folding outside logical planning. They are marked with /* TODO remove me */ and will be handled in a separate PR
1 parent 4eaa2ea commit 55141a2

File tree

18 files changed

+979
-138
lines changed

18 files changed

+979
-138
lines changed

docs/changelog/130944.yaml

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
pr: 130944
2+
summary: Remove unnecessary calls to Fold
3+
area: ES|QL
4+
type: enhancement
5+
issues:
6+
- 119756

x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/expression/TypeResolutions.java

Lines changed: 0 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -133,36 +133,6 @@ public static TypeResolution isFoldable(Expression e, String operationName, Para
133133
return TypeResolution.TYPE_RESOLVED;
134134
}
135135

136-
/**
137-
* Is this {@link Expression#foldable()} and not {@code null}.
138-
*
139-
* @deprecated instead of calling this, check for a {@link Literal} containing
140-
* {@code null}. Foldable expressions will be folded by other rules,
141-
* eventually, to a {@link Literal}.
142-
*/
143-
@Deprecated
144-
public static TypeResolution isNotNullAndFoldable(Expression e, String operationName, ParamOrdinal paramOrd) {
145-
TypeResolution resolution = isFoldable(e, operationName, paramOrd);
146-
147-
if (resolution.unresolved()) {
148-
return resolution;
149-
}
150-
151-
if (e.dataType() == DataType.NULL || e.fold(FoldContext.small()) == null) {
152-
resolution = new TypeResolution(
153-
format(
154-
null,
155-
"{}argument of [{}] cannot be null, received [{}]",
156-
paramOrd == null || paramOrd == DEFAULT ? "" : paramOrd.name().toLowerCase(Locale.ROOT) + " ",
157-
operationName,
158-
Expressions.name(e)
159-
)
160-
);
161-
}
162-
163-
return resolution;
164-
}
165-
166136
public static TypeResolution isNotNull(Expression e, String operationName, ParamOrdinal paramOrd) {
167137
if (e.dataType() == DataType.NULL) {
168138
return new TypeResolution(
Lines changed: 151 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,151 @@
1+
/*
2+
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
3+
* or more contributor license agreements. Licensed under the Elastic License
4+
* 2.0; you may not use this file except in compliance with the Elastic License
5+
* 2.0.
6+
*/
7+
package org.elasticsearch.xpack.esql.expression.function;
8+
9+
import org.elasticsearch.common.lucene.BytesRefs;
10+
import org.elasticsearch.core.Nullable;
11+
import org.elasticsearch.xpack.esql.EsqlIllegalArgumentException;
12+
import org.elasticsearch.xpack.esql.common.Failure;
13+
import org.elasticsearch.xpack.esql.common.Failures;
14+
import org.elasticsearch.xpack.esql.core.expression.Expression;
15+
import org.elasticsearch.xpack.esql.core.expression.Literal;
16+
17+
import static org.elasticsearch.common.logging.LoggerMessageFormat.format;
18+
import static org.elasticsearch.xpack.esql.common.Failure.fail;
19+
20+
public class FunctionUtils {
21+
/**
22+
* A utility class to validate the type resolution of expressions before and after logical planning.
23+
* If null is passed for Failures to the constructor, it means we are only type resolution.
24+
* This is usually called when doing pre-logical planning validation.
25+
* If a {@link Failures} instance is passed, it means we are doing post-logical planning validation as well.
26+
* This is usually called after folding is done, during
27+
* {@link org.elasticsearch.xpack.esql.capabilities.PostOptimizationVerificationAware} verification
28+
*/
29+
public static class TypeResolutionValidator {
30+
31+
Expression.TypeResolution typeResolution = Expression.TypeResolution.TYPE_RESOLVED;
32+
@Nullable
33+
private final Failures postValidationFailures; // null means we are doing pre-folding validation only
34+
private final Expression field;
35+
36+
public static TypeResolutionValidator forPreOptimizationValidation(Expression field) {
37+
return new TypeResolutionValidator(field, null);
38+
}
39+
40+
public static TypeResolutionValidator forPostOptimizationValidation(Expression field, Failures failures) {
41+
return new TypeResolutionValidator(field, failures);
42+
}
43+
44+
private TypeResolutionValidator(Expression field, Failures failures) {
45+
this.field = field;
46+
this.postValidationFailures = failures;
47+
}
48+
49+
public void invalidIfPostValidation(Failure failure) {
50+
if (postValidationFailures != null) {
51+
postValidationFailures.add(failure);
52+
}
53+
}
54+
55+
public void invalid(Expression.TypeResolution message) {
56+
typeResolution = message;
57+
if (postValidationFailures != null) {
58+
postValidationFailures.add(fail(field, message.message()));
59+
}
60+
}
61+
62+
public Expression.TypeResolution getResolvedType() {
63+
return typeResolution;
64+
}
65+
}
66+
67+
public static Integer limitValue(Expression limitField, String sourceText) {
68+
if (limitField instanceof Literal literal) {
69+
Object value = literal.value();
70+
if (value instanceof Integer intValue) {
71+
return intValue;
72+
}
73+
}
74+
throw new EsqlIllegalArgumentException(format(null, "Limit value must be an integer in [{}], found [{}]", sourceText, limitField));
75+
}
76+
77+
/**
78+
* We check that the limit is not null and that if it is a literal, it is a positive integer
79+
* During postOptimizationVerification folding is already done, so we also verify that it is definitively a literal
80+
*/
81+
public static Expression.TypeResolution resolveTypeLimit(Expression limitField, String sourceText, TypeResolutionValidator validator) {
82+
if (limitField == null) {
83+
validator.invalid(
84+
new Expression.TypeResolution(format(null, "Limit must be a constant integer in [{}], found [{}]", sourceText, limitField))
85+
);
86+
} else if (limitField instanceof Literal literal) {
87+
if (literal.value() == null) {
88+
validator.invalid(
89+
new Expression.TypeResolution(
90+
format(null, "Limit must be a constant integer in [{}], found [{}]", sourceText, limitField)
91+
)
92+
);
93+
} else {
94+
int value = (Integer) literal.value();
95+
if (value <= 0) {
96+
validator.invalid(
97+
new Expression.TypeResolution(format(null, "Limit must be greater than 0 in [{}], found [{}]", sourceText, value))
98+
);
99+
}
100+
}
101+
} else {
102+
// it is expected that the expression is a literal after folding
103+
// we fail if it is not a literal
104+
validator.invalidIfPostValidation(
105+
fail(limitField, "Limit must be a constant integer in [{}], found [{}]", sourceText, limitField)
106+
);
107+
}
108+
return validator.getResolvedType();
109+
}
110+
111+
/**
112+
* We check that the query is not null and that if it is a literal, it is a string
113+
* During postOptimizationVerification folding is already done, so we also verify that it is definitively a literal
114+
*/
115+
public static Expression.TypeResolution resolveTypeQuery(Expression queryField, String sourceText, TypeResolutionValidator validator) {
116+
if (queryField == null) {
117+
validator.invalid(
118+
new Expression.TypeResolution(format(null, "Query must be a valid string in [{}], found [{}]", sourceText, queryField))
119+
);
120+
} else if (queryField instanceof Literal literal) {
121+
if (literal.value() == null) {
122+
validator.invalid(
123+
new Expression.TypeResolution(format(null, "Query value cannot be null in [{}], but got [{}]", sourceText, queryField))
124+
);
125+
}
126+
} else {
127+
// it is expected that the expression is a literal after folding
128+
// we fail if it is not a literal
129+
validator.invalidIfPostValidation(fail(queryField, "Query must be a valid string in [{}], found [{}]", sourceText, queryField));
130+
}
131+
return validator.getResolvedType();
132+
}
133+
134+
public static Object queryAsObject(Expression queryField, String sourceText) {
135+
if (queryField instanceof Literal literal) {
136+
return literal.value();
137+
}
138+
throw new EsqlIllegalArgumentException(
139+
format(null, "Query value must be a constant string in [{}], found [{}]", sourceText, queryField)
140+
);
141+
}
142+
143+
public static String queryAsString(Expression queryField, String sourceText) {
144+
if (queryField instanceof Literal literal) {
145+
return BytesRefs.toString(literal.value());
146+
}
147+
throw new EsqlIllegalArgumentException(
148+
format(null, "Query value must be a constant string in [{}], found [{}]", sourceText, queryField)
149+
);
150+
}
151+
}

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/aggregate/Sample.java

Lines changed: 17 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -17,29 +17,33 @@
1717
import org.elasticsearch.compute.aggregation.SampleIntAggregatorFunctionSupplier;
1818
import org.elasticsearch.compute.aggregation.SampleLongAggregatorFunctionSupplier;
1919
import org.elasticsearch.xpack.esql.EsqlIllegalArgumentException;
20+
import org.elasticsearch.xpack.esql.capabilities.PostOptimizationVerificationAware;
21+
import org.elasticsearch.xpack.esql.common.Failures;
2022
import org.elasticsearch.xpack.esql.core.expression.Expression;
21-
import org.elasticsearch.xpack.esql.core.expression.FoldContext;
2223
import org.elasticsearch.xpack.esql.core.expression.Literal;
2324
import org.elasticsearch.xpack.esql.core.tree.NodeInfo;
2425
import org.elasticsearch.xpack.esql.core.tree.Source;
2526
import org.elasticsearch.xpack.esql.core.type.DataType;
2627
import org.elasticsearch.xpack.esql.expression.function.Example;
2728
import org.elasticsearch.xpack.esql.expression.function.FunctionInfo;
2829
import org.elasticsearch.xpack.esql.expression.function.FunctionType;
30+
import org.elasticsearch.xpack.esql.expression.function.FunctionUtils;
2931
import org.elasticsearch.xpack.esql.expression.function.Param;
3032
import org.elasticsearch.xpack.esql.planner.PlannerUtils;
3133
import org.elasticsearch.xpack.esql.planner.ToAggregator;
3234

3335
import java.io.IOException;
3436
import java.util.List;
3537

36-
import static org.elasticsearch.common.logging.LoggerMessageFormat.format;
3738
import static org.elasticsearch.xpack.esql.core.expression.TypeResolutions.ParamOrdinal.FIRST;
3839
import static org.elasticsearch.xpack.esql.core.expression.TypeResolutions.ParamOrdinal.SECOND;
39-
import static org.elasticsearch.xpack.esql.core.expression.TypeResolutions.isNotNullAndFoldable;
40+
import static org.elasticsearch.xpack.esql.core.expression.TypeResolutions.isNotNull;
4041
import static org.elasticsearch.xpack.esql.core.expression.TypeResolutions.isType;
42+
import static org.elasticsearch.xpack.esql.expression.function.FunctionUtils.TypeResolutionValidator.forPostOptimizationValidation;
43+
import static org.elasticsearch.xpack.esql.expression.function.FunctionUtils.TypeResolutionValidator.forPreOptimizationValidation;
44+
import static org.elasticsearch.xpack.esql.expression.function.FunctionUtils.resolveTypeLimit;
4145

42-
public class Sample extends AggregateFunction implements ToAggregator {
46+
public class Sample extends AggregateFunction implements ToAggregator, PostOptimizationVerificationAware {
4347
public static final NamedWriteableRegistry.Entry ENTRY = new NamedWriteableRegistry.Entry(Expression.class, "Sample", Sample::new);
4448

4549
@FunctionInfo(
@@ -110,14 +114,14 @@ protected TypeResolution resolveType() {
110114
return new TypeResolution("Unresolved children");
111115
}
112116
var typeResolution = isType(field(), dt -> dt != DataType.UNSIGNED_LONG, sourceText(), FIRST, "any type except unsigned_long").and(
113-
isNotNullAndFoldable(limitField(), sourceText(), SECOND)
117+
isNotNull(limitField(), sourceText(), SECOND)
114118
).and(isType(limitField(), dt -> dt == DataType.INTEGER, sourceText(), SECOND, "integer"));
115119
if (typeResolution.unresolved()) {
116120
return typeResolution;
117121
}
118-
int limit = limitValue();
119-
if (limit <= 0) {
120-
return new TypeResolution(format(null, "Limit must be greater than 0 in [{}], found [{}]", sourceText(), limit));
122+
TypeResolution result = resolveTypeLimit(limitField(), sourceText(), forPreOptimizationValidation(limitField()));
123+
if (result.equals(TypeResolution.TYPE_RESOLVED) == false) {
124+
return result;
121125
}
122126
return TypeResolution.TYPE_RESOLVED;
123127
}
@@ -164,11 +168,15 @@ Expression limitField() {
164168
}
165169

166170
private int limitValue() {
167-
return (int) limitField().fold(FoldContext.small() /* TODO remove me */);
171+
return FunctionUtils.limitValue(limitField(), sourceText());
168172
}
169173

170174
Expression uuid() {
171175
return parameters().get(1);
172176
}
173177

178+
@Override
179+
public void postOptimizationVerification(Failures failures) {
180+
FunctionUtils.resolveTypeLimit(limitField(), sourceText(), forPostOptimizationValidation(limitField(), failures));
181+
}
174182
}

0 commit comments

Comments
 (0)