Add ESQL match function #113374
Add ESQL match function #113374
Conversation
carlosdelest
added
auto-backport
| return resolution; | ||
| } | ||
|
|
||
| public static TypeResolution isNotFoldable(Expression e, String operationName, ParamOrdinal paramOrd) { |
There was a problem hiding this comment.
Removed isNotFoldable() method as it was unused, and created isNotNull()
| // Source commands | ||
| assertEquals("1:13: [QSTR] function cannot be used after SHOW", error("show info | where qstr(\"8.16.0\")")); | ||
| assertEquals("1:13: [QSTR] function cannot be used after SHOW", error("show info | where qstr(\"Anna\")")); | ||
| assertEquals("1:17: [QSTR] function cannot be used after ROW", error("row a= \"Anna\" | where qstr(\"Anna\")")); |
There was a problem hiding this comment.
Tests for match and qstr have been refactored so they are shared where possible
There was a problem hiding this comment.
Thank you for addressing some of the comments from yesterday @carlosdelest . Some of them that you might have missed are highlighted below.
|
|
||
| private static void checkDisjunction(Set<Failure> failures, Or or, Expression left, Expression right) { | ||
| left.forEachDown(FullTextFunction.class, ftf -> { | ||
| if (canPushToSource(right, x -> false) == false) { |
There was a problem hiding this comment.
Calling canPushToSource from the coordinator is not quite right. I'd suggests to disable Match with disjunctions for now as a known limitation, and open a separate issue to find a solution in general.
There was a problem hiding this comment.
OK, let's disable this entirely and use a follow up issue. Done in 0dd782c
| } | ||
|
|
||
| @Override | ||
| public String functionName() { |
There was a problem hiding this comment.
Could you please change the name of
MatchFunctionandQueryStringFunctiontoMatchandQueryStringrespectively?
Don't forget the change the name of these two classes.
| public void testQueryStringWithDisjunctionsThatCannotBePushedDown() { | ||
| assumeTrue("skipping because QSTR is not enabled", EsqlCapabilities.Cap.QSTR_FUNCTION.isEnabled()); | ||
|
|
||
| checkWithDisjunctionsThatCannotBePushedDown("QSTR", "qstr(\"first_name: Anna\")"); | ||
| } | ||
|
|
||
| public void testMatchWithDisjunctionsThatCannotBePushedDown() { | ||
| assumeTrue("skipping because MATCH function is not enabled", EsqlCapabilities.Cap.MATCH_FUNCTION.isEnabled()); | ||
|
|
||
| checkWithDisjunctionsThatCannotBePushedDown("MATCH", "match(first_name, \"Anna\")"); | ||
| } | ||
|
|
||
| private void checkWithDisjunctionsThatCannotBePushedDown(String functionName, String functionInvocation) { | ||
| VerificationException ve = expectThrows( | ||
| VerificationException.class, | ||
| () -> plannerOptimizer.plan("from test | where " + functionInvocation + " or length(first_name) > 12", IS_SV_STATS) | ||
| ); | ||
| assertThat( | ||
| ve.getMessage(), | ||
| containsString( | ||
| LoggerMessageFormat.format( | ||
| null, | ||
| "1:19: Invalid condition [{} or length(first_name) > 12]. " | ||
| + "Function {} can't be used as part of an or condition that includes [length(first_name) > 12]", | ||
| functionInvocation, | ||
| functionName | ||
| ) | ||
| ) | ||
| ); | ||
| ve = expectThrows( | ||
| VerificationException.class, | ||
| () -> plannerOptimizer.plan( | ||
| "from test | where (" + functionInvocation + " and emp_no < 1000) or (length(first_name) > 12 and emp_no > 1000)", | ||
| IS_SV_STATS | ||
| ) | ||
| ); | ||
| assertThat( | ||
| ve.getMessage(), | ||
| containsString( | ||
| LoggerMessageFormat.format( | ||
| null, | ||
| "1:19: Invalid condition [({} and emp_no < 1000) or (length(first_name) > 12 and emp_no > 1000)]. " | ||
| + "Function {} can't be used as part of an or condition that includes [length(first_name) > 12 and emp_no > 1000]", | ||
| functionInvocation, | ||
| functionName | ||
| ) | ||
| ) | ||
| ); | ||
| } | ||
|
|
||
| public void testMatchFunctionDisjunctionNonPushableClauses() { | ||
| assumeTrue("skipping because MATCH function is not enabled", EsqlCapabilities.Cap.MATCH_FUNCTION.isEnabled()); | ||
| String queryText = """ | ||
| from test | ||
| | where match(first_name, "Peter") or length(last_name) > 10 | ||
| """; | ||
|
|
||
| VerificationException ve = expectThrows(VerificationException.class, () -> plannerOptimizer.plan(queryText, IS_SV_STATS)); | ||
| assertThat( | ||
| ve.getMessage(), | ||
| containsString( | ||
| "Invalid condition [match(first_name, \"Peter\") or length(last_name) > 10]. " | ||
| + "Function MATCH can't be used as part of an or condition that includes [length(last_name) > 10]" | ||
| ) | ||
| ); | ||
| } | ||
|
|
||
| public void testMatchFunctionIsNotNullable() { | ||
| assumeTrue("skipping because MATCH function is not enabled", EsqlCapabilities.Cap.MATCH_FUNCTION.isEnabled()); | ||
|
|
||
| String queryText = """ | ||
| row n = null | eval text = n + 5 | where match(text::keyword, "Anna") | ||
| """; | ||
|
|
||
| VerificationException ve = expectThrows(VerificationException.class, () -> plannerOptimizer.plan(queryText, IS_SV_STATS)); | ||
| assertThat( | ||
| ve.getMessage(), | ||
| containsString("[MATCH] cannot operate on [text::keyword], which is not a field from an index mapping") | ||
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
Move these tests catching VerificationException to LogicalPlanOptimizerTests.
| * @param action the action to execute for each parent of the specified typeToken | ||
| */ | ||
| @SuppressWarnings("unchecked") | ||
| public <E extends T> E forEachParent(Class<E> typeToken, BiConsumer<E, ? super T> action) { |
There was a problem hiding this comment.
Could this be moved the
Verifier? Just want to be cautious,Nodeis an important class, we change it only if it is necessary.
Don't forget to move this into Verifier, and keep Node unchanged.
|
Sorry @fang-xing-esql , but I didn't see any additional comments nor can I find them now. Thanks for pasting them again.
Done in d4e6256
|
|
@fang-xing-esql , I think I have addressed all comments as of now. If there are no more comments to address, let me know and I'll add the |
|
@elasticmachine update branch |
|
@fang-xing-esql , I added the |
# Conflicts: # x-pack/plugin/esql/src/main/antlr/EsqlBaseParser.g4 # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/EsqlBaseLexer.interp # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/EsqlBaseLexer.java # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/EsqlBaseParser.interp # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/EsqlBaseParser.java # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/EsqlParser.java # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/ExpressionBuilder.java
There was a problem hiding this comment.
LGTM! Thanks a lot for all the efforts and being patient with me @carlosdelest !
I wonder if there is an issue created for keeping track of full text functions with disjunctions(OR) yet? Can we have it linked to this PR, so that we don't forget about it? Thanks again!
💔 Backport failedThe backport operation could not be completed due to the following error: You can use sqren/backport to manually backport by running |
💚 All backports created successfully
Questions ?Please refer to the Backport tool documentation |
(cherry picked from commit a262eb6)
6 participants
Adds a
match()function, behind snapshot builds.This is similar to the work done on #112590. It adds a new
FullTextFunctionfor match queries.This PR modifies the ESQL grammar to allow
matchto be a valid function identifier - as theMATCHoperator introduced theMATCHtoken.Some reworking needed to be done in
FullTextFunctionfor adding multiple ordinal parameters support.It has the same limitations as QSTR -
FullTextFunctionsneed to be pushed down to Lucene as part of the local physical plan optimizing. We're limiting the commands that can be used before the function, so we don't allow commands that may alter the existing columns (DROP, KEEP, EVAL).