ES|QL: Add support for RRF options#134227
Conversation
|
Pinging @elastic/es-search-relevance (Team:Search Relevance) |
carlosdelest
left a comment
There was a problem hiding this comment.
LGTM 👍
A couple of questions on the grammar and other minor things.
x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/fuse/FuseConfig.java
Show resolved
Hide resolved
| } | ||
| Double numericValue = null; | ||
| try { | ||
| numericValue = EsqlDataTypeConverter.stringToDouble(value.fold(FoldContext.small()).toString()); |
There was a problem hiding this comment.
Why the extra step converting from String? Shouldn't it be a Number already when folded, given it's a numeric data type?
There was a problem hiding this comment.
yep this felt bad when I wrote it initially - forgot to reconsider the approach - removed this check here and used the folded value.
| RrfConfig config = (RrfConfig) fuse.fuseConfig(); | ||
|
|
||
| return source.with( | ||
| new RrfScoreEvalOperator.Factory(discriminatorPosition, scorePosition, config.rankConstant(), config.weights()), |
There was a problem hiding this comment.
Should we pass the RrfConfig itself so it's easier to extend if needed?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I see 😞 . Moving RrfConfig may make sense and can be explored as a follow up.
| } | ||
|
|
||
| public void testFuse() { | ||
| String queryPrefix = "from test metadata _score, _index, _id | fork (where true) (where true)"; |
There was a problem hiding this comment.
Let's check capability to avoid release build errors
| var weight = weights.get(discriminator); | ||
| weight = weight == null ? 1 : weight; |
There was a problem hiding this comment.
❓ Should we use getOrDefault instead?
Related:
#123389
#131078
Adds support for options for RRF:
So the scope of this PR is:
Things that remain for FUSE that are not done here: