[ES|QL] Replace RoundTo linear search evaluator with manual evaluators#131733
Conversation
x-pack/plugin/esql/build.gradle
Outdated
| "src/main/java/org/elasticsearch/xpack/esql/parser/EsqlBaseParser*.java", | ||
| "src/main/generated/**/*.java", | ||
| "src/main/generated-src/generated/**/*.java" | ||
| "src/main/generated-src/**/*.java" |
There was a problem hiding this comment.
Skip the format check on all files under src/main/generated-src/, I don't see this subfolder src/main/generated-src/generated/.
There was a problem hiding this comment.
I liked that we were running spotless on these - it forces us to write the templates in a way that keeps the style consistent. It's a pain though. Can you try removing this and fixing the templates? I know it's really picky and annoying. But it helps keep the code more readable.
There was a problem hiding this comment.
Thanks for the tricks!
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
|
Pinging @elastic/es-perf (Team:Performance) |
|
Hi @fang-xing-esql, I've created a changelog YAML for you. |
x-pack/plugin/esql/build.gradle
Outdated
| "src/main/java/org/elasticsearch/xpack/esql/parser/EsqlBaseParser*.java", | ||
| "src/main/generated/**/*.java", | ||
| "src/main/generated-src/generated/**/*.java" | ||
| "src/main/generated-src/**/*.java" |
There was a problem hiding this comment.
I liked that we were running spotless on these - it forces us to write the templates in a way that keeps the style consistent. It's a pain though. Can you try removing this and fixing the templates? I know it's really picky and annoying. But it helps keep the code more readable.
| } | ||
|
|
||
| @Evaluator(extraName = "5") | ||
| static double process(double field, @Fixed double p0, @Fixed double p1, @Fixed double p2, @Fixed double p3, @Fixed double p4) { |
There was a problem hiding this comment.
Could you add a comment to all of the process methods that say that it's basically a hand unrolled binary search?
| @Fixed double p5, | ||
| @Fixed double p6 | ||
| ) { | ||
| if (field < p3) { |
There was a problem hiding this comment.
p3 is the median of these 7 numbers.
|
Thanks for reviewing @nik9000! Comments to those manual binary search evaluators are added. |
…-tracking * upstream/main: (106 commits) Pipelines: Add `created_date` and `modified_date` (elastic#130847) add thread pool change availability (elastic#131734) Add failure store availability info / and port over privileges (elastic#131729) add availability information for ssl handshake timeout settings (elastic#131786) add availability information for rescore_vector (elastic#131710) add availability to oversample value of 0 (elastic#131707) clarify hnsw filter heuristic setting availability (elastic#131715) add availability info for default heap dump path change (elastic#131713) clarify default algorithms per stack version (elastic#131728) Refine error messages in `Fork` for correctness and clarity. (elastic#131701) [ES|QL] Replace RoundTo linear search evaluator with manual evaluators (elastic#131733) ESQL: Fix buildParams in tests with --configuration-cache (elastic#131826) Unmute `CrossClusterEsqlRCS2EnrichUnavailableRemotesIT#testEsqlEnrichWithSkipUnavailable` (elastic#131916) Allow templates for `.chat-*` index template (elastic#131914) ESQL: Fix NPE on empty to_lower/to_upper call (elastic#131917) Fix score computation in ES91Int4VectorsScorer (elastic#131905) Register a blob cache long counter metric for total evicted regions (elastic#131862) Move plan attribute resolution to its own component (elastic#131830) Make restore support multi-project (elastic#131661) Use logically more correct expression (elastic#131869) ...
This is a subtask of #131341.
The
RoundToLinearSearchEvaluatordoes not outperform the manual evaluators, in some cases(data volume dependent), it’s even slower than theDateTruncDatetimeEvaluator.Below are detailed profiling results (copied from PR #131341). This PR proposes replacing the linear search evaluators with manual ones. If the number of buckets is less than 11, the corresponding manual evaluator will be used instead. This helps performance regression tests when #131341 is merged.
The correctness of these manual evaluators is well covered by the existing
RoundToTests.There may be more opportunities to fine-tune performance by experimenting with a wider variety of bucket counts and data volumes. From the current observations, smaller data volumes appear to be more sensitive to the choice of evaluator.