Skip to content

Commit 09bbf06

Browse files
julian-elasticjulian-elastic
committed
Address code review comments
1 parent 5012a2e commit 09bbf06

File tree

9 files changed

+65
-59
lines changed

9 files changed

+65
-59
lines changed

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

Lines changed: 30 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -19,35 +19,43 @@
1919

2020
public class FunctionUtils {
2121
/**
22-
* A utility class to validate the type resolution of expressions before and after folding.
22+
* A utility class to validate the type resolution of expressions before and after logical planning.
2323
* 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
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
2727
* {@link org.elasticsearch.xpack.esql.capabilities.PostOptimizationVerificationAware} verification
2828
*/
2929
public static class TypeResolutionValidator {
3030

3131
Expression.TypeResolution typeResolution = Expression.TypeResolution.TYPE_RESOLVED;
3232
@Nullable
33-
private final Failures postFoldingFailures; // null means we are doing pre-folding validation only
33+
private final Failures postValidationFailures; // null means we are doing pre-folding validation only
3434
private final Expression field;
3535

36-
public TypeResolutionValidator(Expression field, Failures failures) {
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) {
3745
this.field = field;
38-
this.postFoldingFailures = failures;
46+
this.postValidationFailures = failures;
3947
}
4048

41-
public void reportPostFoldingFailure(Failure failure) {
42-
if (postFoldingFailures != null) {
43-
postFoldingFailures.add(failure);
49+
public void invalidIfPostValidation(Failure failure) {
50+
if (postValidationFailures != null) {
51+
postValidationFailures.add(failure);
4452
}
4553
}
4654

47-
public void reportPreFoldingFailure(Expression.TypeResolution message) {
55+
public void invalid(Expression.TypeResolution message) {
4856
typeResolution = message;
49-
if (postFoldingFailures != null) {
50-
postFoldingFailures.add(fail(field, message.message()));
57+
if (postValidationFailures != null) {
58+
postValidationFailures.add(fail(field, message.message()));
5159
}
5260
}
5361

@@ -70,31 +78,30 @@ public static Integer limitValue(Expression limitField, String sourceText) {
7078
* We check that the limit is not null and that if it is a literal, it is a positive integer
7179
* During postOptimizationVerification folding is already done, so we also verify that it is definitively a literal
7280
*/
73-
public static Expression.TypeResolution resolveTypeLimit(Expression limitField, String sourceText, Failures failures) {
74-
TypeResolutionValidator validator = new TypeResolutionValidator(limitField, failures);
81+
public static Expression.TypeResolution resolveTypeLimit(Expression limitField, String sourceText, TypeResolutionValidator validator) {
7582
if (limitField == null) {
76-
validator.reportPreFoldingFailure(
83+
validator.invalid(
7784
new Expression.TypeResolution(format(null, "Limit must be a constant integer in [{}], found [{}]", sourceText, limitField))
7885
);
7986
} else if (limitField instanceof Literal literal) {
8087
if (literal.value() == null) {
81-
validator.reportPreFoldingFailure(
88+
validator.invalid(
8289
new Expression.TypeResolution(
8390
format(null, "Limit must be a constant integer in [{}], found [{}]", sourceText, limitField)
8491
)
8592
);
8693
} else {
8794
int value = (Integer) literal.value();
8895
if (value <= 0) {
89-
validator.reportPreFoldingFailure(
96+
validator.invalid(
9097
new Expression.TypeResolution(format(null, "Limit must be greater than 0 in [{}], found [{}]", sourceText, value))
9198
);
9299
}
93100
}
94101
} else {
95102
// it is expected that the expression is a literal after folding
96103
// we fail if it is not a literal
97-
validator.reportPostFoldingFailure(
104+
validator.invalidIfPostValidation(
98105
fail(limitField, "Limit must be a constant integer in [{}], found [{}]", sourceText, limitField)
99106
);
100107
}
@@ -105,24 +112,21 @@ public static Expression.TypeResolution resolveTypeLimit(Expression limitField,
105112
* We check that the query is not null and that if it is a literal, it is a string
106113
* During postOptimizationVerification folding is already done, so we also verify that it is definitively a literal
107114
*/
108-
public static Expression.TypeResolution resolveTypeQuery(Expression queryField, String sourceText, Failures failures) {
109-
TypeResolutionValidator validator = new TypeResolutionValidator(queryField, failures);
115+
public static Expression.TypeResolution resolveTypeQuery(Expression queryField, String sourceText, TypeResolutionValidator validator) {
110116
if (queryField == null) {
111-
validator.reportPreFoldingFailure(
117+
validator.invalid(
112118
new Expression.TypeResolution(format(null, "Query must be a valid string in [{}], found [{}]", sourceText, queryField))
113119
);
114120
} else if (queryField instanceof Literal literal) {
115121
if (literal.value() == null) {
116-
validator.reportPreFoldingFailure(
122+
validator.invalid(
117123
new Expression.TypeResolution(format(null, "Query value cannot be null in [{}], but got [{}]", sourceText, queryField))
118124
);
119125
}
120126
} else {
121127
// it is expected that the expression is a literal after folding
122128
// we fail if it is not a literal
123-
validator.reportPostFoldingFailure(
124-
fail(queryField, "Query must be a valid string in [{}], found [{}]", sourceText, queryField)
125-
);
129+
validator.invalidIfPostValidation(fail(queryField, "Query must be a valid string in [{}], found [{}]", sourceText, queryField));
126130
}
127131
return validator.getResolvedType();
128132
}

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

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,8 @@
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.TypeResolutionValidator.forPostOptimizationValidation;
43+
import static org.elasticsearch.xpack.esql.expression.function.FunctionUtils.TypeResolutionValidator.forPreOptimizationValidation;
4244
import static org.elasticsearch.xpack.esql.expression.function.FunctionUtils.resolveTypeLimit;
4345

4446
public class Sample extends AggregateFunction implements ToAggregator, PostOptimizationVerificationAware {
@@ -117,7 +119,7 @@ protected TypeResolution resolveType() {
117119
if (typeResolution.unresolved()) {
118120
return typeResolution;
119121
}
120-
TypeResolution result = resolveTypeLimit(limitField(), sourceText(), null);
122+
TypeResolution result = resolveTypeLimit(limitField(), sourceText(), forPreOptimizationValidation(limitField()));
121123
if (result.equals(TypeResolution.TYPE_RESOLVED) == false) {
122124
return result;
123125
}
@@ -175,6 +177,6 @@ Expression uuid() {
175177

176178
@Override
177179
public void postOptimizationVerification(Failures failures) {
178-
FunctionUtils.resolveTypeLimit(limitField(), sourceText(), failures);
180+
FunctionUtils.resolveTypeLimit(limitField(), sourceText(), forPostOptimizationValidation(limitField(), failures));
179181
}
180182
}

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

Lines changed: 17 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@
3232
import org.elasticsearch.xpack.esql.expression.function.FunctionInfo;
3333
import org.elasticsearch.xpack.esql.expression.function.FunctionType;
3434
import org.elasticsearch.xpack.esql.expression.function.FunctionUtils;
35+
import org.elasticsearch.xpack.esql.expression.function.FunctionUtils.TypeResolutionValidator;
3536
import org.elasticsearch.xpack.esql.expression.function.Param;
3637
import org.elasticsearch.xpack.esql.io.stream.PlanStreamInput;
3738
import org.elasticsearch.xpack.esql.planner.ToAggregator;
@@ -48,6 +49,8 @@
4849
import static org.elasticsearch.xpack.esql.core.expression.TypeResolutions.isNotNull;
4950
import static org.elasticsearch.xpack.esql.core.expression.TypeResolutions.isString;
5051
import static org.elasticsearch.xpack.esql.core.expression.TypeResolutions.isType;
52+
import static org.elasticsearch.xpack.esql.expression.function.FunctionUtils.TypeResolutionValidator.forPostOptimizationValidation;
53+
import static org.elasticsearch.xpack.esql.expression.function.FunctionUtils.TypeResolutionValidator.forPreOptimizationValidation;
5154

5255
public class Top extends AggregateFunction implements ToAggregator, SurrogateExpression, PostOptimizationVerificationAware {
5356
public static final NamedWriteableRegistry.Entry ENTRY = new NamedWriteableRegistry.Entry(Expression.class, "Top", Top::new);
@@ -166,7 +169,7 @@ protected TypeResolution resolveType() {
166169
if (result.equals(TypeResolution.TYPE_RESOLVED) == false) {
167170
return result;
168171
}
169-
result = resolveTypeOrder(null);
172+
result = resolveTypeOrder(forPreOptimizationValidation(orderField()));
170173
if (result.equals(TypeResolution.TYPE_RESOLVED) == false) {
171174
return result;
172175
}
@@ -178,23 +181,20 @@ protected TypeResolution resolveType() {
178181
* During postOptimizationVerification folding is already done, so we also verify that it is definitively a literal
179182
*/
180183
private TypeResolution resolveTypeLimit() {
181-
return FunctionUtils.resolveTypeLimit(limitField(), sourceText(), null);
184+
return FunctionUtils.resolveTypeLimit(limitField(), sourceText(), forPreOptimizationValidation(limitField()));
182185
}
183186

184187
/**
185188
* 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".
186189
* During postOptimizationVerification folding is already done, so we also verify that it is definitively a literal
187190
*/
188-
private Expression.TypeResolution resolveTypeOrder(Failures failures) {
189-
FunctionUtils.TypeResolutionValidator validator = new FunctionUtils.TypeResolutionValidator(orderField(), failures);
191+
private Expression.TypeResolution resolveTypeOrder(TypeResolutionValidator validator) {
190192
Expression order = orderField();
191193
if (order == null) {
192-
validator.reportPreFoldingFailure(
193-
new TypeResolution(format(null, "Order must be a valid string in [{}], found [{}]", sourceText(), order))
194-
);
194+
validator.invalid(new TypeResolution(format(null, "Order must be a valid string in [{}], found [{}]", sourceText(), order)));
195195
} else if (order instanceof Literal literal) {
196196
if (literal.value() == null) {
197-
validator.reportPreFoldingFailure(
197+
validator.invalid(
198198
new TypeResolution(
199199
format(
200200
null,
@@ -209,7 +209,7 @@ private Expression.TypeResolution resolveTypeOrder(Failures failures) {
209209
} else {
210210
String value = BytesRefs.toString(literal.value());
211211
if (value == null || value.equalsIgnoreCase(ORDER_ASC) == false && value.equalsIgnoreCase(ORDER_DESC) == false) {
212-
validator.reportPreFoldingFailure(
212+
validator.invalid(
213213
new TypeResolution(
214214
format(
215215
null,
@@ -226,7 +226,7 @@ private Expression.TypeResolution resolveTypeOrder(Failures failures) {
226226
} else {
227227
// it is expected that the expression is a literal after folding
228228
// we fail if it is not a literal
229-
validator.reportPostFoldingFailure(fail(order, "Order must be a valid string in [{}], found [{}]", sourceText(), order));
229+
validator.invalidIfPostValidation(fail(order, "Order must be a valid string in [{}], found [{}]", sourceText(), order));
230230
}
231231
return validator.getResolvedType();
232232
}
@@ -238,11 +238,11 @@ public void postOptimizationVerification(Failures failures) {
238238
}
239239

240240
private void postOptimizationVerificationLimit(Failures failures) {
241-
FunctionUtils.resolveTypeLimit(limitField(), sourceText(), failures);
241+
FunctionUtils.resolveTypeLimit(limitField(), sourceText(), forPostOptimizationValidation(limitField(), failures));
242242
}
243243

244244
private void postOptimizationVerificationOrder(Failures failures) {
245-
resolveTypeOrder(failures);
245+
resolveTypeOrder(forPostOptimizationValidation(orderField(), failures));
246246
}
247247

248248
@Override
@@ -287,19 +287,12 @@ public AggregatorFunctionSupplier supplier() {
287287
@Override
288288
public Expression surrogate() {
289289
var s = source();
290-
try {
291-
if (limitValue() == 1) {
292-
if (orderValue()) {
293-
return new Min(s, field());
294-
} else {
295-
return new Max(s, field());
296-
}
290+
if (orderField() instanceof Literal && limitField() instanceof Literal && limitValue() == 1) {
291+
if (orderValue()) {
292+
return new Min(s, field());
293+
} else {
294+
return new Max(s, field());
297295
}
298-
} catch (EsqlIllegalArgumentException e) {
299-
// If the limit is not a literal or is not a positive integer, we cannot create a surrogate
300-
// so we return null to indicate that no surrogate can be created.
301-
// This is possible if the limit is an expression, and folding has not been done yet.
302-
return null;
303296
}
304297
return null;
305298
}

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

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,8 @@
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.TypeResolutionValidator.forPostOptimizationValidation;
60+
import static org.elasticsearch.xpack.esql.expression.function.FunctionUtils.TypeResolutionValidator.forPreOptimizationValidation;
5961
import static org.elasticsearch.xpack.esql.expression.function.FunctionUtils.resolveTypeQuery;
6062

6163
/**
@@ -113,7 +115,7 @@ protected TypeResolution resolveQuery(TypeResolutions.ParamOrdinal queryOrdinal)
113115
if (result.unresolved()) {
114116
return result;
115117
}
116-
result = resolveTypeQuery(query(), sourceText(), null);
118+
result = resolveTypeQuery(query(), sourceText(), forPreOptimizationValidation(query()));
117119
if (result.equals(TypeResolution.TYPE_RESOLVED) == false) {
118120
return result;
119121
}
@@ -419,6 +421,6 @@ public static FieldAttribute fieldAsFieldAttribute(Expression field) {
419421

420422
@Override
421423
public void postOptimizationVerification(Failures failures) {
422-
resolveTypeQuery(query(), sourceText(), failures);
424+
resolveTypeQuery(query(), sourceText(), forPostOptimizationValidation(query(), failures));
423425
}
424426
}

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -79,6 +79,7 @@
7979
import static org.elasticsearch.xpack.esql.core.type.DataType.TEXT;
8080
import static org.elasticsearch.xpack.esql.core.type.DataType.UNSIGNED_LONG;
8181
import static org.elasticsearch.xpack.esql.core.type.DataType.VERSION;
82+
import static org.elasticsearch.xpack.esql.expression.function.FunctionUtils.TypeResolutionValidator.forPreOptimizationValidation;
8283
import static org.elasticsearch.xpack.esql.expression.function.FunctionUtils.resolveTypeQuery;
8384
import static org.elasticsearch.xpack.esql.expression.predicate.operator.comparison.EsqlBinaryComparison.formatIncompatibleTypesMessage;
8485

@@ -327,7 +328,7 @@ private TypeResolution resolveQuery() {
327328
if (result.unresolved()) {
328329
return result;
329330
}
330-
result = resolveTypeQuery(query(), sourceText(), null);
331+
result = resolveTypeQuery(query(), sourceText(), forPreOptimizationValidation(query()));
331332
if (result.equals(TypeResolution.TYPE_RESOLVED) == false) {
332333
return result;
333334
}

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,7 @@
6262
import static org.elasticsearch.xpack.esql.core.type.DataType.INTEGER;
6363
import static org.elasticsearch.xpack.esql.core.type.DataType.KEYWORD;
6464
import static org.elasticsearch.xpack.esql.core.type.DataType.TEXT;
65+
import static org.elasticsearch.xpack.esql.expression.function.FunctionUtils.TypeResolutionValidator.forPreOptimizationValidation;
6566
import static org.elasticsearch.xpack.esql.expression.function.FunctionUtils.resolveTypeQuery;
6667

6768
/**
@@ -202,7 +203,7 @@ private TypeResolution resolveQuery() {
202203
if (result.unresolved()) {
203204
return result;
204205
}
205-
result = resolveTypeQuery(query(), sourceText(), null);
206+
result = resolveTypeQuery(query(), sourceText(), forPreOptimizationValidation(query()));
206207
if (result.equals(TypeResolution.TYPE_RESOLVED) == false) {
207208
return result;
208209
}

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,7 @@
8080
import static org.elasticsearch.xpack.esql.core.type.DataType.TEXT;
8181
import static org.elasticsearch.xpack.esql.core.type.DataType.UNSIGNED_LONG;
8282
import static org.elasticsearch.xpack.esql.core.type.DataType.VERSION;
83+
import static org.elasticsearch.xpack.esql.expression.function.FunctionUtils.TypeResolutionValidator.forPreOptimizationValidation;
8384
import static org.elasticsearch.xpack.esql.expression.function.FunctionUtils.resolveTypeQuery;
8485

8586
/**
@@ -428,7 +429,7 @@ private TypeResolution resolveQuery() {
428429
if (result.unresolved()) {
429430
return result;
430431
}
431-
result = resolveTypeQuery(query(), sourceText(), null);
432+
result = resolveTypeQuery(query(), sourceText(), forPreOptimizationValidation(query()));
432433
if (result.equals(TypeResolution.TYPE_RESOLVED) == false) {
433434
return result;
434435
}

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,7 @@
7070
import static org.elasticsearch.xpack.esql.core.type.DataType.INTEGER;
7171
import static org.elasticsearch.xpack.esql.core.type.DataType.KEYWORD;
7272
import static org.elasticsearch.xpack.esql.core.type.DataType.TEXT;
73+
import static org.elasticsearch.xpack.esql.expression.function.FunctionUtils.TypeResolutionValidator.forPreOptimizationValidation;
7374
import static org.elasticsearch.xpack.esql.expression.function.FunctionUtils.resolveTypeQuery;
7475

7576
/**
@@ -319,7 +320,7 @@ private TypeResolution resolveQuery() {
319320
if (result.unresolved()) {
320321
return result;
321322
}
322-
result = resolveTypeQuery(query(), sourceText(), null);
323+
result = resolveTypeQuery(query(), sourceText(), forPreOptimizationValidation(query()));
323324
if (result.equals(TypeResolution.TYPE_RESOLVED) == false) {
324325
return result;
325326
}

0 commit comments

Comments
 (0)