I needed to change build.gradle for both esql-core and esql because we need SemanticQueryBuilder.
I had to remove x-pack-ml because of jar hell.
The problem is that the categorize integration tests no longer work:

REPRODUCE WITH: ./gradlew ":x-pack:plugin:esql:qa:server:single-node:javaRestTest" --tests "org.elasticsearch.xpack.esql.qa.single_node.EsqlSpecIT" -Dtests.method="test {categorize.Categorize ASYNC}" 

[2024-11-18T19:19:08,448][ERROR][o.e.b.ElasticsearchUncaughtExceptionHandler] [test-cluster-0] fatal error in thread [elasticsearch[test-cluster-0][search][T#47]], exiting java.lang.NoClassDefFoundError: org/elasticsearch/xpack/ml/job/categorization/CategorizationAnalyzer |  
-- | --
  | at org.elasticsearch.xpack.esql.expression.function.grouping.Categorize.toEvaluator(Categorize.java:112) |  
  | at org.elasticsearch.xpack.esql.evaluator.EvalMapper.toEvaluator(EvalMapper.java:55) |  
  | at org.elasticsearch.xpack.esql.planner.LocalExecutionPlanner.planEval(LocalExecutionPlanner.java:410) |  
  | at org.elasticsearch.xpack.esql.planner.LocalExecutionPlanner.plan(LocalExecutionPlanner.java:197) |  
  | at org.elasticsearch.xpack.esql.planner.LocalExecutionPlanner.planAggregation(LocalExecutionPlanner.java:242) |  
  | at org.elasticsearch.xpack.esql.planner.LocalExecutionPlanner.plan(LocalExecutionPlanner.java:189)

...

Caused by: java.lang.ClassNotFoundException: org.elasticsearch.xpack.ml.job.categorization.CategorizationAnalyzer |  
-- | --
  | at java.base/java.net.URLClassLoader.findClass(URLClassLoader.java:445) |  
  | at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:595) |  
  | at java.base/java.net.FactoryURLClassLoader.loadClass(URLClassLoader.java:870) |  
  | at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:528)

I don't know how to fix this - if I leave x-pack-ml here, we get Jar hell when Elasticsearch starts:

	at org.elasticsearch.server@9.0.0-SNAPSHOT/org.elasticsearch.bootstrap.Elasticsearch.main(Elasticsearch.java:76)
Caused by: java.lang.IllegalStateException: jar hell!
class: com.ibm.icu.impl.Assert
jar1: /Users/ioana/projects/elasticsearch/distribution/archives/darwin-tar/build/install/elasticsearch-9.0.0-SNAPSHOT/modules/x-pack-ml/icu4j-68.2.jar
jar2: /Users/ioana/projects/elasticsearch/distribution/archives/darwin-tar/build/install/elasticsearch-9.0.0-SNAPSHOT/modules/x-pack-esql-core/icu4j-68.2.jar
	at org.elasticsearch.base@9.0.0-SNAPSHOT/org.elasticsearch.jdk.JarHell.checkClass(JarHell.java:316)
	at org.elasticsearch.base@9.0.0-SNAPSHOT/org.elasticsearch.jdk.JarHell.checkJarHell(JarHell.java:234)
	at org.elasticsearch.server@9.0.0-SNAPSHOT/org.elasticsearch.plugins.PluginsUtils.checkBundleJarHell(PluginsUtils.java:319)
	... 9 more

inference and ml seem to have many common dependencies and AFAIK esql is the only plugin that seems to need both.

Solved this by no longer using SemanticQueryBuilder - this means we no longer need to extend from x-pack-inference.

I'm not sure how to make both search and ml work here, we should figure out how to deal with the dependency other than deferring it. @jan-elastic @ChrisHegarty do you have any idea whether it is ok to remove x-pack-ml here, or how to make it work?

it is ok to remove x-pack-ml here, or how to make it work?

It's okay to remove it because we added it to x-pack-esql-core - x-pack-esql-core now extends x-pack-ml and x-pack-esql extends x-pack-esql-core.
When I simply removed x-pack-ml and replaced it x-pack-inference, the CATEGORIZE tests would start to fail. So I know this should be safe, because I can see CATEGORIZE works as expected (none of the tests for it fail).

we should figure out how to deal with the dependency other than deferring it.

I agree - I went back and forth on this and this is the best we could come up with:

• we could move SemanticQueryBuilder in x-pack-core - this would mean moving some additional classes like SemanticTextFieldMapper, maybe more, I haven't unravelled this one.
• however even if we can use SemanticQueryBuilder in ES|QL this would only be temporary - the reason for this is that for _search we want the match query to actually support semantic_text (Match query support for semantic_text fields - POC DO NOT MERGE #112166 - a POC and the actual implementation will be done in the following weeks).
• if you take a look at the match query POC it looks like MatchQueryBuilder has semantic_text support - MatchQueryBuilder is in server so we should be able to use without dependency issues (we already use it).

So my take is that moving SemanticQueryBuilder does not gain us much if support in MatchQueryBuilder is coming soon. And I think that once that's coming, we might be able to revert build.gradle to what it was originally.
I captured that in a bullet point in #115103

It's a pity we can't access this constant from the ML plugin 😓

What prevents us from accessing this constant from the ML plugin? Is it related to the dependency issue?

These are not from the ML plugin - they are from the inference plugin:

We can import things from the ML plugin, but once we extend the inference plugin we end up with jar hell.

I removed this test - it failed for serverless tests on mixed version clusters with Invalid EsField type [SemanticTextEsField]. However:

• this test is a kind of a hack - it puts the full internal representation for the semantic_text field which will change, so this test will need change to use the test inference plugin so that the embeddings are internally generated - but then we have the same problem as Add mixed mode BWC testing support for test-specific plugins #115166

• the amount of CSV tests we have already cover the fact that for ESQL the block loader is capable of only returning the original text content

• semantic_text is only available in snapshots - so not available for serverless - the fact that this test fails has no meaning for serverless prod

Having a yml test for semantic_text is better, if serverless is affected, can this be muted on serverless? Better keep it for stateful if possible.

I reverted the removal and muted the test on serverless.
Also keeping track in #115103 to not forget to unmute this test in serverless once the changes here make it in 9.0.0 snapshot

I'm not sure how to make both search and ml work here, we should figure out how to deal with the dependency other than deferring it. @jan-elastic @ChrisHegarty do you have any idea whether it is ok to remove x-pack-ml here, or how to make it work?

The first inferenceId is used here, match works with one inferenceId only, however multiple inferenceId can be defined in SemanticTextEsField. it will be appreciated if there are some comments here, and if we don't deal with multiple inference ids, does it make sense to keep just one inference id in SemanticTextEsField?

In case there are multiple inferenceIds, do we want to throw an exception here? Similar with what is done in Verifier.

In case there are multiple inferenceIds, do we want to throw an exception here? Similar with what is done in Verifier.

I guess it does not hurt to throw an exception - but at this point in the execution, the Verifier already checked and returned an error if we are dealing with multiple inference ids.

Is IllegalStateException a 400 or 500?

Please consider decoupling this from IndexResolver and make an InferenceResolver to handle inference related tasks.

More positive tests will be appreciated. How does it work with multi-valued fields? More combinations(conjunction) with other functions and operators, before/after stats/eval when it works, for the completeness. There could be surprises, without testing it out, it is hard to tell.

Same as match-function, more variety of queries will be appreciated.

More negative tests will be appreciated, we have code to detect errors, without having negative tests to cover those code paths, it is hard to tell whether they work as expected. Here are a few scenarios that I can think of:

• Match on semantic_text fields with multiple inference_ids
• Match on multiple indices that have a common index pattern, the field names is the same, however they may have different data types - text, keyword, semantic_text
• QSTR or KQL with semantic_text field in the index mapping, they are search functions as well.
• Include semantic_text in MatchTests and MatchOperatorTests.

Having a yml test for semantic_text is better, if serverless is affected, can this be muted on serverless? Better keep it for stateful if possible.

Could you add this case if it hasn't been covered yet? Schema is here.

+ curl -u elastic:password -v -X POST 'localhost:9200/_query?format=txt&pretty' -H 'Content-Type: application/json' '-d
{
  "query": "from testidx00 | eval x = textfield | where match(x, \"live\")"
}
'
{
  "error" : {
    "root_cause" : [
      {
        "type" : "null_pointer_exception",
        "reason" : "Cannot invoke \"org.elasticsearch.inference.InferenceResults.getClass()\" because \"this.inferenceResults\" is null"
      }
    ],
    "type" : "null_pointer_exception",
    "reason" : "Cannot invoke \"org.elasticsearch.inference.InferenceResults.getClass()\" because \"this.inferenceResults\" is null",
    "suppressed" : [
      {
        "type" : "exception",
        "reason" : "1 further exceptions were dropped"
      },
      {
        "type" : "task_cancelled_exception",
        "reason" : "parent task was cancelled [cancelled on failure]"
      }
    ]
  },
  "status" : 500
}

+ curl -u elastic:password -v -X POST 'localhost:9200/_query?format=txt&pretty' -H 'Content-Type: application/json' '-d
{
  "query": "from testidx04 | eval x = textfield | where match(x, \"live\")"
}
'
              textfield              |          textfield.keyword          |                  x                  
-------------------------------------+-------------------------------------+-------------------------------------
live long and prosper 04 text keyword|live long and prosper 04 text keyword|live long and prosper 04 text keyword

After thinking about it more, I wonder if we should allow multiple inference ids on the same field, if the inference ids come from different indices, and each index has only one inference id on this field. This is more like a usability issue.

If I have multiple indices that have the same semantic text field name, I couldn't find a way to query them in at once. Does it look like a valid use case? Schema is here.

Thanks for fixing this!
+ curl -u elastic:password -v -X POST 'localhost:9200/_query?format=txt&pretty' -H 'Content-Type: application/json' '-d
{
  "query": "from testidx00, testidx01 | where match(textfield, \"live\")"
}
'

{
  "error" : {
    "root_cause" : [
      {
        "type" : "verification_exception",
        "reason" : "Found 1 problem\nline 1:41: [MATCH] function cannot operate on [textfield] because it is configured with multiple inference IDs."
      }
    ],
    "type" : "verification_exception",
    "reason" : "Found 1 problem\nline 1:41: [MATCH] function cannot operate on [textfield] because it is configured with multiple inference IDs."
  },
  "status" : 400
}

+ curl -u elastic:password -v -X POST 'localhost:9200/_query?format=txt&pretty' -H 'Content-Type: application/json' '-d
{
  "query": "from testidx00, testidx01 | where match(textfield::keyword, \"live\") | eval k = textfield::keyword | keep k"
}
'

{
  "error" : {
    "root_cause" : [
      {
        "type" : "verification_exception",
        "reason" : "Found 1 problem\nline 1:41: [MATCH] function cannot operate on [textfield::keyword], which is not a field from an index mapping"
      }
    ],
    "type" : "verification_exception",
    "reason" : "Found 1 problem\nline 1:41: [MATCH] function cannot operate on [textfield::keyword], which is not a field from an index mapping"
  },
  "status" : 400
}

@fang-xing-esql , the problem with multiple inference IDs is relevance. It does not make sense to mix multiple inference IDs from the scoring point of view, as different models will get different scores that can't directly be combined. That's the main reason it is not allowed on the _search endpoint.

In _search, users can combine different inference ids using different query clauses in _search, that can the be further adjusted via boosting.

Even if it's technically possible to do this from a filtering PoV, it won't make sense to do it once we combine with score.

Can we take this as a follow up item?
Since this restriction is also present in _search, for all the reasons @carlosdelest mentioned, I thought having it in ES|QL is reasonable for now.
The time where it becomes relevant to lift this restriction is when we introduce RRF, in that case we might want to allow RRF between two subqueries that query the same field on different indices: WHERE where match(textfield, "live") AND _index == "testidx00" and WHERE where match(textfield, "live") AND _index == "testidx01".

Yes, this can be addressed as a follow up.

There is another scenario that I can think of - there are two indices with the same field name, one is semantic text, the other is a text field for example. If I query each of them individually, I got expected results, however if I query both of them together, I got empty results. It is similar to an issue that I brought up to Carlos on his PR, however, semantic text has a different type of query for now, perhaps it has to be taken care of in a different way.

+ curl -u elastic:password -v -X POST 'localhost:9200/_query?format=txt&pretty' -H 'Content-Type: application/json' '-d
{
  "query": "from testidx00 | where match(textfield, \"live\")"
}
'

          textfield           
------------------------------
live long and prosper 00 elser

+ curl -u elastic:password -v -X POST 'localhost:9200/_query?format=txt&pretty' -H 'Content-Type: application/json' '-d
{
  "query": "from testidx04  | where match(textfield, \"live\")"
}
'

              textfield              |          textfield.keyword          
-------------------------------------+-------------------------------------
live long and prosper 04 text keyword|live long and prosper 04 text keyword

+ curl -u elastic:password -v -X POST 'localhost:9200/_query?format=txt&pretty' -H 'Content-Type: application/json' '-d
{
  "query": "from testidx00, testidx04 | where match(textfield::keyword, \"live\")  | eval k = textfield::keyword | keep k"
}
'

       k       
---------------

Good find! I added this as a follow up of things to do before we take this out of snapshot in #115103.