Search in ES|QL: Add MATCH operator by ioanatia · Pull Request #110971 · elastic/elasticsearch

ioanatia · 2024-07-17T12:03:32Z

Part of #110131
Adds a MATCH operator, e.g.:

FROM search-movies
| WHERE title MATCH "harry potter"
| KEEP title

A few notes:

I did not add csv tests because the push down of the match predicate to Lucene is done with LocalPhysicalPlanOptimizer and csv tests do no execute the rules from LocalPhysicalPlanOptimizer. To test the push down of MATCH we used integration tests in MatchOperatorPushDownIT Andrei pointed out we have 2 different types of CSV tests - tracking this as a follow up in Full-text search in ES|QL #110131
I tried to find as many places where failures can occur when using MATCH and catch them early at the Verifier level so that we can return a VerificationException rather than have this fail at the compute engine level.
If there is a better place to push down the match predicates to the lucene query let me know, for now this happens in LocalPhysicalPlanOptimizer
I don't claim I patched all the ways this could fail deeper at the data notes level or compute engine - when I did not find an obvious fix, I added an integration test in MatchOperatorPushDownIT and marked with a TODO so we don't forget about it. Full-text search in ES|QL #110131 has a separate item to ensure that we have graceful failure modes for both the MATCH operator and MATCH command

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/ExpressionBuilder.java

x-pack/plugin/esql/src/main/antlr/EsqlBaseLexer.g4

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/Verifier.java

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

...ql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LocalPhysicalPlanOptimizerTests.java

elasticsearchmachine · 2024-07-18T14:19:44Z

Pinging @elastic/es-analytical-engine (Team:Analytics)

elasticsearchmachine · 2024-07-18T14:19:44Z

Pinging @elastic/es-search-relevance (Team:Search Relevance)

...rc/internalClusterTest/java/org/elasticsearch/xpack/esql/plugin/MatchOperatorPushDownIT.java

nik9000 · 2024-07-18T14:35:31Z

...rc/internalClusterTest/java/org/elasticsearch/xpack/esql/plugin/MatchOperatorPushDownIT.java

+            """;
+
+        var error = expectThrows(VerificationException.class, () -> run(query));
+        assertThat(error.getMessage(), containsString("Unknown column [something]"));


This doesn't feel like and IT on the pushdown - more like on MATCH itself. I might rename the class.

If you want to test the pushdown I might try using the profile option on the request and looking for the LuceneSourceOperator's status. It should have a list of queries it run and that should tell you that you pushed down.

I think you could do all of this with an ESRestTestCase if you wanted which gives you the advantage of being able to run it against many versions later.

For some of these failures I think I'd prefer them in the yaml test cases. They run against multiple versions by default and they have to be pretty simple so it can be easier to read them. Just because you know they can't surprise you.

I would, also, be ok with the tests in VerifierTests (the VerificationException ones).

I added yaml tests as well - I liked these these tests because I can easily enable logging and debug in the IDE - I find it easier to do that here than in yaml or CSV specs.

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/Verifier.java

elasticsearchmachine · 2024-07-22T11:37:51Z

Hi @ioanatia, I've created a changelog YAML for you.

astefan

Even if it takes longer, I would like this feature to be merged with csv-spec tests as well. It's something we usually do with features, bug-fixes, enhancements in ESQL.

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/Verifier.java

astefan · 2024-07-22T12:10:35Z

...rc/internalClusterTest/java/org/elasticsearch/xpack/esql/plugin/MatchOperatorPushDownIT.java

+            """;
+
+        var error = expectThrows(VerificationException.class, () -> run(query));
+        assertThat(error.getMessage(), containsString("Unknown column [something]"));


I would, also, be ok with the tests in VerifierTests (the VerificationException ones).

nik9000 · 2024-07-22T16:04:44Z

Even if it takes longer, I would like this feature to be merged with csv-spec tests as well. It's something we usually do with features, bug-fixes, enhancements in ESQL.

I think we only have two options for csv-spec tests:

Don't run these csv-spec tests in CsvTests via one of those assumeFalsees for a capability. So they'd only run in the big integration tests. Like with ./gradlew -p x-pack/plugin/esql/qa/server/single-node check.
Convert the CsvTests thing to build a Lucene index. That seems like a lot. Maybe not a bad idea, but a lot.

I opened #111157 to see if we could run these at runtime, but that's going to take some time to review and integrate into this. Also, it still needs a lucene index. I think we probably should skip the csv-spec tests in CsvTests.

nik9000

If we want CSV tests, I think this is the shortest route to them: https://gist.github.com/nik9000/38946d2ba2639db64fe5ee28d0b805a7

It adds a capability that's not supported in older versions and so the tests can be skipped.

nik9000 · 2024-07-24T19:36:25Z

x-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test/esql/180_match_operator.yml

+        - "No limit defined, adding default limit of \\[.*\\]"
+      esql.query:
+        body:
+          query: 'FROM test | WHERE content MATCH "fox" | KEEP id | SORT id'


This is going to fail without a capability, I think.

Wait. You check the cluster feature already. I think we're better off checking capabilities. We're just adding too many things to do it all with features.

This has been updated to use a capability. 👍

nik9000 · 2024-07-24T19:42:28Z

If we want CSV tests, I think this is the shortest route to them: https://gist.github.com/nik9000/38946d2ba2639db64fe5ee28d0b805a7

Wait. Ignore this. you did it.

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plugin/EsqlFeatures.java

x-pack/plugin/esql/qa/testFixtures/src/main/resources/where-match.csv-spec

nik9000 · 2024-07-24T19:44:04Z

x-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test/esql/180_match_operator.yml

