-
Notifications
You must be signed in to change notification settings - Fork 25.2k
[ES|QL] COMPLETION command logical plan optimizer #126763
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
[ES|QL] COMPLETION command logical plan optimizer #126763
Conversation
Pinging @elastic/es-analytical-engine (Team:Analytics) |
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.
LGTM - have a question about some code being tested
@@ -238,6 +245,53 @@ public void testSelectivelyPushDownFilterPastFunctionAgg() { | |||
assertEquals(expected, new PushDownAndCombineFilters().apply(fb)); | |||
} | |||
|
|||
// from ... | where a > 1 | COMPLETION "some prompt" WITH reranker AS completion | where b < 2 and match(completion, some text) | |||
// => ... | where a > 1 AND b < 2| COMPLETION "some prompt" WITH reranker AS completion | match(completion, some text) | |||
public void testPushDownFilterPastCompletion() { |
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 might have missed this - is the PushDownAndCombineLimits
change being tested?
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.
Added some test for PushDownAndCombineLimits
(it is wild but none were created for other commands)
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.
The LogicalPlanOptimizerTests
have testCombineLimits
and testMultipleCombineLimits
, but indeed, I don't immediately see tests for the case where there are compatible plans between the two limits. Thanks a lot for the added tests.
ee35f39
to
4eab7d5
Compare
The
COMPLETION
command is expected to be costly because it is doing an inference for each row of the dataset.This PR tries to limit the number of rows to process by applying the same logical plan than others
GeneratingPlan
(Eval
,Enrich
,RegexExtract
, ...):COMPLETION
command when possibleCOMPLETION
command when possibleWork for the completion command is tracked in: #124405
PS: the command is available only for snapshot build.