ES|QL: Enable semantic search in FORK #125960
Conversation
ioanatia
added
>non-issue
Team:Analytics
elasticsearchmachine
added
the
Team:Search Relevance
|
Pinging @elastic/es-search-relevance (Team:Search Relevance) |
elasticsearchmachine
removed
the
Team:Analytics
ioanatia
requested review from
carlosdelest,
tteofili,
fang-xing-esql and
afoucret
| Holder<Boolean> hasFullTextFunction = new Holder<>(false); | ||
| p.forEachExpression(FullTextFunction.class, unused -> hasFullTextFunction.set(true)); | ||
|
|
||
| if (p instanceof Fork fork) { |
There was a problem hiding this comment.
Should Fork implement forEachExpression instead? That would make things easier for the clients that will not need to take it into account, and be in line with the Composite pattern...
There was a problem hiding this comment.
No, Fork should not implement forEachExpression.
There is a slippery slope if we decide to do that since in the future we might decide we need to implement anyMatch, forEachExpressionUp, forEachExpressionDown etc and that's not what we want.
We either treat Fork as the outlier it currently is (because we have nothing like it in ES|QL atm) or we go back and make Fork a n-ary plan (it is currently a UnaryPlan).
If Fork sub plans are treated as children then we would most likely not need to make any big changes to QueryBuilderResolver.
While it sound good in theory to just make Fork a n-ary plan, we tried this already when we did the POC and found out it has its own set of challenges.
So for now I am focusing on getting Fork to work as it should even if it means having this special handling in a couple of places.
There was a problem hiding this comment.
If you already went down that route, that's enough to me 😅
| @@ -91,5 +108,15 @@ public FullTextFunctionsRewritable rewrite(QueryRewriteContext ctx) throws IOExc | |||
| } | |||
| return updated.get() ? new FullTextFunctionsRewritable(newPlan) : this; | |||
| } | |||
|
|
|||
| private LogicalPlan transformPlan(LogicalPlan plan, Function<FullTextFunction, ? extends Expression> rule) { | |||
| return plan.transformExpressionsDown(FullTextFunction.class, rule).transformDown(Fork.class, fork -> { | |||
There was a problem hiding this comment.
Same here, we may override transformExpressionsDown for Fork
tracked in #121950
semantic search does not currently work in FORK branches - this was listed as a follow up in #121950
The fix requires modifying the
QueryBuilderResolverto include theFORKbranches when doing the query builder rewrite forFullTextFunctionexpressions.The fix is tested through CSV tests that do semantic search in FORK branches.