Skip to content

Commit 324f243

Browse files
carlosdelestcarlosdelest
committed
Don't allow disjunctions to be used with scoring
1 parent ad575b5 commit 324f243

File tree

3 files changed

+107
-41
lines changed

3 files changed

+107
-41
lines changed

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

Lines changed: 61 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,18 +16,21 @@
1616
import org.elasticsearch.xpack.esql.common.Failures;
1717
import org.elasticsearch.xpack.esql.core.expression.Expression;
1818
import org.elasticsearch.xpack.esql.core.expression.FoldContext;
19+
import org.elasticsearch.xpack.esql.core.expression.MetadataAttribute;
1920
import org.elasticsearch.xpack.esql.core.expression.Nullability;
2021
import org.elasticsearch.xpack.esql.core.expression.TranslationAware;
2122
import org.elasticsearch.xpack.esql.core.expression.TypeResolutions;
2223
import org.elasticsearch.xpack.esql.core.expression.function.Function;
2324
import org.elasticsearch.xpack.esql.core.expression.predicate.logical.BinaryLogic;
2425
import org.elasticsearch.xpack.esql.core.expression.predicate.logical.Not;
26+
import org.elasticsearch.xpack.esql.core.expression.predicate.logical.Or;
2527
import org.elasticsearch.xpack.esql.core.planner.ExpressionTranslator;
2628
import org.elasticsearch.xpack.esql.core.planner.TranslatorHandler;
2729
import org.elasticsearch.xpack.esql.core.querydsl.query.Query;
2830
import org.elasticsearch.xpack.esql.core.querydsl.query.TranslationAwareExpressionQuery;
2931
import org.elasticsearch.xpack.esql.core.tree.Source;
3032
import org.elasticsearch.xpack.esql.core.type.DataType;
33+
import org.elasticsearch.xpack.esql.core.util.Holder;
3134
import org.elasticsearch.xpack.esql.evaluator.mapper.EvaluatorMapper;
3235
import org.elasticsearch.xpack.esql.plan.logical.Aggregate;
3336
import org.elasticsearch.xpack.esql.plan.logical.EsRelation;
@@ -222,15 +225,72 @@ private static void checkFullTextQueryFunctions(LogicalPlan plan, Failures failu
222225
m -> "[" + m.functionName() + "] " + m.functionType(),
223226
failures
224227
);
225-
// checkFullTextSearchDisjunctions(condition, ftf -> "[" + ftf.functionName() + "] " + ftf.functionType(), failures);
226228
checkFullTextFunctionsParents(condition, failures);
229+
230+
boolean usesScore = plan.output()
231+
.stream()
232+
.anyMatch(attr -> attr instanceof MetadataAttribute ma && ma.name().equals(MetadataAttribute.SCORE));
233+
if (usesScore) {
234+
checkFullTextSearchDisjunctions(condition, failures);
235+
}
227236
} else {
228237
plan.forEachExpression(FullTextFunction.class, ftf -> {
229238
failures.add(fail(ftf, "[{}] {} is only supported in WHERE commands", ftf.functionName(), ftf.functionType()));
230239
});
231240
}
232241
}
233242

243+
/**
244+
* Checks whether a condition contains a disjunction with a full text search.
245+
* If it does, check that every element of the disjunction is a full text search or combinations (AND, OR, NOT) of them.
246+
* If not, add a failure to the failures collection.
247+
*
248+
* @param condition condition to check for disjunctions of full text searches
249+
* @param failures failures collection to add to
250+
*/
251+
public static void checkFullTextSearchDisjunctions(Expression condition, Failures failures) {
252+
Holder<Boolean> isInvalid = new Holder<>(false);
253+
condition.forEachDown(Or.class, or -> {
254+
if (isInvalid.get()) {
255+
// Exit early if we already have a failures
256+
return;
257+
}
258+
boolean hasFullText = or.anyMatch(FullTextFunction.class::isInstance);
259+
if (hasFullText) {
260+
boolean hasOnlyFullText = onlyFullTextFunctionsInExpression(or);
261+
if (hasOnlyFullText == false) {
262+
isInvalid.set(true);
263+
failures.add(
264+
fail(
265+
or,
266+
"Invalid condition when using METADATA _score [{}]. Full text functions can be used in an OR condition, "
267+
+ "but only if just full text functions are used in the OR condition",
268+
or.sourceText()
269+
)
270+
);
271+
}
272+
}
273+
});
274+
}
275+
276+
/**
277+
* Checks whether an expression contains just full text functions or negations (NOT) and combinations (AND, OR) of full text functions
278+
*
279+
* @param expression expression to check
280+
* @return true if all children are full text functions or negations of full text functions, false otherwise
281+
*/
282+
private static boolean onlyFullTextFunctionsInExpression(Expression expression) {
283+
if (expression instanceof FullTextFunction) {
284+
return true;
285+
} else if (expression instanceof Not) {
286+
return onlyFullTextFunctionsInExpression(expression.children().get(0));
287+
} else if (expression instanceof BinaryLogic binaryLogic) {
288+
return onlyFullTextFunctionsInExpression(binaryLogic.left()) && onlyFullTextFunctionsInExpression(binaryLogic.right());
289+
}
290+
291+
return false;
292+
}
293+
234294
/**
235295
* Checks all commands that exist before a specific type satisfy conditions.
236296
*

‎x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/physical/local/PushFiltersToSource.java‎

Lines changed: 4 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
package org.elasticsearch.xpack.esql.optimizer.rules.physical.local;
99

1010
import org.elasticsearch.index.query.QueryBuilder;
11+
import org.elasticsearch.xpack.esql.common.Failures;
1112
import org.elasticsearch.xpack.esql.core.expression.Alias;
1213
import org.elasticsearch.xpack.esql.core.expression.Attribute;
1314
import org.elasticsearch.xpack.esql.core.expression.AttributeMap;
@@ -21,7 +22,6 @@
2122
import org.elasticsearch.xpack.esql.core.expression.predicate.Range;
2223
import org.elasticsearch.xpack.esql.core.expression.predicate.logical.BinaryLogic;
2324
import org.elasticsearch.xpack.esql.core.expression.predicate.logical.Not;
24-
import org.elasticsearch.xpack.esql.core.expression.predicate.logical.Or;
2525
import org.elasticsearch.xpack.esql.core.expression.predicate.nulls.IsNotNull;
2626
import org.elasticsearch.xpack.esql.core.expression.predicate.nulls.IsNull;
2727
import org.elasticsearch.xpack.esql.core.expression.predicate.operator.comparison.BinaryComparison;
@@ -30,7 +30,6 @@
3030
import org.elasticsearch.xpack.esql.core.querydsl.query.Query;
3131
import org.elasticsearch.xpack.esql.core.type.DataType;
3232
import org.elasticsearch.xpack.esql.core.util.CollectionUtils;
33-
import org.elasticsearch.xpack.esql.core.util.Holder;
3433
import org.elasticsearch.xpack.esql.core.util.Queries;
3534
import org.elasticsearch.xpack.esql.expression.function.fulltext.FullTextFunction;
3635
import org.elasticsearch.xpack.esql.expression.function.scalar.ip.CIDRMatch;
@@ -281,39 +280,9 @@ public static boolean canPushSpatialFunctionToSource(BinarySpatialFunction s, Lu
281280
* @param condition condition to check for disjunctions of full text searches
282281
*/
283282
private static boolean checkPushableFullTextSearchFunctions(Expression condition) {
284-
Holder<Boolean> isPushable = new Holder<>(true);
285-
condition.forEachDown(Or.class, or -> {
286-
if (isPushable.get() == false) {
287-
// Exit early if we already have a failures
288-
return;
289-
}
290-
boolean hasFullText = or.anyMatch(FullTextFunction.class::isInstance);
291-
if (hasFullText) {
292-
boolean hasOnlyFullText = onlyFullTextFunctionsInExpression(or);
293-
if (hasOnlyFullText == false) {
294-
isPushable.set(false);
295-
}
296-
}
297-
});
298-
return isPushable.get();
299-
}
300-
301-
/**
302-
* Checks whether an expression contains just full text functions or negations (NOT) and combinations (AND, OR) of full text functions
303-
*
304-
* @param expression expression to check
305-
* @return true if all children are full text functions or negations of full text functions, false otherwise
306-
*/
307-
private static boolean onlyFullTextFunctionsInExpression(Expression expression) {
308-
if (expression instanceof FullTextFunction) {
309-
return true;
310-
} else if (expression instanceof Not) {
311-
return onlyFullTextFunctionsInExpression(expression.children().get(0));
312-
} else if (expression instanceof BinaryLogic binaryLogic) {
313-
return onlyFullTextFunctionsInExpression(binaryLogic.left()) && onlyFullTextFunctionsInExpression(binaryLogic.right());
314-
}
315-
316-
return false;
283+
Failures failures = new Failures();
284+
FullTextFunction.checkFullTextSearchDisjunctions(condition, failures);
285+
return failures.hasFailures() == false;
317286
}
318287

