Skip to content

ES|QL: Add support for RRF options #134227

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 7 commits into from
Sep 10, 2025
Merged

ES|QL: Add support for RRF options #134227

merged 7 commits into from
Sep 10, 2025

Conversation

@ioanatia
Copy link
Contributor

@ioanatia ioanatia commented Sep 5, 2025

Related:

#123389
#131078

Adds support for options for RRF:

FROM books METADATA _id, _score, _index
| FORK ( WHERE title:"Tolkien" | SORT _score, _id DESC | LIMIT 3 )
       ( WHERE author:"Tolkien" | SORT _score, _id DESC | LIMIT 3)
| FUSE rrf WITH {"rank_constant": 60, "weights": { "fork1": 0.3, "fork2": 0.7 } }

So the scope of this PR is:

  • make lexer/parser changes to allow options (the good part when we add linear combination support we don't need to change the grammar)
  • validate options
  • apply the options at the operator level when we calculate the scores

Things that remain for FUSE that are not done here:

  • support for linear combination
  • support to specify the grouping columns, score column and discriminator column
  • CCS support
  • more verifications for FUSE, e.g. there needs to be a limit / pipeline breaker before FUSE etc

@ioanatia ioanatia added >non-issue Team:Search - Relevance The Search organization Search Relevance team v9.2.0 :Search Relevance/ES|QL Search functionality in ES|QL labels Sep 5, 2025
@elasticsearchmachine elasticsearchmachine added Team:Search Relevance Meta label for the Search Relevance team in Elasticsearch and removed Team:Search - Relevance The Search organization Search Relevance team labels Sep 5, 2025
@elasticsearchmachine
Copy link
Collaborator

elasticsearchmachine commented Sep 5, 2025

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

carlosdelest
carlosdelest approved these changes Sep 8, 2025
View reviewed changes
Copy link
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 questions on the grammar and other minor things.

.../plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/fuse/FuseScoreEval.java Outdated
}
Double numericValue = null;
try {
numericValue = EsqlDataTypeConverter.stringToDouble(value.fold(FoldContext.small()).toString());
Copy link
Member

@carlosdelest carlosdelest Sep 8, 2025

Choose a reason for hiding this comment

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

Why the extra step converting from String? Shouldn't it be a Number already when folded, given it's a numeric data type?

Copy link
Contributor Author

@ioanatia ioanatia Sep 9, 2025

Choose a reason for hiding this comment

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

yep this felt bad when I wrote it initially - forgot to reconsider the approach - removed this check here and used the folded value.

...ck/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/planner/LocalExecutionPlanner.java
RrfConfig config = (RrfConfig) fuse.fuseConfig();

return source.with(
new RrfScoreEvalOperator.Factory(discriminatorPosition, scorePosition, config.rankConstant(), config.weights()),
Copy link
Member

@carlosdelest carlosdelest Sep 8, 2025

Choose a reason for hiding this comment

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

Should we pass the RrfConfig itself so it's easier to extend if needed?

Copy link
Contributor Author

@ioanatia ioanatia Sep 9, 2025

Choose a reason for hiding this comment

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

Had the same thought initially.
But RrfScoreEvalOperator lives in esql/compute, so it can't reference RrfConfig.
I can look into maybe moving RrfConfig to esql/compute as a follow up.

Copy link
Member

@carlosdelest carlosdelest Sep 9, 2025

Choose a reason for hiding this comment

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

I see 😞 . Moving RrfConfig may make sense and can be explored as a follow up.

x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/VerifierTests.java
}

public void testFuse() {
String queryPrefix = "from test metadata _score, _index, _id | fork (where true) (where true)";
Copy link
Member

@carlosdelest carlosdelest Sep 8, 2025

Choose a reason for hiding this comment

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

Let's check capability to avoid release build errors

afoucret
afoucret reviewed Sep 9, 2025
View reviewed changes
...ugin/esql/compute/src/main/java/org/elasticsearch/compute/operator/RrfScoreEvalOperator.java Outdated
Comment on lines 68 to 69
var weight = weights.get(discriminator);
weight = weight == null ? 1 : weight;
Copy link
Contributor

@afoucret afoucret Sep 8, 2025

Choose a reason for hiding this comment

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

❓ Should we use getOrDefault instead?

@ioanatia ioanatia merged commit be9d4e9 into elastic:main Sep 10, 2025
34 checks passed
@ioanatia ioanatia deleted the fuse_rrf_options branch September 10, 2025 08:29
@ioanatia ioanatia mentioned this pull request Jul 18, 2025
Open
17 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@afoucret afoucret afoucret left review comments

@carlosdelest carlosdelest carlosdelest 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.2.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@ioanatia @elasticsearchmachine @afoucret @carlosdelest