-
Notifications
You must be signed in to change notification settings - Fork 25.6k
ESQL: Add asynchronous pre-optimization step for logical plan #131440
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
ESQL: Add asynchronous pre-optimization step for logical plan #131440
Conversation
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
benchmarks/src/main/java/org/elasticsearch/benchmark/_nightly/esql/QueryPlanningBenchmark.java
Outdated
Show resolved
Hide resolved
.../esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanPreOptimizerTests.java
Outdated
Show resolved
Hide resolved
.../esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanPreOptimizerTests.java
Outdated
Show resolved
Hide resolved
.../esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanPreOptimizerTests.java
Outdated
Show resolved
Hide resolved
|
@astefan Got a couple of CI failures caused by the branch being outdated but all is green right 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.
Few notes and comments:
- The
text_embeddingfunction is currently the main recipient of this PR at this time. It is supposed to call an external service and create what is, essentially, an array offloats that can be later used in the query - due to its nature, the performance of the
text_embeddingfunction mainly depends on the external service availability, which can be a bottleneck - because this step resembles the one of the
enrichpolicies discovery orlookupindices resolution, it was decided that resolving thetext_embeddingoutput value is best placed in the overall "discovery" (index resolution, analyzer, logical optimizer, physical optimizer) set of steps. This set of steps is performed on the coordinator node - another argument in favor of using the pre-optimizer step is the one of using the Literal that comes out of the
text_embeddingfunction in further optimization rules from the LogicalPlanOptimizer. I personally do not believe this as a strong argument. There could be a similar optimization step on each data node. - also, calling the external service has a cost associated with it. One of the arguments in favor of calling this service on the coordinator node only is that the cost is greatly reduced this way. It is essentially one call. There are some downside to this decision:
- the coordinator becomes a bottleneck
- some steps that still validate the correctness of the query (everything that comes after the pre-optimization step - logical optimizer, physical optimizer, local logical optimizer, local physical optimizer) can wait unnecessarily a long time before replying back to the user with a potentially invalid query response
- any shard/index specific optimizations cannot be applied. For example, if the field that is used in the
text_embeddingfunction has anullvalue or no value on some of the shards, I am assuming that calling the external service for that specific value/field is unnnecessary.
- I think it is OK to start with this pre-optimizer step only for constant values (for example
TEXT_EMBEDDING("Who is Victor Hugo?", "test_dense_inference")only from the point of view of calling the external service only once- there is also a LocalLogicalPlanOptimizer. If there is any shard/index specific behavior my preference would be to to do this "external service" call from each "relevant" data node or to have a heuristic logic that decides which approach is better: one call from coordinator or multiple calls from each data node.
- I am not convinced that the
EsqlSessionshould change the way it's handling the Listeners while calling the entire flow of analyzer, logical optimizer, physical optimizer, but I understand the need to have this call async. If it doesn't break any existent tests/behavior it's ok with me.
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
…king * upstream/main: (100 commits) Term vector API on stateless search nodes (elastic#129902) TEST Fix ThreadPoolMergeSchedulerStressTestIT testMergingFallsBehindAndThenCatchesUp (elastic#131636) Add inference.put_custom rest-api-spec (elastic#131660) ESQL: Fewer serverless docs in tests (elastic#131651) Skip search on indices with INDEX_REFRESH_BLOCK (elastic#129132) Mute org.elasticsearch.indices.cluster.RemoteSearchForceConnectTimeoutIT testTimeoutSetting elastic#131656 [jdk] Resolve EA OpenJDK builds to our JDK archive (elastic#131237) Add optimized path for intermediate values aggregator (elastic#131390) Correctly handling download_database_on_pipeline_creation within a pipeline processor within a default or final pipeline (elastic#131236) Refresh potential lost connections at query start for `_search` (elastic#130463) Add template_id to patterned-text type (elastic#131401) Integrate LIKE/RLIKE LIST with ReplaceStringCasingWithInsensitiveRegexMatch rule (elastic#131531) [ES|QL] Add doc for the COMPLETION command (elastic#131010) ESQL: Add times to topn status (elastic#131555) ESQL: Add asynchronous pre-optimization step for logical plan (elastic#131440) ES|QL: Improve generative tests for FORK [130015] (elastic#131206) Update index mapping update privileges (elastic#130894) ESQL: Added Sample operator NamedWritable to plugin (elastic#131541) update `kibana_system` to grant it access to `.chat-*` system index (elastic#131419) Clarify heap size configuration (elastic#131607) ...
…-tracking * upstream/main: (44 commits) Term vector API on stateless search nodes (elastic#129902) TEST Fix ThreadPoolMergeSchedulerStressTestIT testMergingFallsBehindAndThenCatchesUp (elastic#131636) Add inference.put_custom rest-api-spec (elastic#131660) ESQL: Fewer serverless docs in tests (elastic#131651) Skip search on indices with INDEX_REFRESH_BLOCK (elastic#129132) Mute org.elasticsearch.indices.cluster.RemoteSearchForceConnectTimeoutIT testTimeoutSetting elastic#131656 [jdk] Resolve EA OpenJDK builds to our JDK archive (elastic#131237) Add optimized path for intermediate values aggregator (elastic#131390) Correctly handling download_database_on_pipeline_creation within a pipeline processor within a default or final pipeline (elastic#131236) Refresh potential lost connections at query start for `_search` (elastic#130463) Add template_id to patterned-text type (elastic#131401) Integrate LIKE/RLIKE LIST with ReplaceStringCasingWithInsensitiveRegexMatch rule (elastic#131531) [ES|QL] Add doc for the COMPLETION command (elastic#131010) ESQL: Add times to topn status (elastic#131555) ESQL: Add asynchronous pre-optimization step for logical plan (elastic#131440) ES|QL: Improve generative tests for FORK [130015] (elastic#131206) Update index mapping update privileges (elastic#130894) ESQL: Added Sample operator NamedWritable to plugin (elastic#131541) update `kibana_system` to grant it access to `.chat-*` system index (elastic#131419) Clarify heap size configuration (elastic#131607) ...
Description
This PR adds a new asynchronous pre-optimization step to the ES|QL logical plan execution pipeline.
The pre-optimization step is positioned between the
Analyzerand theOptimizer, allowing for asynchronous operations to be performed before once the logical plan is analyzed and before it is optimized.Context / Use case
This infrastructure is required for the
TEXT_EMBEDDINGfunction implementation (issue #131022).By evaluating text embeddings before query optimization, we ensure they benefit from all subsequent constant. optimizations.
The
PreMapperwas originally place before theOptimizerbut is has been moved after, so it can be used for this purpose.Key Changes
PRE_OPTIMIZEDstage in theLogicalPlanLogicalPlanPreOptimizerclass for handling asynchronous pre-optimizationLogicalPreOptimizerContextto support the pre-optimization processEsqlSessionto include the pre-optimization step in the execution flow