319288
private static boolean isPushableSpatialAttribute(Expression exp, LucenePushdownPredicates p) {

‎x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/VerifierTests.java‎

Lines changed: 42 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
package org.elasticsearch.xpack.esql.analysis;
99

1010
import org.elasticsearch.Build;
11+
import org.elasticsearch.common.logging.LoggerMessageFormat;
1112
import org.elasticsearch.test.ESTestCase;
1213
import org.elasticsearch.xpack.esql.VerificationException;
1314
import org.elasticsearch.xpack.esql.action.EsqlCapabilities;
@@ -1420,26 +1421,62 @@ private void checkWithDisjunctions(String functionName, String functionInvocatio
14201421
}
14211422

14221423
public void testFullTextFunctionsDisjunctions() {
1423-
checkWithFullTextFunctionsDisjunctions("MATCH", "match(last_name, \"Smith\")", "function");
1424-
checkWithFullTextFunctionsDisjunctions(":", "last_name : \"Smith\"", "operator");
1425-
checkWithFullTextFunctionsDisjunctions("QSTR", "qstr(\"last_name: Smith\")", "function");
1426-
checkWithFullTextFunctionsDisjunctions("KQL", "kql(\"last_name: Smith\")", "function");
1424+
checkWithFullTextFunctionsDisjunctions("match(last_name, \"Smith\")");
1425+
checkWithFullTextFunctionsDisjunctions("last_name : \"Smith\"");
1426+
checkWithFullTextFunctionsDisjunctions("qstr(\"last_name: Smith\")");
1427+
checkWithFullTextFunctionsDisjunctions("kql(\"last_name: Smith\")");
14271428
}
14281429

1429-
private void checkWithFullTextFunctionsDisjunctions(String functionName, String functionInvocation, String functionType) {
1430+
private void checkWithFullTextFunctionsDisjunctions(String functionInvocation) {
14301431

1432+
// Disjunctions with non-pushable functions - scoring
1433+
checkdisjunctionScoringError("1:35", functionInvocation + " or length(first_name) > 10");
1434+
checkdisjunctionScoringError("1:35", "match(last_name, \"Anneke\") or (" + functionInvocation + " and length(first_name) > 10)");
1435+
checkdisjunctionScoringError(
1436+
"1:35",
1437+
"(" + functionInvocation + " and length(first_name) > 0) or (match(last_name, \"Anneke\") and length(first_name) > 10)"
1438+
);
1439+
1440+
// Disjunctions with non-pushable functions - no scoring
14311441
query("from test | where " + functionInvocation + " or length(first_name) > 10");
14321442
query("from test | where match(last_name, \"Anneke\") or (" + functionInvocation + " and length(first_name) > 10)");
14331443
query(
14341444
"from test | where ("
14351445
+ functionInvocation
14361446
+ " and length(first_name) > 0) or (match(last_name, \"Anneke\") and length(first_name) > 10)"
14371447
);
1448+
1449+
// Disjunctions with full text functions - no scoring
14381450
query("from test | where " + functionInvocation + " or match(first_name, \"Anna\")");
14391451
query("from test | where " + functionInvocation + " or not match(first_name, \"Anna\")");
14401452
query("from test | where (" + functionInvocation + " or match(first_name, \"Anna\")) and length(first_name) > 10");
14411453
query("from test | where (" + functionInvocation + " or match(first_name, \"Anna\")) and match(last_name, \"Smith\")");
14421454
query("from test | where " + functionInvocation + " or (match(first_name, \"Anna\") and match(last_name, \"Smith\"))");
1455+
1456+
// Disjunctions with full text functions - scoring
1457+
query("from test metadata _score | where " + functionInvocation + " or match(first_name, \"Anna\")");
1458+
query("from test metadata _score | where " + functionInvocation + " or not match(first_name, \"Anna\")");
1459+
query("from test metadata _score | where (" + functionInvocation + " or match(first_name, \"Anna\")) and length(first_name) > 10");
1460+
query(
1461+
"from test metadata _score | where (" + functionInvocation + " or match(first_name, \"Anna\")) and match(last_name, \"Smith\")"
1462+
);
1463+
query(
1464+
"from test metadata _score | where " + functionInvocation + " or (match(first_name, \"Anna\") and match(last_name, \"Smith\"))"
1465+
);
1466+
1467+
}
1468+
1469+
private void checkdisjunctionScoringError(String position, String expression) {
1470+
assertEquals(
1471+
LoggerMessageFormat.format(
1472+
null,
1473+
"{}: Invalid condition when using METADATA _score [{}]. Full text functions can be used in an OR condition, "
1474+
+ "but only if just full text functions are used in the OR condition",
1475+
position,
1476+
expression
1477+
),
1478+
error("from test metadata _score | where " + expression)
1479+
);
14431480
}
14441481

14451482
public void testQueryStringFunctionWithNonBooleanFunctions() {

0 commit comments

Comments
 (0)