+        - "No limit defined, adding default limit of \\[.*\\]"
+      esql.query:
+        body:
+          query: 'FROM test | WHERE content MATCH "fox" | KEEP id | SORT id'


Wait. You check the cluster feature already. I think we're better off checking capabilities. We're just adding too many things to do it all with features.

nik9000 · 2024-07-24T19:45:33Z

x-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test/esql/180_match_operator.yml

+
+  - match: { status: 400 }
+  - match: { error.type: verification_exception }
+  - match: { error.reason: "Found 1 problem\nline 1:19: MATCH requires a text or keyword field, but [id] has type [integer]" }


Checking my understanding - do we want MATCH to skip non-string fields in the future? Like, if I do WHERE int MATCH "1" should that fail? Or match nothing? Or match the literal 1? It's all good to match whatever makes sense in this PR though.

Our aim is to add full text search capabilities and not necessarily have full parity with the match query - this is why I restricted this to text and keyword for now - if we want to add support for data, boolean and numeric fields we can definitely do that later. not in this PR though

Matching on keyword and text seems reasonable. It's not clear to me how much value there is matching on more field types, but I agree that that can be discussed separately.

ChrisHegarty

I'm adding my approval to this PR. It LGTM, testing is comprehensive and all outstanding comments have been addressed.

While not strictly necessary, it would be good to have another approval, from someone who has commented on the PR.

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java

ioanatia · 2024-07-26T05:54:14Z

leaving this open for a while longer unless there are any other comments - otherwise I will be merging this later today.

ivancea

🚀

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java

…action/EsqlCapabilities.java Co-authored-by: Iván Cea Fontenla <ivancea96@outlook.com>

ioanatia added :Search/Search Search-related issues that do not fall into other categories Team:Search Meta label for search team labels Jul 17, 2024

elasticsearchmachine added the v8.16.0 label Jul 17, 2024

ioanatia mentioned this pull request Jul 17, 2024

Full-text search in ES|QL #110131

Closed

14 tasks

ioanatia commented Jul 17, 2024

View reviewed changes

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/ExpressionBuilder.java Show resolved Hide resolved

javanna added :Search Relevance/Search Catch all for Search Relevance and removed :Search/Search Search-related issues that do not fall into other categories labels Jul 17, 2024

astefan reviewed Jul 18, 2024

View reviewed changes

astefan added the :Analytics/ES|QL AKA ESQL label Jul 18, 2024

ioanatia marked this pull request as ready for review July 18, 2024 14:19

ioanatia requested a review from astefan July 18, 2024 14:19

elasticsearchmachine added Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) Team:Search Relevance Meta label for the Search Relevance team in Elasticsearch labels Jul 18, 2024

elasticsearchmachine removed the Team:Search Meta label for search team label Jul 18, 2024

nik9000 reviewed Jul 18, 2024

View reviewed changes

astefan added the >feature label Jul 22, 2024

astefan reviewed Jul 22, 2024

View reviewed changes

ioanatia requested review from a team as code owners July 22, 2024 14:31

ioanatia and others added 8 commits July 22, 2024 16:33

Grammar changes

1ff8116

Push match

1f9a0c7

Add query push down integration tests

5aea417

Add verifier tests

5b02f1c

Address review comments

23ed5b4

Update docs/changelog/110971.yaml

290e511

Change integration tests after review comments

d26baba

Fix typo and add comment for Verifier checks

0ff45c7

Add CSV tests

985e31d

ioanatia force-pushed the match_operator branch from 8b205b6 to 985e31d Compare July 22, 2024 14:34

ioanatia removed request for a team July 22, 2024 14:36

Fix generated changelog

db9c705

Add yml tests and fix verifier test

7394f2e

ioanatia requested review from ChrisHegarty, astefan and nik9000 July 24, 2024 09:39

ioanatia mentioned this pull request Jul 24, 2024

ES|QL MATCH command #111147

Merged

nik9000 reviewed Jul 24, 2024

View reviewed changes

nik9000 requested changes Jul 24, 2024

View reviewed changes

ioanatia added 2 commits July 25, 2024 10:48

Use capabilities instead of node features

2c2fcf1

Merge remote-tracking branch 'elasticsearch/main' into match_operator

eb5b72f

ChrisHegarty approved these changes Jul 25, 2024

View reviewed changes

ChrisHegarty requested a review from nik9000 July 25, 2024 09:58

nik9000 approved these changes Jul 25, 2024

View reviewed changes

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java Outdated Show resolved Hide resolved

Enable capability on snapshot builds only

817eea0

ivancea approved these changes Jul 26, 2024

View reviewed changes

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java Outdated Show resolved Hide resolved

Update x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/…

a429194

…action/EsqlCapabilities.java Co-authored-by: Iván Cea Fontenla <ivancea96@outlook.com>

ioanatia merged commit 8df1c50 into elastic:main Jul 26, 2024

ChrisHegarty added the ES|QL-ui Impacts ES|QL UI label Jul 26, 2024

carlosdelest mentioned this pull request Sep 11, 2024

ESQL QSTR function #112590

Merged

Conversation

ioanatia commented Jul 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

elasticsearchmachine commented Jul 18, 2024

Uh oh!

elasticsearchmachine commented Jul 18, 2024

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

elasticsearchmachine commented Jul 22, 2024

Uh oh!

astefan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nik9000 commented Jul 22, 2024

Uh oh!

nik9000 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nik9000 commented Jul 24, 2024

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChrisHegarty left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ioanatia commented Jul 26, 2024

Uh oh!

ivancea left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

ioanatia commented Jul 17, 2024 •

edited

Loading

ChrisHegarty left a comment •

edited

Loading