Make search functions translation aware #118355

ioanatia · 2024-12-10T15:40:22Z

Following up on #118106 - we break adding semantic search in ES|QL into multiple parts:

All FullTextFunction will own their translation to Lucene queries and FullTextFunction instances will store their query builder.
Introduce the query rewrite phase on the coordinator that would rewrite the initial QueryBuilders for FullTextFunctions and replace the FullTextFunction expression with new ones that store the rewritten query builders in the logical plan.
Finally - when MatchQueryBuilder supports semantic text (should happen very soon Add match support for semantic_text fields #117839) enable the match function to receive semantic_text fields to perform semantic search.

This is just taking care of (1). We introduce a TranslationAware interface and FullTextFunction instances will store their own QueryBuilder which will be serialized.

I wasn't sure which type of tests to add since most of it is a refactor on how we do the FullTextFunction -> query builder translation.

ioanatia · 2024-12-10T15:42:18Z

...plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/fulltext/Kql.java

+    @SuppressWarnings("rawtypes")
+    @Override
+    protected ExpressionTranslator translator() {
+        return new EsqlExpressionTranslators.KqlFunctionTranslator();


I guess we could move the translators from EsqlExpressionTranslators but for now I just leave them as they are - I wasn't sure we need to move them. happy to hear otherwise.

My preference is to move them, but not in this PR - as a follow up.

ioanatia · 2024-12-10T15:48:21Z

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

+    @SuppressWarnings("rawtypes")
+    protected abstract ExpressionTranslator translator();
+
+    public abstract Expression replaceQueryBuilder(QueryBuilder queryBuilder);


this is not used yet - but this is what we will call for the query rewrite phase - see #118106

ChrisHegarty

Overall, I think that this is a good refactoring and improvement that will facilitate better encapsulation and reuse of existing builders. LGTM

ChrisHegarty · 2024-12-11T11:07:30Z

...plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/fulltext/Kql.java

+    @SuppressWarnings("rawtypes")
+    @Override
+    protected ExpressionTranslator translator() {
+        return new EsqlExpressionTranslators.KqlFunctionTranslator();


My preference is to move them, but not in this PR - as a follow up.

...n/java/org/elasticsearch/xpack/esql/core/querydsl/query/TranslationAwareExpressionQuery.java

...n/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/expression/TranslationAware.java

…functions

elasticsearchmachine · 2024-12-12T09:24:26Z

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

luigidellaquila

Thanks @ioanatia, I had a first look at the PR an it seems good.
I added a comment about a couple more things we can add to better encapsulate the pushdown logic.

cc. @costin maybe you want to have a look as well, I know you have ideas around these new interfaces.

luigidellaquila · 2024-12-12T14:05:38Z

...n/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/expression/TranslationAware.java

+ * {@link Query} translation, instead of relying on the registered translators from EsqlExpressionTranslators.
+ */
+public interface TranslationAware {
+    Query asQuery(TranslatorHandler translatorHandler);


I think it would make sense to centralize in this interface all the pushdown logic, including

capabilities, eg. can it be pushed down to Lucene at all? Maybe sometimes it depends on its inputs?

constraints, eg. does this function need to be pushed down to be executed? Does it also have an inline implementation that works without pushdown?

The first point is covered by PushFiltersToSource physical rule. I'm not sure if we want ad-hoc logic for each function or if we can simplify it to a single and generic behavior (ie. removing the if (exp instanceof Term). In both cases I'd rather have an exp instanceof TranslationAware there, so that one day we can also let other operators/functions implement it and remove that long list of instanceofs.

For the second point I'm thinking about the Verifier logic around FullTextFunctions. It's a complex topic that involves some deep understanding of how commands interact with each other, I put it here for completeness but probably we can address it in a follow-up PR

thank you for the review!

For the first point the PushFiltersToSource we need to look into it but I think we don't need the condition for Term - we removed a similar one for Match - see https://github.com/elastic/elasticsearch/pull/117555/files#r1858385620

For the second point, agreed we can do it in a follow up - I think it would be a big enough change that we would want to review separately.

fang-xing-esql

Thank you @ioanatia. It is a good step forward to add TranslationAware. I added a few things that I can think of so far.

fang-xing-esql · 2024-12-12T14:37:34Z

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

+        return Objects.equals(queryBuilder, ((FullTextFunction) obj).queryBuilder);
+    }
+
+    @SuppressWarnings("rawtypes")


Add a @Override annotation here.

fang-xing-esql · 2024-12-12T14:46:16Z

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

+            return new TranslationAwareExpressionQuery(source(), queryBuilder);
+        }
+
+        ExpressionTranslator translator = translator();


If ExpressionTranslator<? extends FullTextFunction> is used here, the @SuppressWarnings("rawtypes") can go away. The same applies to the translator() of the five FullTextFunctions.

fang-xing-esql · 2024-12-12T14:51:05Z

...plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/fulltext/Kql.java

-    public static final NamedWriteableRegistry.Entry ENTRY = new NamedWriteableRegistry.Entry(Expression.class, "Kql", Kql::new);
+    public static final NamedWriteableRegistry.Entry ENTRY = new NamedWriteableRegistry.Entry(Expression.class, "Kql", Kql::readFrom);
+
+    public Kql(Source source, Expression queryString, QueryBuilder queryBuilder) {


Can this be private? And it looks better(to me) if this is after the constructor with FunctionInfo. The same applies to the other three - Match, QueryString and Term.

I made them private and then the EsqlNodeSubclassTests started failing because AFAICT they expect that the argument number from the longest public constructor to correlate with the number of args from NodeInfo.
This to me seems to be a quirk of the EsqlNodeSubclassTests rather than a test catching a real bug with FullTextFunctions

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

…functions

* Introduce TranslationAware interface * Serialize query builder * Fix EsqlNodeSubclassTests * Add javadoc * Address review comments * Revert changes on making constructors private

* Introduce TranslationAware interface * Serialize query builder * Fix EsqlNodeSubclassTests * Add javadoc * Address review comments * Revert changes on making constructors private Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

) * Introduce TranslationAware interface * Serialize query builder * Fix EsqlNodeSubclassTests * Add javadoc * Address review comments * Revert changes on making constructors private Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

ioanatia added 3 commits December 10, 2024 13:47

Introduce TranslationAware interface

7102c33

Serialize query builder

ce68659

Fix EsqlNodeSubclassTests

da29b3f

ioanatia added >non-issue :Analytics/ES|QL AKA ESQL labels Dec 10, 2024

elasticsearchmachine added the v9.0.0 label Dec 10, 2024

ioanatia commented Dec 10, 2024

View reviewed changes

ioanatia changed the title ~~Make search function translation aware~~ Make search functions translation aware Dec 10, 2024

ioanatia commented Dec 10, 2024

View reviewed changes

ioanatia requested review from fang-xing-esql and ChrisHegarty December 10, 2024 15:48

ChrisHegarty approved these changes Dec 11, 2024

View reviewed changes

ioanatia added 2 commits December 11, 2024 14:27

Add javadoc

5acb8d5

Merge remote-tracking branch 'elasticsearch/main' into query_builder_…

a195a87

…functions

ioanatia marked this pull request as ready for review December 12, 2024 09:24

elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Dec 12, 2024

luigidellaquila reviewed Dec 12, 2024

View reviewed changes

fang-xing-esql reviewed Dec 12, 2024

View reviewed changes

Address review comments

667d2a6

ioanatia requested a review from fang-xing-esql December 12, 2024 16:32

ioanatia added 2 commits December 13, 2024 10:05

Merge remote-tracking branch 'elasticsearch/main' into query_builder_…

e4ec2f3

…functions

Revert changes on making constructors private

f12b980

ioanatia merged commit a765f89 into elastic:main Dec 13, 2024
16 checks passed

ioanatia mentioned this pull request Dec 13, 2024

Backport 8.x - Make search functions translation aware (#118355) #118677

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make search functions translation aware #118355

Make search functions translation aware #118355

ioanatia commented Dec 10, 2024 •

edited

Loading

ioanatia Dec 10, 2024

ChrisHegarty Dec 11, 2024

ioanatia Dec 10, 2024

ChrisHegarty left a comment

ChrisHegarty Dec 11, 2024

elasticsearchmachine commented Dec 12, 2024

luigidellaquila left a comment

luigidellaquila Dec 12, 2024

ioanatia Dec 12, 2024 •

edited

Loading

fang-xing-esql left a comment

fang-xing-esql Dec 12, 2024

fang-xing-esql Dec 12, 2024

fang-xing-esql Dec 12, 2024

ioanatia Dec 13, 2024

Make search functions translation aware #118355

Make search functions translation aware #118355

Conversation

ioanatia commented Dec 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChrisHegarty left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticsearchmachine commented Dec 12, 2024

luigidellaquila left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ioanatia Dec 12, 2024 • edited Loading

Choose a reason for hiding this comment

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

ioanatia commented Dec 10, 2024 •

edited

Loading

ioanatia Dec 12, 2024 •

edited

Loading