Skip to content
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

[Backport 2.x] Refactor the filter rewrite optimization #15179

Merged
merged 1 commit into from
Aug 19, 2024

Conversation

opensearch-trigger-bot[bot]
Copy link
Contributor

Backport 170ea27 from #14464.

* Refactor

Split the single Helper classes and move the classes into a new package for any optimization we introduced for search path.
Rename the class name to make it more straightforward and general

Signed-off-by: bowenlan-amzn <bowenlan23@gmail.com>

* Refactor

refactor the canOptimize logic
sort out the basic rule about how to provide data from aggregator, and where to put common logic

Signed-off-by: bowenlan-amzn <bowenlan23@gmail.com>

* Refactor

refactor the data provider and try optimize logic

Signed-off-by: bowenlan-amzn <bowenlan23@gmail.com>

* Refactor

Signed-off-by: bowenlan-amzn <bowenlan23@gmail.com>

* Refactor

extract segment match all logic

Signed-off-by: bowenlan-amzn <bowenlan23@gmail.com>

* Refactor

Signed-off-by: bowenlan-amzn <bowenlan23@gmail.com>

* Refactor

inline class

Signed-off-by: bowenlan-amzn <bowenlan23@gmail.com>

* Fix a bug

Signed-off-by: bowenlan-amzn <bowenlan23@gmail.com>

* address comment

Signed-off-by: bowenlan-amzn <bowenlan23@gmail.com>

* prepareFromSegment now doesn't return Ranges

Signed-off-by: bowenlan-amzn <bowenlan23@gmail.com>

* how it looks like when introduce interfaces

Signed-off-by: bowenlan-amzn <bowenlan23@gmail.com>

* remove interface, clean up

Signed-off-by: bowenlan-amzn <bowenlan23@gmail.com>

* improve doc

Signed-off-by: bowenlan-amzn <bowenlan23@gmail.com>

* move multirangetraversal logic to helper

Signed-off-by: bowenlan-amzn <bowenlan23@gmail.com>

* improve the refactor

package name -> filterrewrite
move tree traversal logic to new class
add documentation for important abstract methods
add sub class for composite aggregation bridge

Signed-off-by: bowenlan-amzn <bowenlan23@gmail.com>

* Address Marc's comments

Signed-off-by: bowenlan-amzn <bowenlan23@gmail.com>

* Address concurrent segment search concern

To save the ranges per segment, now change to a map that save ranges for segments separately.

The increment document function "incrementBucketDocCount" should already be thread safe, as it's the same method used by normal aggregation execution path

Signed-off-by: bowenlan-amzn <bowenlan23@gmail.com>

* remove circular dependency

Signed-off-by: bowenlan-amzn <bowenlan23@gmail.com>

* Address comment

- remove map of segment ranges, pass in by calling getRanges when needed
- use AtomicInteger for the debug info

Signed-off-by: bowenlan-amzn <bowenlan23@gmail.com>

---------

Signed-off-by: bowenlan-amzn <bowenlan23@gmail.com>
(cherry picked from commit 170ea27)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Copy link
Contributor

github-actions bot commented Aug 9, 2024

✅ Gradle check result for 6c301f2: SUCCESS

Copy link

codecov bot commented Aug 9, 2024

Codecov Report

Attention: Patch coverage is 85.50369% with 59 lines in your changes missing coverage. Please review.

Project coverage is 71.59%. Comparing base (6fbd079) to head (6c301f2).
Report is 20 commits behind head on 2.x.

Files Patch % Lines
...ilterrewrite/FilterRewriteOptimizationContext.java 82.60% 9 Missing and 3 partials ⚠️
...arch/aggregations/bucket/filterrewrite/Helper.java 85.00% 4 Missing and 8 partials ⚠️
...tions/bucket/filterrewrite/PointTreeTraversal.java 88.15% 5 Missing and 4 partials ⚠️
...t/filterrewrite/DateHistogramAggregatorBridge.java 87.03% 1 Missing and 6 partials ⚠️
...ns/bucket/filterrewrite/RangeAggregatorBridge.java 82.85% 1 Missing and 5 partials ⚠️
...arch/aggregations/bucket/filterrewrite/Ranges.java 75.00% 2 Missing and 3 partials ⚠️
...egations/bucket/composite/CompositeAggregator.java 86.95% 2 Missing and 1 partial ⚠️
...ucket/filterrewrite/CompositeAggregatorBridge.java 77.77% 0 Missing and 2 partials ⚠️
.../bucket/histogram/AutoDateHistogramAggregator.java 94.73% 1 Missing ⚠️
...ions/bucket/histogram/DateHistogramAggregator.java 90.90% 1 Missing ⚠️
... and 1 more
Additional details and impacted files
@@             Coverage Diff              @@
##                2.x   #15179      +/-   ##
============================================
- Coverage     71.64%   71.59%   -0.05%     
- Complexity    62892    62956      +64     
============================================
  Files          5137     5144       +7     
  Lines        295074   295053      -21     
  Branches      42979    42960      -19     
============================================
- Hits         211399   211244     -155     
- Misses        66004    66156     +152     
+ Partials      17671    17653      -18     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sandeshkr419
Copy link
Contributor

@mch2 We should merge this as well since only changelog failed as the origin PR did not have a changelog either.

@mch2 mch2 merged commit 1f36470 into 2.x Aug 19, 2024
58 of 59 checks passed
@mch2 mch2 deleted the backport/backport-14464-to-2.x branch August 19, 2024 16:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants