-
Notifications
You must be signed in to change notification settings - Fork 25.6k
[ES|QL] Replace RoundTo linear search evaluator with manual evaluators #131733
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] 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be p4 instead?
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.
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.