Skip to content

ES|QL: MMR more fixes#144225

Merged
ioanatia merged 18 commits intoelastic:mainfrom
ioanatia:mmr_planner_fix
Mar 30, 2026
Merged

ES|QL: MMR more fixes#144225
ioanatia merged 18 commits intoelastic:mainfrom
ioanatia:mmr_planner_fix

Conversation

@ioanatia
Copy link
Copy Markdown
Member

@ioanatia ioanatia commented Mar 13, 2026 •
edited
Loading

still a draft, still in progress
Fixes various MMR bugs:

  1. closes [CI] EsqlNodeSubclassTests testTransform {class org.elasticsearch.xpack.esql.plan.logical.MMR} failing #142674

The way we mutate queryVector in the constructor, wrapping it in a to_dense_vector function call, causes these tests to fail:

elasticsearch/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/MMR.java

Lines 45 to 58 in 12a10a9

public MMR(
Source source,
LogicalPlan child,
Attribute diversifyField,
Expression limit,
@Nullable Expression queryVector,
@Nullable Expression options
) {
super(source, child);
this.diversifyField = diversifyField;
this.limit = limit;
this.queryVector = processQueryVector(queryVector);
this.options = options;
}

This is because queryVector is part of the NodeInfo for MMR and it seems to be an anti-pattern to mutate it in the constructor (I couldn't find other LogicalPlans where we do this).

  1. We don't check that the query vector is actually foldable and resolved to a constant. So it's possible to do | MMR my_vector ON my_other_vector LIMIT 10. Surprisingly, the ES|Q succeeds, because we evaluate the query vector to null in MMRExec.

  2. (in progress) Query vectors that are keywords, can lead to 500 errors:

MMR "ef" ON rgb_vector LIMIT 10

error:

{
  "error": {
    "root_cause": [
      {
        "type": "class_cast_exception",
        "reason": "class java.lang.Float cannot be cast to class java.util.ArrayList (java.lang.Float and java.util.ArrayList are in module java.base of loader 'bootstrap')"
      }
    ],
    "type": "class_cast_exception",
    "reason": "class java.lang.Float cannot be cast to class java.util.ArrayList (java.lang.Float and java.util.ArrayList are in module java.base of loader 'bootstrap')"
  },
  "status": 500
}
  1. We don't validate that lambda needs to be a number:
FROM colors
| WHERE rgb_vector is not null
| LIMIT 100
| MMR "efefef" ON rgb_vector LIMIT 10 WITH {"lambda": "abc"}

error:

{
  "error": {
    "root_cause": [
      {
        "type": "verification_exception",
        "reason": """Found 1 problem
line 2:58: class org.apache.lucene.util.BytesRef cannot be cast to class java.lang.Double (org.apache.lucene.util.BytesRef is in module org.apache.lucene.core@10.4.0 of loader 'app'; java.lang.Double is in module java.base of loader 'bootstrap')"""
      }
    ],
    "type": "verification_exception",
    "reason": """Found 1 problem
line 2:58: class org.apache.lucene.util.BytesRef cannot be cast to class java.lang.Double (org.apache.lucene.util.BytesRef is in module org.apache.lucene.core@10.4.0 of loader 'app'; java.lang.Double is in module java.base of loader 'bootstrap')"""
  },
  "status": 400
}

@ioanatia ioanatia added >non-issue Team:Search Relevance Meta label for the Search Relevance team in Elasticsearch :Search Relevance/ES|QL Search functionality in ES|QL labels Mar 13, 2026
@ioanatia ioanatia requested a review from carlosdelest March 19, 2026 13:43
@ioanatia ioanatia marked this pull request as ready for review March 19, 2026 13:43
@elasticsearchmachine
Copy link
Copy Markdown
Collaborator

elasticsearchmachine commented Mar 19, 2026

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

carlosdelest
carlosdelest reviewed Mar 19, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@carlosdelest carlosdelest left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! A couple of things to address:

  • A possible ClassCastException when using a non-double value for lambda
  • A test for the zero case for lambda

@ioanatia ioanatia requested a review from tteofili March 27, 2026 16:28
tteofili
tteofili approved these changes Mar 30, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@tteofili tteofili left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ioanatia ioanatia merged commit 11766ea into elastic:main Mar 30, 2026
35 checks passed
@ioanatia ioanatia deleted the mmr_planner_fix branch March 30, 2026 15:14
mamazzol pushed a commit to mamazzol/elasticsearch that referenced this pull request Mar 30, 2026
@ioanatia @mamazzol
ES|QL: MMR more fixes (elastic#144225)
f78557e
mouhc1ne pushed a commit to shmuelhanoch/elasticsearch that referenced this pull request Mar 31, 2026
@ioanatia @mouhc1ne
ES|QL: MMR more fixes (elastic#144225)
a178be4
@ioanatia ioanatia mentioned this pull request Mar 31, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@carlosdelest carlosdelest carlosdelest left review comments

@tteofili tteofili tteofili approved these changes

Assignees

No one assigned

Labels

>non-issue :Search Relevance/ES|QL Search functionality in ES|QL Team:Search Relevance Meta label for the Search Relevance team in Elasticsearch v9.4.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[CI] EsqlNodeSubclassTests testTransform {class org.elasticsearch.xpack.esql.plan.logical.MMR} failing

4 participants

@ioanatia @elasticsearchmachine @tteofili @carlosdelest