-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Remove filter rewrite optimization for range aggregations when segment is not effective match all #15194
Remove filter rewrite optimization for range aggregations when segment is not effective match all #15194
Conversation
❌ Gradle check result for ae267fa: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
❌ Gradle check result for 110f3b0: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
server/src/main/java/org/opensearch/search/aggregations/bucket/range/RangeAggregator.java
Outdated
Show resolved
Hide resolved
rest-api-spec/src/main/resources/rest-api-spec/test/search.aggregation/40_range.yml
Outdated
Show resolved
Hide resolved
server/src/main/java/org/opensearch/search/aggregations/bucket/range/RangeAggregator.java
Outdated
Show resolved
Hide resolved
...rg/opensearch/search/aggregations/bucket/filterrewrite/FilterRewriteOptimizationContext.java
Outdated
Show resolved
Hide resolved
...rg/opensearch/search/aggregations/bucket/filterrewrite/FilterRewriteOptimizationContext.java
Outdated
Show resolved
Hide resolved
The original improvement had this condition for gating the improvement. I am wondering if we still have it somewhere? |
Moved from checking for explicit |
{"run-benchmark-test": "id_5"} |
The Jenkins job url is https://build.ci.opensearch.org/job/benchmark-pull-request/447/ . Final results will be published once the job is completed. |
Benchmark ResultsBenchmark Results for Job: https://build.ci.opensearch.org/job/benchmark-pull-request/447/
|
Benchmark Baseline Comparison ResultsBenchmark Results for Job: https://build.ci.opensearch.org/job/benchmark-compare/9/
|
No notable benchmark changes after update. |
The backport to
To backport manually, run these commands in your terminal: # Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/OpenSearch/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/OpenSearch/backport-2.x
# Create a new branch
git switch --create backport/backport-15194-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 ef87b397e3c4d9571e6a29a96e438c3e006a0ea7
# Push it to GitHub
git push --set-upstream origin backport/backport-15194-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/OpenSearch/backport-2.x Then, create a pull request where the |
The backport to
To backport manually, run these commands in your terminal: # Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/OpenSearch/backport-2.16 2.16
# Navigate to the new working tree
pushd ../.worktrees/OpenSearch/backport-2.16
# Create a new branch
git switch --create backport/backport-15194-to-2.16
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 ef87b397e3c4d9571e6a29a96e438c3e006a0ea7
# Push it to GitHub
git push --set-upstream origin backport/backport-15194-to-2.16
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/OpenSearch/backport-2.16 Then, create a pull request where the |
…t is not effective match all (opensearch-project#15194) --------- Signed-off-by: Finn Carroll <carrofin@amazon.com> (cherry picked from commit ef87b39)
…t is not effective match all (opensearch-project#15194) --------- Signed-off-by: Finn Carroll <carrofin@amazon.com>
…t is not effective match all (opensearch-project#15194) --------- Signed-off-by: Finn Carroll <carrofin@amazon.com>
Description
The filter rewrite optimization does not immediately disqualify requests containing a top level query with an aggregation. For example, date histogram aggregations consider the top-level query to see if it can be accommodated within the optimization by updating the aggregation ranges.
The range aggregation does not include this logic and currently allows for the optimized filter rewrite path even when a top-level query is present. When this top-level query is not match all we risk returning incorrect results to the user as our optimized path does not account for anything other than the range aggregation bounds.
This change disables the filter rewrite optimization path for range aggregations when the query is not effective match all at the segment level.
Related Issues
Resolves #15169
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.