-
Notifications
You must be signed in to change notification settings - Fork 25.3k
ES|QL: CCS check for FORK/RERANK/COMPLETION #129463
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.
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
@@ -32,7 +33,7 @@ | |||
* A Fork is a n-ary {@code Plan} where each child is a sub plan, e.g. | |||
* {@code FORK [WHERE content:"fox" ] [WHERE content:"dog"] } | |||
*/ | |||
public class Fork extends LogicalPlan implements PostAnalysisPlanVerificationAware { | |||
public class Fork extends LogicalPlan implements PostAnalysisPlanVerificationAware, TelemetryAware { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did sneak this one here, since I think it's the only change we require in order to include FORK in the telemetry payload and did not want to make it a separate PR.
Pinging @elastic/es-analytical-engine (Team:Analytics) |
@@ -3124,7 +3124,7 @@ public void testInvalidJoinPatterns() { | |||
var joinPattern = randomIndexPattern(CROSS_CLUSTER, without(WILDCARD_PATTERN), without(INDEX_SELECTOR)); | |||
expectError( | |||
"FROM " + fromPatterns + " | LOOKUP JOIN " + joinPattern + " ON " + randomIdentifier(), | |||
"invalid index pattern [" + unquoteIndexPattern(joinPattern) + "], remote clusters are not supported in LOOKUP JOIN" | |||
"invalid index pattern [" + unquoteIndexPattern(joinPattern) + "], remote clusters are not supported with LOOKUP JOIN" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know I slightly changed the error message - but I wanted to reuse the same check for all commands cc @smalyshev
although I heard that for LOOKUP JOIN we might be able to remove these soon anyway 🤞
return new LookupJoin(source, p, right, joinFields); | ||
}; | ||
} | ||
|
||
private void checkForRemoteClusters(LogicalPlan plan, Source source, String commandName) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It feels a bit weird that we're running the same code 3 times from 3 different places just to get different error message... I wonder if there isn't a way to somehow capture it better so we don't need to walk the whole (sub-)tree each time anew.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is just reusing the same approach we had for LOOKUP JOIN.
even on main right now, if an ES|QL query has 3 LOOKUP JOIN commands, we are doing the same check 3 times.
I thought that if we had a better way to do it this would have been flagged in #120277
also it's just a temporary limitation, since FORK/COMPLETION/FORK are all going to be tech preview features first and CCS support will be done as part of the transition to GA.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it looks like we also have another place in LogicalPlanBuilder
where walk the plan looking for UnresolvedRelation
:
Lines 316 to 325 in 4e926ae
public PlanFactory visitStatsCommand(EsqlBaseParser.StatsCommandContext ctx) { | |
final Stats stats = stats(source(ctx), ctx.grouping, ctx.stats); | |
return input -> { | |
if (input.anyMatch(p -> p instanceof UnresolvedRelation ur && ur.indexMode() == IndexMode.TIME_SERIES)) { | |
return new TimeSeriesAggregate(source(ctx), input, stats.groupings, stats.aggregates, null); | |
} else { | |
return new Aggregate(source(ctx), input, stats.groupings, stats.aggregates); | |
} | |
}; | |
} |
just wanted to check that it's a pattern we use in some other places as well, not just for CCS check 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah we do such things repeatedly and I am not a big fan of this though I am not sure yet how to fix it. It will also become more complicated with CPS since we may not be able to know whether certain index pattern means remote or local without wider context, which is not available at that point. Not sure what to do with it, maybe these checks can be moved to a different place or checked in some other way. Needs more thinking. I guess it's ok for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get it - there might be ways we could improve it, but as you said it's not clear how.
Hopefully we figure out CCS support before we need to go back and see how these check should be improved😄
💔 Backport failed
You can use sqren/backport to manually backport by running |
related #121950
These commands will be initially released as tech preview and CCS support is not added yet.
We will most likely need a manual backport for RERANK/COMPLETION for 8.19.
With FORK we just didn't get to make the cross cluster tests stable enough.
RERANK and COMPLETION use the inference APIs
COMPLETION allows you to send an LLM prompt and store the output in a column.
RERANK uses the inference rerank API which again uses a configured ML model.
Th the rerank API supports semantic reranking, e.g. you send a list of strings and query string and we return back a list of scores representing how similar the query string is to each of the strings we received.
The problem with both COMPLETION and RERANK is that in order for them to work properly with CCS we would have to enforce a mechanism where we check that the same inference ID is configured in all clusters.
If it's missing in a cluster or if for the same inference ID, you configured it with different LLMs in different clusters, at worst we would fail with a bad error, at best it might look like your query succeeded, but your results would not make much sense.
As part of GAing FORK/COMPLETION/RERANK we will need to support CCS.
But it's not a requirement for tech preview.