Skip to content

Commit 094aa80

Browse files
julian-elasticjulian-elastic
committed
Address code review feedback
1 parent 6714b8b commit 094aa80

File tree

9 files changed

+129
-90
lines changed

9 files changed

+129
-90
lines changed

‎x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/FunctionUtils.java‎

Lines changed: 76 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,9 @@
77
package org.elasticsearch.xpack.esql.expression.function;
88

99
import org.elasticsearch.common.lucene.BytesRefs;
10+
import org.elasticsearch.core.Nullable;
1011
import org.elasticsearch.xpack.esql.EsqlIllegalArgumentException;
12+
import org.elasticsearch.xpack.esql.common.Failure;
1113
import org.elasticsearch.xpack.esql.common.Failures;
1214
import org.elasticsearch.xpack.esql.core.expression.Expression;
1315
import org.elasticsearch.xpack.esql.core.expression.Literal;
@@ -16,6 +18,43 @@
1618
import static org.elasticsearch.xpack.esql.common.Failure.fail;
1719

1820
public class FunctionUtils {
21+
/**
22+
* A utility class to validate the type resolution of expressions before and after folding.
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-folding validation.
25+
* If a {@link Failures} instance is passed, it means we are doing post-folding validation as well.
26+
* This is usually called after folding is done, from during {@link org.elasticsearch.xpack.esql.capabilities.PostOptimizationVerificationAware} verification
27+
*/
28+
public static class TypeResolutionValidator {
29+
30+
Expression.TypeResolution typeResolution = Expression.TypeResolution.TYPE_RESOLVED;
31+
@Nullable
32+
private final Failures postFoldingFailures; // null means we are doing pre-folding validation only
33+
private final Expression field;
34+
35+
public TypeResolutionValidator(Expression field, Failures failures) {
36+
this.field = field;
37+
this.postFoldingFailures = failures;
38+
}
39+
40+
public void reportPostFoldingFailure(Failure failure) {
41+
if (postFoldingFailures != null) {
42+
postFoldingFailures.add(failure);
43+
}
44+
}
45+
46+
public void reportPreFoldingFailure(Expression.TypeResolution message) {
47+
typeResolution = message;
48+
if (postFoldingFailures != null) {
49+
postFoldingFailures.add(fail(field, message.message()));
50+
}
51+
}
52+
53+
public Expression.TypeResolution getResolvedType() {
54+
return typeResolution;
55+
}
56+
}
57+
1958
public static Integer limitValue(Expression limitField, String sourceText) {
2059
if (limitField instanceof Literal literal) {
2160
Object value = literal.value();
@@ -28,71 +67,63 @@ public static Integer limitValue(Expression limitField, String sourceText) {
2867

2968
/**
3069
* We check that the limit is not null and that if it is a literal, it is a positive integer
31-
* We will do a more thorough check in the postOptimizationVerification once folding is done.
70+
* During postOptimizationVerification folding is already done, so we also verify that it is definitively a literal
3271
*/
33-
public static Expression.TypeResolution resolveTypeLimit(Expression limitField, String sourceText) {
72+
public static Expression.TypeResolution resolveTypeLimit(Expression limitField, String sourceText, Failures failures) {
73+
TypeResolutionValidator validator = new TypeResolutionValidator(limitField, failures);
3474
if (limitField == null) {
35-
return new Expression.TypeResolution(
36-
format(null, "Limit must be a constant integer in [{}], found [{}]", sourceText, limitField)
75+
validator.reportPreFoldingFailure(
76+
new Expression.TypeResolution(format(null, "Limit must be a constant integer in [{}], found [{}]", sourceText, limitField))
3777
);
38-
}
39-
if (limitField instanceof Literal literal) {
78+
} else if (limitField instanceof Literal literal) {
4079
if (literal.value() == null) {
41-
return new Expression.TypeResolution(
42-
format(null, "Limit must be a constant integer in [{}], found [{}]", sourceText, limitField)
80+
validator.reportPreFoldingFailure(
81+
new Expression.TypeResolution(
82+
format(null, "Limit must be a constant integer in [{}], found [{}]", sourceText, limitField)
83+
)
4384
);
44-
}
45-
int value = (Integer) literal.value();
46-
if (value <= 0) {
47-
return new Expression.TypeResolution(format(null, "Limit must be greater than 0 in [{}], found [{}]", sourceText, value));
48-
}
49-
}
50-
return Expression.TypeResolution.TYPE_RESOLVED;
51-
}
52-
53-
public static void postOptimizationVerificationLimit(Failures failures, Expression limitField, String sourceText) {
54-
if (limitField == null) {
55-
failures.add(fail(limitField, "Limit must be a constant integer in [{}], found [{}]", sourceText, limitField));
56-
}
57-
if (limitField instanceof Literal literal) {
58-
int value = (Integer) literal.value();
59-
if (value <= 0) {
60-
failures.add(fail(limitField, "Limit must be greater than 0 in [{}], found [{}]", sourceText, value));
85+
} else {
86+
int value = (Integer) literal.value();
87+
if (value <= 0) {
88+
validator.reportPreFoldingFailure(
89+
new Expression.TypeResolution(format(null, "Limit must be greater than 0 in [{}], found [{}]", sourceText, value))
90+
);
91+
}
6192
}
6293
} else {
6394
// it is expected that the expression is a literal after folding
6495
// we fail if it is not a literal
65-
failures.add(fail(limitField, "Limit must be a constant integer in [{}], found [{}]", sourceText, limitField));
96+
validator.reportPostFoldingFailure(
97+
fail(limitField, "Limit must be a constant integer in [{}], found [{}]", sourceText, limitField)
98+
);
6699
}
100+
return validator.getResolvedType();
67101
}
68102

69-
public static Expression.TypeResolution resolveTypeQuery(Expression queryField, String sourceText) {
103+
/**
104+
* We check that the query is not null and that if it is a literal, it is a string
105+
* During postOptimizationVerification folding is already done, so we also verify that it is definitively a literal
106+
*/
107+
public static Expression.TypeResolution resolveTypeQuery(Expression queryField, String sourceText, Failures failures) {
108+
TypeResolutionValidator validator = new TypeResolutionValidator(queryField, failures);
70109
if (queryField == null) {
71-
return new Expression.TypeResolution(format(null, "Query must be a valid string in [{}], found [{}]", sourceText, queryField));
72-
}
73-
if (queryField instanceof Literal literal) {
110+
validator.reportPreFoldingFailure(
111+
new Expression.TypeResolution(format(null, "Query must be a valid string in [{}], found [{}]", sourceText, queryField))
112+
);
113+
} else if (queryField instanceof Literal literal) {
74114
if (literal.value() == null) {
75-
return new Expression.TypeResolution(
76-
format(null, "Query value cannot be null in [{}], but got [{}]", sourceText, queryField)
115+
validator.reportPreFoldingFailure(
116+
new Expression.TypeResolution(format(null, "Query value cannot be null in [{}], but got [{}]", sourceText, queryField))
77117
);
78118
}
79-
}
80-
return Expression.TypeResolution.TYPE_RESOLVED;
81-
}
82-
83-
public static void postOptimizationVerificationQuery(Failures failures, Expression queryField, String sourceText) {
84-
if (queryField == null) {
85-
failures.add(fail(queryField, "Query must be a valid string in [{}], found [{}]", sourceText, queryField));
86-
}
87-
if (queryField instanceof Literal literal) {
88-
if (literal.value() == null) {
89-
failures.add(fail(queryField, "Invalid query value in [{}], found [{}]", sourceText, literal.value()));
90-
}
91119
} else {
92120
// it is expected that the expression is a literal after folding
93121
// we fail if it is not a literal
94-
failures.add(fail(queryField, "Query must be a valid string in [{}], found [{}]", sourceText, queryField));
122+
validator.reportPostFoldingFailure(
123+
fail(queryField, "Query must be a valid string in [{}], found [{}]", sourceText, queryField)
124+
);
95125
}
126+
return validator.getResolvedType();
96127
}
97128

98129
public static Object queryAsObject(Expression queryField, String sourceText) {

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

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,6 @@
3939
import static org.elasticsearch.xpack.esql.core.expression.TypeResolutions.ParamOrdinal.SECOND;
4040
import static org.elasticsearch.xpack.esql.core.expression.TypeResolutions.isNotNull;
4141
import static org.elasticsearch.xpack.esql.core.expression.TypeResolutions.isType;
42-
import static org.elasticsearch.xpack.esql.expression.function.FunctionUtils.postOptimizationVerificationLimit;
4342
import static org.elasticsearch.xpack.esql.expression.function.FunctionUtils.resolveTypeLimit;
4443

4544
public class Sample extends AggregateFunction implements ToAggregator, PostOptimizationVerificationAware {
@@ -118,7 +117,7 @@ protected TypeResolution resolveType() {
118117
if (typeResolution.unresolved()) {
119118
return typeResolution;
120119
}
121-
TypeResolution result = resolveTypeLimit(limitField(), sourceText());
120+
TypeResolution result = resolveTypeLimit(limitField(), sourceText(), null);
122121
if (result.equals(TypeResolution.TYPE_RESOLVED) == false) {
123122
return result;
124123
}
@@ -176,6 +175,6 @@ Expression uuid() {
176175

177176
@Override
178177
public void postOptimizationVerification(Failures failures) {
179-
postOptimizationVerificationLimit(failures, limitField(), sourceText());
178+
FunctionUtils.resolveTypeLimit(limitField(), sourceText(), failures);
180179
}
181180
}

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

Lines changed: 44 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -166,7 +166,7 @@ protected TypeResolution resolveType() {
166166
if (result.equals(TypeResolution.TYPE_RESOLVED) == false) {
167167
return result;
168168
}
169-
result = resolveTypeOrder();
169+
result = resolveTypeOrder(null);
170170
if (result.equals(TypeResolution.TYPE_RESOLVED) == false) {
171171
return result;
172172
}
@@ -175,35 +175,60 @@ protected TypeResolution resolveType() {
175175

176176
/**
177177
* We check that the limit is not null and that if it is a literal, it is a positive integer
178-
* We will do a more thorough check in the postOptimizationVerification once folding is done.
178+
* During postOptimizationVerification folding is already done, so we also verify that it is definitively a literal
179179
*/
180180
private TypeResolution resolveTypeLimit() {
181-
return FunctionUtils.resolveTypeLimit(limitField(), sourceText());
181+
return FunctionUtils.resolveTypeLimit(limitField(), sourceText(), null);
182182
}
183183

184184
/**
185185
* We check that the order is not null and that if it is a literal, it is one of the two valid values: "asc" or "desc".
186-
* We will do a more thorough check in the postOptimizationVerification once folding is done.
186+
* During postOptimizationVerification folding is already done, so we also verify that it is definitively a literal
187187
*/
188-
private TypeResolution resolveTypeOrder() {
188+
private Expression.TypeResolution resolveTypeOrder(Failures failures) {
189+
FunctionUtils.TypeResolutionValidator validator = new FunctionUtils.TypeResolutionValidator(orderField(), failures);
189190
Expression order = orderField();
190191
if (order == null) {
191-
return new TypeResolution(format(null, "Order must be a valid string in [{}], found [{}]", sourceText(), order));
192-
}
193-
if (order instanceof Literal literal) {
192+
validator.reportPreFoldingFailure(
193+
new TypeResolution(format(null, "Order must be a valid string in [{}], found [{}]", sourceText(), order))
194+
);
195+
} else if (order instanceof Literal literal) {
194196
if (literal.value() == null) {
195-
return new TypeResolution(
196-
format(null, "Invalid order value in [{}], expected [{}, {}] but got [{}]", sourceText(), ORDER_ASC, ORDER_DESC, order)
197-
);
198-
}
199-
String value = BytesRefs.toString(literal.value());
200-
if (value == null || value.equalsIgnoreCase(ORDER_ASC) == false && value.equalsIgnoreCase(ORDER_DESC) == false) {
201-
return new TypeResolution(
202-
format(null, "Invalid order value in [{}], expected [{}, {}] but got [{}]", sourceText(), ORDER_ASC, ORDER_DESC, order)
197+
validator.reportPreFoldingFailure(
198+
new TypeResolution(
199+
format(
200+
null,
201+
"Invalid order value in [{}], expected [{}, {}] but got [{}]",
202+
sourceText(),
203+
ORDER_ASC,
204+
ORDER_DESC,
205+
order
206+
)
207+
)
203208
);
209+
} else {
210+
String value = BytesRefs.toString(literal.value());
211+
if (value == null || value.equalsIgnoreCase(ORDER_ASC) == false && value.equalsIgnoreCase(ORDER_DESC) == false) {
212+
validator.reportPreFoldingFailure(
213+
new TypeResolution(
214+
format(
215+
null,
216+
"Invalid order value in [{}], expected [{}, {}] but got [{}]",
217+
sourceText(),
218+
ORDER_ASC,
219+
ORDER_DESC,
220+
order
221+
)
222+
)
223+
);
224+
}
204225
}
226+
} else {
227+
// it is expected that the expression is a literal after folding
228+
// we fail if it is not a literal
229+
validator.reportPostFoldingFailure(fail(order, "Order must be a valid string in [{}], found [{}]", sourceText(), order));
205230
}
206-
return TypeResolution.TYPE_RESOLVED;
231+
return validator.getResolvedType();
207232
}
208233

209234
@Override
@@ -213,26 +238,11 @@ public void postOptimizationVerification(Failures failures) {
213238
}
214239

215240
private void postOptimizationVerificationLimit(Failures failures) {
216-
FunctionUtils.postOptimizationVerificationLimit(failures, limitField(), sourceText());
241+
FunctionUtils.resolveTypeLimit(limitField(), sourceText(), failures);
217242
}
218243

219244
private void postOptimizationVerificationOrder(Failures failures) {
220-
Expression order = orderField();
221-
if (order == null) {
222-
failures.add(fail(order, "Order must be a valid string in [{}], found [{}]", sourceText(), order));
223-
}
224-
if (order instanceof Literal literal) {
225-
String value = BytesRefs.toString(literal.value());
226-
if (value == null || value.equalsIgnoreCase(ORDER_ASC) == false && value.equalsIgnoreCase(ORDER_DESC) == false) {
227-
failures.add(
228-
fail(order, "Invalid order value in [{}], expected [{}, {}] but got [{}]", sourceText(), ORDER_ASC, ORDER_DESC, order)
229-
);
230-
}
231-
} else {
232-
// it is expected that the expression is a literal after folding
233-
// we fail if it is not a literal
234-
failures.add(fail(order, "Order must be a valid string in [{}], found [{}]", sourceText(), order));
235-
}
245+
resolveTypeOrder(failures);
236246
}
237247

238248
@Override

‎x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/fulltext/FullTextFunction.java‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,6 @@
5656
import static org.elasticsearch.xpack.esql.core.expression.TypeResolutions.ParamOrdinal.DEFAULT;
5757
import static org.elasticsearch.xpack.esql.core.expression.TypeResolutions.isNotNull;
5858
import static org.elasticsearch.xpack.esql.core.expression.TypeResolutions.isString;
59-
import static org.elasticsearch.xpack.esql.expression.function.FunctionUtils.postOptimizationVerificationQuery;
6059
import static org.elasticsearch.xpack.esql.expression.function.FunctionUtils.resolveTypeQuery;
6160

6261
/**
@@ -114,7 +113,7 @@ protected TypeResolution resolveQuery(TypeResolutions.ParamOrdinal queryOrdinal)
114113
if (result.unresolved()) {
115114
return result;
116115
}
117-
result = resolveTypeQuery(query(), sourceText());
116+
result = resolveTypeQuery(query(), sourceText(), null);
118117
if (result.equals(TypeResolution.TYPE_RESOLVED) == false) {
119118
return result;
120119
}
@@ -420,6 +419,6 @@ public static FieldAttribute fieldAsFieldAttribute(Expression field) {
420419

421420
@Override
422421
public void postOptimizationVerification(Failures failures) {
423-
postOptimizationVerificationQuery(failures, query(), sourceText());
422+
resolveTypeQuery(query(), sourceText(), failures);
424423
}
425424
}

‎x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/fulltext/Match.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -327,7 +327,7 @@ private TypeResolution resolveQuery() {
327327
if (result.unresolved()) {
328328
return result;
329329
}
330-
result = resolveTypeQuery(query(), sourceText());
330+
result = resolveTypeQuery(query(), sourceText(), null);
331331
if (result.equals(TypeResolution.TYPE_RESOLVED) == false) {
332332
return result;
333333
}

‎x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/fulltext/MatchPhrase.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -202,7 +202,7 @@ private TypeResolution resolveQuery() {
202202
if (result.unresolved()) {
203203
return result;
204204
}
205-
result = resolveTypeQuery(query(), sourceText());
205+
result = resolveTypeQuery(query(), sourceText(), null);
206206
if (result.equals(TypeResolution.TYPE_RESOLVED) == false) {
207207
return result;
208208
}

‎x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/fulltext/MultiMatch.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -428,7 +428,7 @@ private TypeResolution resolveQuery() {
428428
if (result.unresolved()) {
429429
return result;
430430
}
431-
result = resolveTypeQuery(query(), sourceText());
431+
result = resolveTypeQuery(query(), sourceText(), null);
432432
if (result.equals(TypeResolution.TYPE_RESOLVED) == false) {
433433
return result;
434434
}

‎x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/fulltext/QueryString.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -319,7 +319,7 @@ private TypeResolution resolveQuery() {
319319
if (result.unresolved()) {
320320
return result;
321321
}
322-
result = resolveTypeQuery(query(), sourceText());
322+
result = resolveTypeQuery(query(), sourceText(), null);
323323
if (result.equals(TypeResolution.TYPE_RESOLVED) == false) {
324324
return result;
325325
}

‎x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/vector/Knn.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -217,7 +217,7 @@ private TypeResolution resolveQuery() {
217217
if (result.unresolved()) {
218218
return result;
219219
}
220-
result = resolveTypeQuery(query(), sourceText());
220+
result = resolveTypeQuery(query(), sourceText(), null);
221221
if (result.equals(TypeResolution.TYPE_RESOLVED) == false) {
222222
return result;
223223
}

0 commit comments

Comments
 (0)