From 89015915ad0300f3b64930b5db62fcc5507dd3aa Mon Sep 17 00:00:00 2001 From: carlosdelest Date: Tue, 17 Dec 2024 09:07:57 +0100 Subject: [PATCH] Simplify disjunction algorithm --- .../xpack/esql/plugin/MatchFunctionIT.java | 2 +- .../xpack/esql/analysis/Verifier.java | 54 +++++-------------- .../xpack/esql/analysis/VerifierTests.java | 12 ++--- 3 files changed, 19 insertions(+), 49 deletions(-) diff --git a/x-pack/plugin/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/plugin/MatchFunctionIT.java b/x-pack/plugin/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/plugin/MatchFunctionIT.java index d02201457575f..4757b3c583e4f 100644 --- a/x-pack/plugin/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/plugin/MatchFunctionIT.java +++ b/x-pack/plugin/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/plugin/MatchFunctionIT.java @@ -240,7 +240,7 @@ public void testWhereMatchWithFunctions() { error.getMessage(), containsString( "Invalid condition [match(content, \"fox\") OR to_upper(content) == \"FOX\"]. " - + "[MATCH] function can be used in an OR condition, but only if just full text functions are used in the OR condition" + + "Full text functions can be used in an OR condition, but only if just full text functions are used in the OR condition" ) ); } diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/Verifier.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/Verifier.java index 21da1fa781f8b..6b98b7d69834f 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/Verifier.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/Verifier.java @@ -27,7 +27,6 @@ import org.elasticsearch.xpack.esql.core.expression.predicate.logical.Or; import org.elasticsearch.xpack.esql.core.expression.predicate.operator.comparison.BinaryComparison; import org.elasticsearch.xpack.esql.core.type.DataType; -import org.elasticsearch.xpack.esql.core.util.Holder; import org.elasticsearch.xpack.esql.expression.function.UnsupportedAttribute; import org.elasticsearch.xpack.esql.expression.function.aggregate.AggregateFunction; import org.elasticsearch.xpack.esql.expression.function.aggregate.FilteredExpression; @@ -786,50 +785,23 @@ private static void checkFullTextSearchDisjunctions( // Exit early if we already have a failures return; } - Expression left = or.left(); - boolean leftHasFullText = left.anyMatch(FullTextFunction.class::isInstance); - boolean leftHasOnlyFullText = onlyFullTextFunctionsInExpression(left); - Expression right = or.right(); - boolean rightHasFullText = right.anyMatch(FullTextFunction.class::isInstance); - boolean rightHasOnlyFullText = onlyFullTextFunctionsInExpression(right); - - if (leftHasFullText && leftHasOnlyFullText == false) { - disjunctionFailure(or, left, typeNameProvider, failures); - } else if (rightHasFullText && rightHasOnlyFullText == false) { - disjunctionFailure(or, right, typeNameProvider, failures); - } else if (leftHasFullText ^ rightHasFullText) { - disjunctionFailure(or, leftHasFullText ? left : right, typeNameProvider, failures); + boolean hasFullText = or.anyMatch(FullTextFunction.class::isInstance); + if (hasFullText) { + boolean hasOnlyFullText = onlyFullTextFunctionsInExpression(or); + if (hasOnlyFullText == false) { + failures.add( + fail( + or, + "Invalid condition [{}]. Full text functions can be used in an OR condition, " + + "but only if just full text functions are used in the OR condition", + or.sourceText() + ) + ); + } } }); } - /** - * Add a disjunction failure to the failures collection. - * @param parentExpression parent expression to include in the failure - * @param expressionWithError expression that provoked the failure - * @param typeNameProvider provider for the type name to add in the failure message - * @param failures failures collection to add to - */ - private static void disjunctionFailure( - Expression parentExpression, - Expression expressionWithError, - java.util.function.Function typeNameProvider, - Set failures - ) { - Holder elementName = new Holder<>(); - // Get first function name to add to the failure message - expressionWithError.forEachDown(FullTextFunction.class, ftf -> elementName.set(typeNameProvider.apply(ftf))); - failures.add( - fail( - parentExpression, - "Invalid condition [{}]. {} can be used in an OR condition, " - + "but only if just full text functions are used in the OR condition", - parentExpression.sourceText(), - elementName.get() - ) - ); - } - /** * Checks whether an expression contains just full text functions or negations (NOT) and combinations (AND, OR) of full text functions * diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/VerifierTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/VerifierTests.java index 0e4567e6a4804..73462eddff282 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/VerifierTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/VerifierTests.java @@ -1166,14 +1166,14 @@ public void testMatchInsideEval() throws Exception { public void testMatchFilter() throws Exception { assertEquals( "1:19: Invalid condition [first_name:\"Anna\" or starts_with(first_name, \"Anne\")]. " - + "[:] operator can be used in an OR condition, " + + "Full text functions can be used in an OR condition, " + "but only if just full text functions are used in the OR condition", error("from test | where first_name:\"Anna\" or starts_with(first_name, \"Anne\")") ); assertEquals( - "1:51: Invalid condition [first_name:\"Anna\" OR new_salary > 100]. [:] operator can be used in an OR " - + "condition, but only if just full text functions are used in the OR condition", + "1:51: Invalid condition [first_name:\"Anna\" OR new_salary > 100]. Full text functions can be" + + " used in an OR condition, but only if just full text functions are used in the OR condition", error("from test | eval new_salary = salary + 10 | where first_name:\"Anna\" OR new_salary > 100") ); } @@ -1423,12 +1423,10 @@ private void checkdisjunctionError(String position, String expression, String fu assertEquals( LoggerMessageFormat.format( null, - "{}: Invalid condition [{}]. [{}] {} can be used in an OR condition, " + "{}: Invalid condition [{}]. Full text functions can be used in an OR condition, " + "but only if just full text functions are used in the OR condition", position, - expression, - functionName, - functionType + expression ), error("from test | where " + expression) );