Approximation Framework Enhancement: Update the BKD traversal logic to improve the performance on skewed data #18439
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Prudhvi Godithi <[email protected]>
Signed-off-by: Prudhvi Godithi <[email protected]>
Member
Author
|
Coming from @msfroh comment #18342 (comment), I will update the PR to remove the changes from NumberFieldMapper class, once I see the benchmark results improve for different datasets that has different numeric types (Related PR from benchmark workloads repo opensearch-project/opensearch-benchmark-workloads#655). Coming from this comment #18313 (comment), in past we have seen regression with With the updated code I have tested with Adding @harshavamsi @kkewwei @getsaurabh02 @msfroh to please take a look. Thanks |
3 tasks
Member
Author
|
{"run-benchmark-test": "id_4"} |
Member
Author
|
{"run-benchmark-test": "id_13"} |
Contributor
|
The Jenkins job url is https://build.ci.opensearch.org/job/benchmark-pull-request/3338/ . Final results will be published once the job is completed. |
Contributor
|
The Jenkins job url is https://build.ci.opensearch.org/job/benchmark-pull-request/3339/ . Final results will be published once the job is completed. |
Contributor
|
❌ Gradle check result for 33d7e68: 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? |
Signed-off-by: Prudhvi Godithi <[email protected]>
Contributor
|
❌ Gradle check result for 03478f6: 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? |
Collaborator
Benchmark ResultsBenchmark Results for Job: https://build.ci.opensearch.org/job/benchmark-pull-request/3339/
|
Collaborator
Benchmark Baseline Comparison ResultsBenchmark Results for Job: https://build.ci.opensearch.org/job/benchmark-compare/109/
|
Member
Author
|
{"run-benchmark-test": "id_13"} |
Contributor
|
The Jenkins job url is https://build.ci.opensearch.org/job/benchmark-pull-request/3340/ . Final results will be published once the job is completed. |
Collaborator
Benchmark ResultsBenchmark Results for Job: https://build.ci.opensearch.org/job/benchmark-pull-request/3340/
|
Collaborator
Benchmark Baseline Comparison ResultsBenchmark Results for Job: https://build.ci.opensearch.org/job/benchmark-compare/110/
|
Collaborator
Benchmark ResultsBenchmark Results for Job: https://build.ci.opensearch.org/job/benchmark-pull-request/3338/
|
Collaborator
Benchmark Baseline Comparison ResultsBenchmark Results for Job: https://build.ci.opensearch.org/job/benchmark-compare/111/
|
Member
Author
|
For From the PR opensearch-project/opensearch-benchmark-workloads#655 I had added more query types which I saw nice improvement for No issues seen with |
3 tasks
Signed-off-by: Prudhvi Godithi <[email protected]>
Member
Author
|
While I add some more tests @msfroh @harshavamsi @kkewwei @bowenlan-amzn @asimmahmood1 can you please review the PR and add your thoughts ? |
prudhvigodithi
requested review from
anasalkouz,
andrross and
ashking94
as code owners
kkewwei
approved these changes
Jun 10, 2025
Contributor
The merge button is gray in my side.😢 |
Member
Author
|
Thanks for your review @kkewwei, I see whats happening, you are not part of |
Signed-off-by: Prudhvi Godithi <[email protected]>
Member
Author
|
Just pushed a commit to fix the |
Contributor
cwperks
approved these changes
Jun 10, 2025
github-project-automation
bot
moved this from In-Review
to In Progress
in Performance Roadmap
github-project-automation
bot
moved this from In Progress
to Done
in Performance Roadmap
Contributor
|
@prudhvigodithi thanks for the change. Could you help me understand what is the memory overhead of the clone? and in worst case, how many such clone trees may exist? when does the cloned tree releases memory? |
Member
Author
|
Hey @rishabhmaurya here is some background on why the |
Contributor
|
@prudhvigodithi so for some giant segment (say > 1 gb) and in worst case, what would be the memory overhead due to clone? |
Member
Author
|
Thanks @rishabhmaurya please check the following details: This is the following test I did for both asc and desc sort. The number of clients 1, 200 warmup-iterations and 100 iterations. The index used is big5 (created with 1 shard) with force merged to 1 segment. The tree has 18 levels of depth asc (
|
q-andy
pushed a commit
to q-andy/OpenSearch
that referenced
this pull request
Jun 11, 2025
…o improve the performance on skewed data (opensearch-project#18439) * Initial commit for skewed datasets Signed-off-by: Prudhvi Godithi <[email protected]> * Approximation optimization Signed-off-by: Prudhvi Godithi <[email protected]> * Fix test Signed-off-by: Prudhvi Godithi <[email protected]> * Revert NumberFieldMapper changes Signed-off-by: Prudhvi Godithi <[email protected]> * Test nyc_taxis with updated NumberFieldMapper Signed-off-by: Prudhvi Godithi <[email protected]> * More optimization for asc sorts Signed-off-by: Prudhvi Godithi <[email protected]> * More optimization for asc sorts Signed-off-by: Prudhvi Godithi <[email protected]> * Revert NumberFieldMapper and add tests Signed-off-by: Prudhvi Godithi <[email protected]> * Updated CHANGELOG.md Signed-off-by: Prudhvi Godithi <[email protected]> * Fix flaky test in ApproximatePointRangeQueryTests Signed-off-by: Prudhvi Godithi <[email protected]> --------- Signed-off-by: Prudhvi Godithi <[email protected]>
Contributor
|
@prudhvigodithi thanks for profiling. The clone way of traversing BKD in reverse order looks very promising. So, in the earlier approach, we were executing way too many intersections with tree in NumericComparator with every collection of competitive hits? Whereas, here we are bounded on the clones of the tree by its height, which sounds way more efficient and thus we see less memory utilization too in addition to latency reductions? |
Member
Author
|
Thats true @rishabhmaurya, since we are traversing reverse in the BKD we directly target the highest docs (desc) and once the size is met we early terminate. |
Member
Author
|
Seen some good improvement with nightlies especially for 
Here are some compariosn runs: |
abhita
pushed a commit
to abhita/OpenSearch
that referenced
this pull request
Jun 17, 2025
…o improve the performance on skewed data (opensearch-project#18439) * Initial commit for skewed datasets Signed-off-by: Prudhvi Godithi <[email protected]> * Approximation optimization Signed-off-by: Prudhvi Godithi <[email protected]> * Fix test Signed-off-by: Prudhvi Godithi <[email protected]> * Revert NumberFieldMapper changes Signed-off-by: Prudhvi Godithi <[email protected]> * Test nyc_taxis with updated NumberFieldMapper Signed-off-by: Prudhvi Godithi <[email protected]> * More optimization for asc sorts Signed-off-by: Prudhvi Godithi <[email protected]> * More optimization for asc sorts Signed-off-by: Prudhvi Godithi <[email protected]> * Revert NumberFieldMapper and add tests Signed-off-by: Prudhvi Godithi <[email protected]> * Updated CHANGELOG.md Signed-off-by: Prudhvi Godithi <[email protected]> * Fix flaky test in ApproximatePointRangeQueryTests Signed-off-by: Prudhvi Godithi <[email protected]> --------- Signed-off-by: Prudhvi Godithi <[email protected]>
65 tasks
neuenfeldttj
pushed a commit
to neuenfeldttj/OpenSearch
that referenced
this pull request
Jun 26, 2025
…o improve the performance on skewed data (opensearch-project#18439) * Initial commit for skewed datasets Signed-off-by: Prudhvi Godithi <[email protected]> * Approximation optimization Signed-off-by: Prudhvi Godithi <[email protected]> * Fix test Signed-off-by: Prudhvi Godithi <[email protected]> * Revert NumberFieldMapper changes Signed-off-by: Prudhvi Godithi <[email protected]> * Test nyc_taxis with updated NumberFieldMapper Signed-off-by: Prudhvi Godithi <[email protected]> * More optimization for asc sorts Signed-off-by: Prudhvi Godithi <[email protected]> * More optimization for asc sorts Signed-off-by: Prudhvi Godithi <[email protected]> * Revert NumberFieldMapper and add tests Signed-off-by: Prudhvi Godithi <[email protected]> * Updated CHANGELOG.md Signed-off-by: Prudhvi Godithi <[email protected]> * Fix flaky test in ApproximatePointRangeQueryTests Signed-off-by: Prudhvi Godithi <[email protected]> --------- Signed-off-by: Prudhvi Godithi <[email protected]>Signed-off-by: TJ Neuenfeldt <[email protected]>
neuenfeldttj
pushed a commit
to neuenfeldttj/OpenSearch
that referenced
this pull request
Jun 26, 2025
…o improve the performance on skewed data (opensearch-project#18439) * Initial commit for skewed datasets Signed-off-by: Prudhvi Godithi <[email protected]> * Approximation optimization Signed-off-by: Prudhvi Godithi <[email protected]> * Fix test Signed-off-by: Prudhvi Godithi <[email protected]> * Revert NumberFieldMapper changes Signed-off-by: Prudhvi Godithi <[email protected]> * Test nyc_taxis with updated NumberFieldMapper Signed-off-by: Prudhvi Godithi <[email protected]> * More optimization for asc sorts Signed-off-by: Prudhvi Godithi <[email protected]> * More optimization for asc sorts Signed-off-by: Prudhvi Godithi <[email protected]> * Revert NumberFieldMapper and add tests Signed-off-by: Prudhvi Godithi <[email protected]> * Updated CHANGELOG.md Signed-off-by: Prudhvi Godithi <[email protected]> * Fix flaky test in ApproximatePointRangeQueryTests Signed-off-by: Prudhvi Godithi <[email protected]> --------- Signed-off-by: Prudhvi Godithi <[email protected]>
11 tasks
tandonks
pushed a commit
to tandonks/OpenSearch
that referenced
this pull request
Aug 5, 2025
…o improve the performance on skewed data (opensearch-project#18439) * Initial commit for skewed datasets Signed-off-by: Prudhvi Godithi <[email protected]> * Approximation optimization Signed-off-by: Prudhvi Godithi <[email protected]> * Fix test Signed-off-by: Prudhvi Godithi <[email protected]> * Revert NumberFieldMapper changes Signed-off-by: Prudhvi Godithi <[email protected]> * Test nyc_taxis with updated NumberFieldMapper Signed-off-by: Prudhvi Godithi <[email protected]> * More optimization for asc sorts Signed-off-by: Prudhvi Godithi <[email protected]> * More optimization for asc sorts Signed-off-by: Prudhvi Godithi <[email protected]> * Revert NumberFieldMapper and add tests Signed-off-by: Prudhvi Godithi <[email protected]> * Updated CHANGELOG.md Signed-off-by: Prudhvi Godithi <[email protected]> * Fix flaky test in ApproximatePointRangeQueryTests Signed-off-by: Prudhvi Godithi <[email protected]> --------- Signed-off-by: Prudhvi Godithi <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
8 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Approximation Framework Enhancement: Update the BKD traversal logic to improve the performance on skewed data
Related Issues
#18341
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.