ESQL - Allow full text functions disjunctions for non-full text functions #120291

carlosdelest · 2025-01-16T15:05:02Z

Enables disjunctions in full text functions with non-pushable functions when not using score.

Full text functions create a LuceneQueryExpressionEvaluator that provides the evaluation of a Lucene query over the retrieved documents in the compute engine. This way, full text functions can be evaluated when they are not pushed down.

In order to do that, it is necessary to provide ShardContexts to the EvaluatorMapper interface so the function QueryBuilder can be transformed into a Query, and the LuceneQueryExpressionEvaluator can retrieve the ShardContexts to execute the Query on the corresponding shards.

Pushing down filters now take into account whether a condition contains a disjunction with full text functions. The previous validation has been removed.

This doesn't take into account scoring, as we need to re-compute scores on the compute engine side. For now, we're disabling the use of disjunctions when METADATA _score is active.

Inspired by #113938

Pending work:

Add tests

…terfaces

...l/compute/src/main/java/org/elasticsearch/compute/lucene/LuceneQueryExpressionEvaluator.java

carlosdelest · 2025-01-16T16:56:03Z

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

+        FoldContext foldCtx,
+        Expression exp,
+        Layout layout,
+        List<ShardContext> shardContexts


ShardContexts are provided to the EvalMapper in order to include them on the ToEvaluator interface.

carlosdelest · 2025-01-16T16:56:58Z

...plugin/esql/src/main/java/org/elasticsearch/xpack/esql/evaluator/mapper/EvaluatorMapper.java


        FoldContext foldCtx();
