-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Support/fix for star-tree keyword aggregation dfs mode #18354
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
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #18354 +/- ##
============================================
+ Coverage 72.53% 72.58% +0.05%
- Complexity 67396 67439 +43
============================================
Files 5492 5492
Lines 311127 311128 +1
Branches 45224 45223 -1
============================================
+ Hits 225680 225846 +166
+ Misses 67063 66868 -195
- Partials 18384 18414 +30 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
@@ -109,15 +109,6 @@ public class GlobalOrdinalsStringTermsAggregator extends AbstractStringTermsAggr | |||
protected int segmentsWithMultiValuedOrds = 0; | |||
LongUnaryOperator globalOperator; | |||
|
|||
/** |
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.
This was some dead code which I removed. Not related to the PR.
Signed-off-by: Sandesh Kumar <[email protected]>
❌ Gradle check result for cd9c5be: 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? |
Description
TLDR: DFS flow in keyword terms was returing empty buckets, this PR fixes the flow for DFS case.
Detailed Explanation of Changes:
The existing strategy where we fetch which bucketOrd to update:
For DenseGlobalOrds (used by breadth-first flow) implementation: would return the bucket in for correctly
For RemapGlobalOrds (used by depth-first flow) implementation: was returning
-1
when it was trying to find a bucket, and hence bucket was not getting initialized properly with correct values.We needed a new utility to find or create bucket if not available so I have introduced a new utility to resolve the same:
This change ensures that both the cases. bfs/dfs are handled correctly.
Added test cases to handle the same.
Related Issues
Resolves #[Issue number to be closed when this PR is merged]
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.