ESQL QSTR function by carlosdelest · Pull Request #112590 · elastic/elasticsearch

carlosdelest · 2024-09-06T11:12:05Z

Adds a QSTR function, behind snapshot builds.

This is very much WIP for sharing ideas and early feedback.

Current approach:

Use a FullTextFunction base class for full text query functions
QueryStringFunction returns a boolean - the type was done just to fit as part of WHERE boolean expressions, conceptually it returns a query to be executed at Lucene. This may need its own data type 🤔
FullTextFunctions are able to be pushed down to Lucene as part of the local physical plan optimizing, similar to MATCH operator
Similar to how the MATCH command worked, 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).

…lest/esql-qstr-function # Conflicts: # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LocalPhysicalPlanOptimizer.java

# Conflicts: # x-pack/plugin/esql/qa/testFixtures/src/main/resources/meta.csv-spec # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java

carlosdelest · 2024-09-10T10:16:39Z

@elasticsearchmachine run buildkite/docs-build-pr

# Conflicts: # x-pack/plugin/esql/qa/testFixtures/src/main/resources/meta.csv-spec # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java

elasticsearchmachine · 2024-09-10T17:42:06Z

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

fang-xing-esql

Thanks for putting these together @carlosdelest, my comments are added.

fang-xing-esql · 2024-09-11T04:53:44Z

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

+        var expected = QueryBuilders.boolQuery().must(queryString).must(range);
+        assertThat(query.query().toString(), is(expected.toString()));
+    }
+


It will be great if there are tests with more combinations of functions and operators, to capture the current behaviors. For example:

qstr("last_name: Smith") OR emp_no > 10010 ===> disjunctive
qstr("last_name: Smith") and CIDR_MATCH(ip, "127.0.0.1/32") === both functions can be pushed down into Lucene
qstr(...) and/or qstr(...) ===> it looks like multiple qstrs are allowed but not combined for now, perhaps it can be a candidate for further optimization.
qsrt("last_name: Smith") and/or length(first_name) <= 8 ===> the other side of and/or is not pushed down to Lucene

Thanks @fang-xing-esql ! I've added these tests in c5b45ec.

Are there any other combinations that you can think of, or any rule of thumb that I could use to understand what cases should I be implementing? I haven't found more examples in LocalPhysicalPlanOptimizerTests that could guide me here.

it looks like multiple qstrs are allowed but not combined for now, perhaps it can be a candidate for further optimization.

Probably so - I wonder if queries pushed down to Lucene should be left to Lucene to optimize? 🤔

Thanks for adding more tests!

Are there any other combinations that you can think of, or any rule of thumb that I could use to understand what cases should I be implementing? I haven't found more examples in LocalPhysicalPlanOptimizerTests that could guide me here.

It could be challenging to write effective testcases. We always try to improve test coverage, but it is not guaranteed that everything is covered, there could be many edge cases, I found PR reviews help me a lot to improve test coverage. There are many existing test cases, and it is normal not be able to remember all of them :). These are the things that come to my mind first when writing tests for new functions:

Verify resolutions - function and type resolution. This is done(majority of them) by Analyzer, and these tests are usually covered by AnalyzerTests and VerifierTests.

Verify optimizer builds an optimal/desired plan. These are usually covered by LogicalPlanOptimizerTests, PhysicalPlanOptimizerTests, QueryTranslatorTests, and their local version if exist. For the functions and operators that can appear under where clause, I would consider test compound predicates - conjunctive and disjunctive. For this function in particular, because it got pushed down into Lucene, we could get some combinations of bool, should, must queries, which makes me think that we may want to cover conjunctive and disjunctive, and perhaps some nested cases. Most of the ES|QL functions have their own compute engine implementation, and this doesn't, which makes me think that we may want to cover some combinations of them. I think these are not edge cases, they might be quite common use cases. I'll try to think about more scenarios, and update it here, just don't want to throw all of them at one time :-).

Verify the query returns correct results. These are mainly done in CsvTests, REST and yml tests. I would add more complicated(conjunctive and disjunctive) tests to verify we get correct results.

it looks like multiple qstrs are allowed but not combined for now, perhaps it can be a candidate for further optimization.

Probably so - I wonder if queries pushed down to Lucene should be left to Lucene to optimize? 🤔

Yeah, Lucene might be able to do optimizations on multiple queries pushed down to it. Honestly I'm not an expert of it, still learning. There is something we can do in optimizer, an example of it is CombineBinaryComparisons, if Lucene doesn't do it or doesn't do it well, this rule can be a reference.

fang-xing-esql · 2024-09-11T05:03:55Z

...main/java/org/elasticsearch/xpack/esql/expression/function/fulltext/QueryStringFunction.java

+/**
+ * Full text function that performs a {@link QueryStringQuery} .
+ */
+public class QueryStringFunction extends FullTextFunction {


Usually an ES|QL function has its own AbstractFunctionTestCase, an important usage is to generate documents(some of them are used by Kibana) automatically. This function doesn't have its compute engine implementation, I'm not quite sure how to make an AbstractFunctionTestCase for it yet, need to figure out how to generate the docs automatically, or manually create them.

QueryStringFunction takes only string literal, however if a numeric type is provided, it passes Analyzer, if a field is provided as input, it fails at ExpressionTranslator, these could be candidates for VerifierTests.

Override resolveType() is the usual way to validate input data types and foldable for functions during Analyzer phase, perhaps refer to an ES|QL string function as an example.

Good point on AbstractFunctionTestCase, but we don't want to generate documentation yet since the function is only available in snapshots. Also I might be looking at the wrong place, but we have another function Rate that is only available in snapshots and it also seems to be missing its own AbstractFunctionTestCase.
So I wonder if we can defer the need for a AbstractFunctionTestCase for now, since it seems it requires a bit of rework so that AbstractFunctionTestCase can also work with functions that do not have a compute engine implementation.

QueryStringFunction takes only string literal, however if a numeric type is provided, it passes Analyzer, if a field is provided as input, it fails at ExpressionTranslator, these could be candidates for VerifierTests.

Override resolveType() is the usual way to validate input data types and foldable for functions during Analyzer phase, perhaps refer to an ES|QL string function as an example.

I've tried to address this in 7b2cbaa

++ Ioana on documentation and AbstractFunctionTestCase - we can probably defer this until we plan to move it out of snapshot builds

I added a ui label, so that Kibana is aware of this. As far as I know AbstractFunctionTestCase generates docs for Kibana, we can figure out the best option to move forward.

++ Ioana on documentation and AbstractFunctionTestCase - we can probably defer this until we plan to move it out of snapshot builds

Can an issue be created to keep track of AbstractFunctionTestCase if it won't be included in this PR? It is important when this function is taken out of snapshot, we may not want to loose track of it. Thank you!

I've actually given it a go here in 4b97e6d - it performs type checking and folding.

fang-xing-esql · 2024-09-17T17:33:48Z

x-pack/plugin/esql/qa/testFixtures/src/main/resources/qstr-function.csv-spec

+| limit 5;
+// end::qstr-with-field[]
+
+book_no:keyword | author:text


A result tag(qstr-with-field-result) is missing, the tag is referenced in docs/reference/esql/functions/examples/qstr.asciidoc

I see, I added that in 70dd5d2

fang-xing-esql · 2024-09-17T17:35:48Z

...java/org/elasticsearch/xpack/esql/expression/function/fulltext/QueryStringFunctionTests.java

+
+import static org.hamcrest.Matchers.equalTo;
+
+@FunctionName("qstr")


Thanks for including the generated docs.

Don't forget to commit the changes to AbstractFunctionTestCase, otherwise you may get these messages the next time ./gradlew -p x-pack/plugin/esql/ test runs.

Changes not staged for commit: (use "git add/rm <file>..." to update what will be committed) (use "git restore <file>..." to discard changes in working directory) deleted: docs/reference/esql/functions/description/qstr.asciidoc deleted: docs/reference/esql/functions/examples/qstr.asciidoc deleted: docs/reference/esql/functions/kibana/definition/qstr.json deleted: docs/reference/esql/functions/kibana/docs/qstr.md deleted: docs/reference/esql/functions/layout/qstr.asciidoc deleted: docs/reference/esql/functions/parameters/qstr.asciidoc deleted: docs/reference/esql/functions/signature/qstr.svg deleted: docs/reference/esql/functions/types/qstr.asciidoc

I did that but reverted the change, as this implies changes from the Categorize function. I've opened this PR for dealing with that specific change. Can you please review?

I guess we are pending on the PR to change AbstractFunctionTestCase to generate docs for snapshot functions, without it if just checking in QSTR related docs, it will be a surprise the next time people run ./gradlew -p x-pack/plugin/esql/ test before pushing PRs, these docs will be marked as deleted.

I just merged #113080 👍

fang-xing-esql · 2024-09-17T17:36:28Z

Let's figure out the technical difficulty to have the docs in place in this PR, it is preferable to have it here so that the downstream components can use them more smoothly, but if it is too much effort we can separate them, just don't want to lose track of it.

I think we have a strategy as the docs can be generated for Kibana but not included ES docs. LMKWYT!

Just added some more comments related to docs, we are almost there.

fang-xing-esql · 2024-09-17T17:42:36Z

docs/reference/esql/functions/kibana/definition/qstr.json

+  "examples" : [
+    "from books \n| where qstr(\"author: Faulkner\")\n| keep book_no, author \n| sort book_no \n| limit 5;"
+  ]
+}


Somehow the "preview" : true is not here, I'll update here if I find out why.

This was due to the docs being generated without the merged version from main - fixed in 28b72f9

fang-xing-esql · 2024-09-17T17:45:01Z

docs/reference/esql/functions/examples/qstr.asciidoc

+----
+[%header.monospaced.styled,format=dsv,separator=|]
+|===
+include::{esql-specs}/qstr-function.csv-spec[tag=qstr-with-field-result]


This qstr-with-field-result tag is not in qstr-function.csv-spec.

fang-xing-esql · 2024-09-17T17:45:41Z

docs/reference/esql/functions/layout/qstr.asciidoc

+[[esql-qstr]]
+=== `QSTR`
+
+preview::["Do not use `VALUES` on production environments. This functionality is in technical preview and may be changed or removed in a future release. Elastic will work to fix any issues, but features in technical preview are not subject to the support SLA of official GA features."]


It is weird VALUES is mentioned here.

This was due to the docs being generated without the merged version from main - fixed in 28b72f9

This reverts commit 558b7c7.

carlosdelest · 2024-09-19T05:49:56Z

@fang-xing-esql , I just merged #113080 so docs generation can be done for QSTR.

Can you please check if there are any other pending concerns, or anything we should track for a follow-up PR, so we can merge this one?

Thanks!

fang-xing-esql

LGTM! Thank you for enabling full text search in ES|QL @carlosdelest !

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

carlosdelest · 2024-09-19T14:34:59Z

Thanks for your help @fang-xing-esql !

elasticsearchmachine · 2024-09-19T14:36:02Z

💔 Backport failed

Status	Branch	Result
❌	8.x	Commit could not be cherrypicked due to conflicts

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 112590

carlosdelest · 2024-09-19T14:49:28Z

💚 All backports created successfully

Status	Branch	Result
✅	8.x

Questions ?

Please refer to the Backport tool documentation

(cherry picked from commit 8d1b22e) # Conflicts: # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/EsqlFunctionRegistry.java # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plugin/EsqlPlugin.java # x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/SerializationTestUtils.java

carlosdelest added 6 commits September 6, 2024 09:18

First version for qstr

05fc57a

First round of testing

a3e4852

Remove the need for evaluator

4b04f43

Reformat CSV test

16cc426

Merge remote-tracking branch 'refs/remotes/origin/main' into carlosde…

ada0567

…lest/esql-qstr-function # Conflicts: # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LocalPhysicalPlanOptimizer.java

Fix merge from main, spotless

56b0a11

elasticsearchmachine added the v8.16.0 label Sep 6, 2024

carlosdelest added 7 commits September 6, 2024 13:12

Fix MATCH test

d7acfce

Add tests

fb8ff9d

Allow queries to resolve expressions that are not string based

28725d6

Merge branch 'main' into carlosdelest/esql-qstr-function

0a7a8e9

# Conflicts: # x-pack/plugin/esql/qa/testFixtures/src/main/resources/meta.csv-spec # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java

Fix test

0e19bcd

Fix and add tests

d7595f1

Spotless

48d3604

carlosdelest added 6 commits September 10, 2024 13:05

Minor refactoring

d192b18

Oops, switch patterns are experimental

92c1588

Fix tests

eb3b637

Spotless

9eaed72

Merge branch 'main' into carlosdelest/esql-qstr-function

de2fd89

# Conflicts: # x-pack/plugin/esql/qa/testFixtures/src/main/resources/meta.csv-spec # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java

Fix test

f66f962

carlosdelest marked this pull request as ready for review September 10, 2024 17:41

carlosdelest requested a review from a team as a code owner September 10, 2024 17:41

carlosdelest added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Sep 10, 2024

elasticsearchmachine added the needs:triage Requires assignment of a team area label label Sep 10, 2024

carlosdelest added :Analytics/ES|QL AKA ESQL and removed needs:triage Requires assignment of a team area label labels Sep 10, 2024

carlosdelest added the >non-issue label Sep 10, 2024

fang-xing-esql reviewed Sep 11, 2024

View reviewed changes

fang-xing-esql reviewed Sep 17, 2024

View reviewed changes

carlosdelest added 4 commits September 18, 2024 08:15

Generated docs for snapshot functions

558b7c7

Regenerate docs with new main version

28b72f9

Add missing result tags to spec

70dd5d2

Revert "Generated docs for snapshot functions"

98568bf

This reverts commit 558b7c7.

carlosdelest mentioned this pull request Sep 18, 2024

ESQL - generate docs for snapshot functions #113080

Merged

carlosdelest requested a review from fang-xing-esql September 18, 2024 17:27

fang-xing-esql approved these changes Sep 19, 2024

View reviewed changes

Merge branch 'main' into carlosdelest/esql-qstr-function

77462ac

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

carlosdelest added v8.16.0 auto-backport Automatically create backport pull requests when merged labels Sep 19, 2024

carlosdelest merged commit 8d1b22e into elastic:main Sep 19, 2024

elasticsearchmachine added the backport pending label Sep 19, 2024

carlosdelest mentioned this pull request Sep 19, 2024

[8.x] ESQL QSTR function (#112590) #113189

Merged

carlosdelest added a commit that referenced this pull request Sep 23, 2024

[8.x] ESQL QSTR function (#112590) (#113189)

c3a2b19

ioanatia mentioned this pull request Sep 23, 2024

Remove match command #113383

Merged

carlosdelest mentioned this pull request Sep 23, 2024

Add ESQL match function #113374

Merged

astefan removed the backport pending label Oct 9, 2024

ioanatia mentioned this pull request Sep 10, 2024

Full-text search in ES|QL #110131

Closed

14 tasks

carlosdelest mentioned this pull request Nov 18, 2024

ES|QL QSTR roadmap #116930

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ESQL QSTR function#112590

ESQL QSTR function#112590
carlosdelest merged 47 commits intoelastic:mainfrom
carlosdelest:carlosdelest/esql-qstr-function

carlosdelest commented Sep 6, 2024 •

edited

Loading

carlosdelest commented Sep 10, 2024

elasticsearchmachine commented Sep 10, 2024

fang-xing-esql left a comment

fang-xing-esql Sep 11, 2024

carlosdelest Sep 11, 2024

fang-xing-esql Sep 11, 2024

fang-xing-esql Sep 11, 2024

fang-xing-esql Sep 11, 2024

ioanatia Sep 11, 2024 •

edited

Loading

carlosdelest Sep 11, 2024

carlosdelest Sep 11, 2024

fang-xing-esql Sep 11, 2024

fang-xing-esql Sep 11, 2024

carlosdelest Sep 12, 2024

fang-xing-esql Sep 17, 2024

carlosdelest Sep 18, 2024

fang-xing-esql Sep 17, 2024

carlosdelest Sep 18, 2024

fang-xing-esql Sep 18, 2024

carlosdelest Sep 19, 2024

fang-xing-esql commented Sep 17, 2024 •

edited

Loading

fang-xing-esql Sep 17, 2024

carlosdelest Sep 18, 2024

fang-xing-esql Sep 17, 2024 •

edited

Loading

fang-xing-esql Sep 17, 2024 •

edited

Loading

carlosdelest Sep 18, 2024

carlosdelest commented Sep 19, 2024

fang-xing-esql left a comment

carlosdelest commented Sep 19, 2024

elasticsearchmachine commented Sep 19, 2024

carlosdelest commented Sep 19, 2024

Labels

8 participants


		import static org.hamcrest.Matchers.equalTo;

		@FunctionName("qstr")

Conversation

carlosdelest commented Sep 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

carlosdelest commented Sep 10, 2024

elasticsearchmachine commented Sep 10, 2024

fang-xing-esql left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fang-xing-esql commented Sep 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fang-xing-esql Sep 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

fang-xing-esql Sep 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlosdelest commented Sep 19, 2024

fang-xing-esql left a comment

Choose a reason for hiding this comment

carlosdelest commented Sep 19, 2024

elasticsearchmachine commented Sep 19, 2024

💔 Backport failed

carlosdelest commented Sep 19, 2024

💚 All backports created successfully

Questions ?

Labels

8 participants

carlosdelest commented Sep 6, 2024 •

edited

Loading

ioanatia Sep 11, 2024 •

edited

Loading

fang-xing-esql commented Sep 17, 2024 •

edited

Loading

fang-xing-esql Sep 17, 2024 •

edited

Loading

fang-xing-esql Sep 17, 2024 •

edited

Loading