ES|QL - LuceneTopNSourceOperator data partition strategy uses ContextIndexSearcher slicing#143133
Conversation
…ne-top-n-data-partition-strategy
…p-n-data-partition-strategy' into enhancement/esql-lucene-top-n-data-partition-strategy
…into enhancement/esql-topn-data-partition-strategy # Conflicts: # x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/aggregate/SumOverTimeTests.java
…-data-partition-strategy # Conflicts: # x-pack/plugin/esql/compute/src/main/java/org/elasticsearch/compute/lucene/query/LuceneSliceQueue.java
|
Hi @carlosdelest, I've created a changelog YAML for you. |
…-partition-strategy' into enhancement/esql-topn-data-partition-strategy
…-data-partition-strategy
|
Hi @carlosdelest, I've updated the changelog YAML for you. |
…-data-partition-strategy # Conflicts: # x-pack/plugin/esql/compute/src/main/java/org/elasticsearch/compute/lucene/query/LuceneSourceOperator.java
…-partition-strategy' into enhancement/esql-topn-data-partition-strategy
…-partition-strategy' into enhancement/esql-topn-data-partition-strategy
ioanatia
left a comment
There was a problem hiding this comment.
looks good, I was wondering if we can further simplify some checks now that the node level materialization is enabled on release builds
|
|
||
| private static AutoStrategy topNAutoStrategy() { | ||
| return unusedLimit -> { | ||
| if (EsqlCapabilities.Cap.ENABLE_REDUCE_NODE_LATE_MATERIALIZATION.isEnabled()) { |
There was a problem hiding this comment.
looks like this capability is always enabled - see #142834
do we still need this check - or could we always use LuceneSliceQueue.PartitioningStrategy.SEGMENT
There was a problem hiding this comment.
Ooooh, that happened!
Thanks for checking, I removed the cap check in 15cd5aa
| private static Matcher<String> expectedAutoPartition(String index, String idxPartition) { | ||
| private static Matcher<String> expectedAutoPartition(String index, String idxPartition, boolean score) { | ||
| boolean lateMaterializationEnabled = EsqlCapabilities.Cap.ENABLE_REDUCE_NODE_LATE_MATERIALIZATION.isEnabled(); | ||
| if (score && lateMaterializationEnabled == false) { |
There was a problem hiding this comment.
same here - the cap is always enabled - do we still need this check since this is a test only for single node? (so it's not used for bwc tests).
| * </ul> | ||
| */ | ||
| private static PartitioningStrategy highSpeedAutoStrategy(Query query) { | ||
| public static PartitioningStrategy highSpeedAutoStrategy(Query query) { |
There was a problem hiding this comment.
nitpick - but do we need this to be public 🙈
|
@elasticsearchmachine run elasticsearch-ci/part-3 |
|
@elasticsearchmachine run elasticsearch-ci/part-3 |
…IndexSearcher slicing (elastic#143133)
Related #141770
#142128 used high speed partitioning for LuceneTopNSourceOperator. That improved p90 timing but worsened p50 timing, and was reverted in #142453.
Analyzing the Lucene query execution differences, it was clear that ESQL used more slices than the DSL. An excessive number of slices was harming the average use case.
The
SEGMENTdata partitioning strategy used the LuceneIndexSearcher.slices()static method for calculating the slices. This is not what the DSL does, as it uses the ContextIndexSearcher that in turn overrides some of the default Lucene behavior.This PR modifies the
SEGMENTstrategy by using theIndexSearcherthat it receives as an instance to calculate the slices via thegetSlices()method, instead of using the static IndexSearcher method from Lucene.This is consistent with the DSL and provides an upper limit on the number of slices created based on the available search threads available.