+
+        default List<EsPhysicalOperationProviders.ShardContext> shardContexts() {


Now creating an evaluator can use the shardContexts. This way the signature does not change for the EvaluatorMapper interface.

carlosdelest · 2025-01-16T16:58:32Z

...rc/main/java/org/elasticsearch/xpack/esql/expression/function/fulltext/FullTextFunction.java

+        for (EsPhysicalOperationProviders.ShardContext shardContext : shardContexts) {
+            shardConfigs[i++] = new ShardConfig(shardContext.toQuery(queryBuilder()), shardContext.searcher());
+        }
+        return new LuceneQueryExpressionEvaluator.Factory(shardConfigs);


Create the LuceneQueryExpressionEvaluator with the ShardContexts now that ToEvaluator include them

...asticsearch/xpack/esql/expression/predicate/operator/comparison/InsensitiveEqualsMapper.java

carlosdelest · 2025-01-16T16:59:20Z

...in/java/org/elasticsearch/xpack/esql/optimizer/rules/physical/local/PushFiltersToSource.java

+     *
+     * @param condition        condition to check for disjunctions of full text searches
+     */
+    private static boolean checkPushableFullTextSearchFunctions(Expression condition) {


This is the disjunction detection logic that references FullTextFunction

should this also handle column operator (:) ?

Yes! The operator uses Match function under the hood - it's just syntactic sugar

carlosdelest · 2025-01-16T17:00:48Z

...ck/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/planner/LocalExecutionPlanner.java

        LookupFromIndexService lookupFromIndexService,
-        PhysicalOperationProviders physicalOperationProviders
+        PhysicalOperationProviders physicalOperationProviders,
+        List<ShardContext> shardContexts


We need the ShardContexts so we can use them in the call to EvalMapper.toEvaluator.

It will be great if we can have some comments in the code about using ShardContext in ComputeService, LocalExecutionPlanner and EvalMapper, it took me a while to understand it :-).

carlosdelest · 2025-01-16T17:03:57Z

@nik9000 , I'd like an early review from you on the overall approach. Thanks!

…unctions-disjunctions' into non-issue/esql-full-text-functions-disjunctions

elasticsearchmachine · 2025-01-17T10:18:33Z

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

# Conflicts: # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/fulltext/FullTextFunction.java # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/physical/local/PushFiltersToSource.java

carlosdelest · 2025-01-22T16:22:02Z

@jimczi I'm adding you to this one to check if this solution for implementing disjunctions in full text functions matches your expectations. Scoring is not being handled yet, will do in a follow up.

ioanatia

tested this - works great!
I have one comment that I'd like to be addressed before merging - otherwise 🚀 !

ioanatia · 2025-01-23T13:49:24Z

.../plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/predicate/logical/Or.java

+
+    @Override
+    public boolean translatable(LucenePushdownPredicates pushdownPredicates) {
+        return super.translatable(pushdownPredicates) && FullTextFunction.checkDisjunctionPushable(this);


Does this introduce a side effect where we might not pushdown a disjunction that could be pushed because it uses a FTF? I haven't tested this, but assuming we have:

FROM my-index | WHERE match(foo, "bar") or x == "foobar"

Normally assuming that x == "foobar" can be pushed, we should be able to push match(foo, "bar") or x == "foobar" but I wonder if this still happens.
We added this method because we need it for the check where disjunctions are used with scoring.
I'd actually be in favour of not overriding this method here and just have the check done only in FullTextFunction if possible. It's also awkward how this method is actually called in FullTextFunction and then here we end up calling FullTextFunction.checkDisjunctionPushable(this).

Does this introduce a side effect where we might not pushdown a disjunction that could be pushed because it uses a FTF?

You are correct! There is no longer a need to check disjunctions when scoring is not there. That's much better done in FullTextFunction class.

I corrected this in d23998d and added a plan optimizer test to check that a disjunction with pushable functions is effectively pushed down.

Thank you!

tteofili · 2025-01-27T08:56:45Z

docs/reference/esql/esql-limitations.asciidoc

+* All functions used in the OR clauses are full-text functions themselves, or
+* Scoring is not used


in the meantime scoring has been put out of snapshot, hence it might be useful to reflect it here too.

tteofili · 2025-01-27T09:11:56Z

...l/compute/src/main/java/org/elasticsearch/compute/lucene/LuceneQueryExpressionEvaluator.java

+        // Lucene based operators retrieve DocVectors as first block
+        DocVector docs = page.<DocBlock>getBlock(0).asVector();


I am not sure about this TBH, what about a guard / assertion that makes sure that the expected block is a DocBlock, to avoid CCE in case of implementation changes in Lucene operators.

Good call, I added an assertion in 8e7a835

…ext-functions-disjunctions # Conflicts: # x-pack/plugin/build.gradle # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java

…unctions-disjunctions' into non-issue/esql-full-text-functions-disjunctions

fang-xing-esql

Thank you @carlosdelest, LGTM! I added some thoughts about disjunctions with scores, it does not block this PR to merge, you may decide the best way to handle it.

Having LuceneQueryExpressionEvaluator is a great step forward to allow queries with full text functions and disjunction run into completion, I wonder if it is a good idea to allow disjunction with _score run to completion as well with warnings instead of VerificationException?

fang-xing-esql · 2025-01-27T20:22:49Z

...ck/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/planner/LocalExecutionPlanner.java

        LookupFromIndexService lookupFromIndexService,
-        PhysicalOperationProviders physicalOperationProviders
+        PhysicalOperationProviders physicalOperationProviders,
+        List<ShardContext> shardContexts


It will be great if we can have some comments in the code about using ShardContext in ComputeService, LocalExecutionPlanner and EvalMapper, it took me a while to understand it :-).

fang-xing-esql · 2025-01-27T20:38:18Z

...rc/main/java/org/elasticsearch/xpack/esql/expression/function/fulltext/FullTextFunction.java

+                .stream()
+                .anyMatch(attr -> attr instanceof MetadataAttribute ma && ma.name().equals(MetadataAttribute.SCORE));
+            if (usesScore) {
+                checkFullTextSearchDisjunctions(condition, failures);


We actually can get scores for some disjunctions if both sides of the OR can be pushed down, however it is decided at data nodes, the coordinator node doesn't have this information.

For example, if the checkFullTextSearchDisjunctions is commented out, this query below can get score.

+ curl -u elastic:password -v -X POST 'localhost:9200/_query?format=txt&pretty' -H 'Content-Type: application/json' '-d { "query": "from sample_data metadata _score | where match(message, \"Connected\") or cidr_match(client.ip, \"172.21.3.15\") | drop @timestamp, event.duration, test_date" } ' client.ip | message | _score ---------------+---------------------+------------------ 172.21.2.162 |Connected to 10.1.0.3|0.7329744100570679 172.21.2.113 |Connected to 10.1.0.2|0.7329744100570679 172.21.3.15 |Connection error |0.0 172.21.3.15 |Connection error |0.0 172.21.3.15 |Connection error |0.0 172.21.3.15 |Connected to 10.1.0.1|0.7329744100570679

I'm wondering if it is a good idea to not fail queries with disjunctions and metadata _score? One option that I can think of is that if data nodes can pushdown the OR tree to Lucene, _score is populated by the one returned by Lucene, otherwise if we end up with running full text function with LuceneQueryExpressionEvaluator, we can set _score to null with warnings that mention score is not available for this situation. This is not a new behavior in ES|QL, for the other ES|QL functions, when there are exceptions happen at data nodes, null + warnings will be returned.

I'm wondering if it is a good idea to not fail queries with disjunctions and metadata _score?

I wondered about this as well. I think it's better to fail:

Score would be useless in this situation for relevance scoring

Users have a workaround: Either disable scoring or use full text functions for the query

This is a temporary limitation that we're aiming to fix

I'd prefer to fail early rather than provide a broken score, even with warnings.

…ext-functions-disjunctions # Conflicts: # x-pack/plugin/esql/qa/testFixtures/src/main/resources/match-function.csv-spec # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java

carlosdelest · 2025-01-28T10:44:33Z

It will be great if we can have some comments in the code about using ShardContext in ComputeService, LocalExecutionPlanner and EvalMapper, it took me a while to understand it :-).

@fang-xing-esql I added some comments to EvalMapper.toEvaluator in 5df79d4 - the rest of the changes imply adding ShardContext to constructors, so I don't think there's anything to comment there.

…ext-functions-disjunctions

…unctions-disjunctions' into non-issue/esql-full-text-functions-disjunctions

elasticsearchmachine · 2025-01-28T13:09:29Z

💔 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 120291

carlosdelest · 2025-01-28T13:21:23Z

💚 All backports created successfully

Status	Branch	Result
✅	8.x

Questions ?

Please refer to the Backport tool documentation

…ions (elastic#120291) (cherry picked from commit a87bd7a) # Conflicts: # docs/reference/esql/esql-limitations.asciidoc

…ions (#120291) (#121026) (cherry picked from commit a87bd7a) # Conflicts: # docs/reference/esql/esql-limitations.asciidoc

carlosdelest added 6 commits January 16, 2025 09:10

First version - adding toQuery() to ShardContext

61a479b

Remove the need to have toQuery() on ShardContext

e2e996d

Include shard contexts in ToEvaluator, so no need to differentiate in…

e1ca189

…terfaces

Check disjunctions for pushing down to source

9aa9d78

Spotless

ed62f9f

Fix tests

318b06e

elasticsearchmachine added the v9.0.0 label Jan 16, 2025

carlosdelest and others added 2 commits January 16, 2025 17:34

Fix tests

b4221e4

[CI] Auto commit changes from spotless

6d5b813

carlosdelest commented Jan 16, 2025

View reviewed changes

carlosdelest requested a review from nik9000 January 16, 2025 17:03

carlosdelest added 3 commits January 16, 2025 18:04

Spotless

bb46fd8

Merge remote-tracking branch 'carlosdelest/non-issue/esql-full-text-f…

ad575b5

…unctions-disjunctions' into non-issue/esql-full-text-functions-disjunctions

Don't allow disjunctions to be used with scoring

324f243

carlosdelest requested review from ioanatia and tteofili January 17, 2025 09:20

Merge branch 'main' into non-issue/esql-full-text-functions-disjunctions

bce1934

carlosdelest added >feature auto-backport Automatically create backport pull requests when merged Team:Search Relevance Meta label for the Search Relevance team in Elasticsearch v8.18.0 Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) :Analytics/ES|QL AKA ESQL labels Jan 17, 2025

Update docs/changelog/120291.yaml

4456c24

carlosdelest added 4 commits January 17, 2025 18:15

Add CSV tests

92a3809

Update generated docs

023daea

Moves validation to Or class

c97aeb7

Merge branch 'main' into non-issue/esql-full-text-functions-disjunctions

6b1fc89

carlosdelest requested a review from jimczi January 22, 2025 16:21

ioanatia approved these changes Jan 23, 2025

View reviewed changes

carlosdelest and others added 2 commits January 23, 2025 16:02

OR doesn't need to check for pushable full text functions

d23998d

Merge branch 'main' into non-issue/esql-full-text-functions-disjunctions

6022dad

ioanatia approved these changes Jan 24, 2025

View reviewed changes

tteofili approved these changes Jan 27, 2025

View reviewed changes

carlosdelest added 4 commits January 27, 2025 11:02

Add assertion to check correct type

8e7a835

Minor doc fix

9fdd24f

Merge remote-tracking branch 'origin/main' into non-issue/esql-full-t…

58677e4

…ext-functions-disjunctions # Conflicts: # x-pack/plugin/build.gradle # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java

Merge remote-tracking branch 'carlosdelest/non-issue/esql-full-text-f…

1035871

…unctions-disjunctions' into non-issue/esql-full-text-functions-disjunctions

fang-xing-esql approved these changes Jan 27, 2025

View reviewed changes

Added some comments to EvalMapper.toEvaluator

5df79d4

ioanatia mentioned this pull request Jan 28, 2025

Compute engine evaluation for full text functions #113938

Closed

carlosdelest and others added 2 commits January 28, 2025 11:38

Merge branch 'main' into non-issue/esql-full-text-functions-disjunctions

6503146

carlosdelest enabled auto-merge (squash) January 28, 2025 10:41

carlosdelest added 2 commits January 28, 2025 12:58

Merge remote-tracking branch 'origin/main' into non-issue/esql-full-t…

64b174e

…ext-functions-disjunctions

Merge remote-tracking branch 'carlosdelest/non-issue/esql-full-text-f…

9235b47

…unctions-disjunctions' into non-issue/esql-full-text-functions-disjunctions

carlosdelest merged commit a87bd7a into elastic:main Jan 28, 2025
16 checks passed

elasticsearchmachine added the backport pending label Jan 28, 2025

carlosdelest mentioned this pull request Jan 28, 2025

[8.x] ESQL - Allow full text functions disjunctions for non-full text functions (#120291) #121026

Merged

elasticsearchmachine pushed a commit that referenced this pull request Jan 28, 2025

ESQL - Allow full text functions disjunctions for non-full text funct…

2db1d4f

…ions (#120291) (#121026) (cherry picked from commit a87bd7a) # Conflicts: # docs/reference/esql/esql-limitations.asciidoc

carlosdelest mentioned this pull request Jan 29, 2025

[PoC] ESQL - Add scoring for full text functions disjunctions #121153

Closed

carlosdelest mentioned this pull request Mar 21, 2025

ES|QL QSTR roadmap #116930

Closed


		FoldContext foldCtx();

		default List<EsPhysicalOperationProviders.ShardContext> shardContexts() {

		* All functions used in the OR clauses are full-text functions themselves, or
		* Scoring is not used

		// Lucene based operators retrieve DocVectors as first block
		DocVector docs = page.<DocBlock>getBlock(0).asVector();

ESQL - Allow full text functions disjunctions for non-full text functions #120291

ESQL - Allow full text functions disjunctions for non-full text functions #120291

Uh oh!

Conversation

carlosdelest commented Jan 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

carlosdelest Jan 16, 2025 • 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!

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!

carlosdelest commented Jan 16, 2025

Uh oh!

elasticsearchmachine commented Jan 17, 2025

Uh oh!

carlosdelest commented Jan 22, 2025

Uh oh!

ioanatia 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fang-xing-esql 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!

carlosdelest commented Jan 28, 2025

Uh oh!

Uh oh!

elasticsearchmachine commented Jan 28, 2025

💔 Backport failed

Uh oh!

carlosdelest commented Jan 28, 2025

💚 All backports created successfully

Questions ?

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

carlosdelest commented Jan 16, 2025 •

edited

Loading

carlosdelest Jan 16, 2025 •

edited

Loading