[Star Tree] [Search] Keyword & Numeric Terms Aggregation #17165
Conversation
sandeshkr419
requested review from
anasalkouz,
andrross,
ashking94,
Bukhtawar,
CEHENKLE,
cwperks,
dblock,
dbwiddis,
gbbafna,
jed326,
kotwanikunal,
mch2,
msfroh,
nknize,
owaiskazi19,
reta,
Rishikesh1159,
sachinpkale,
saratvemulapalli,
shwetathareja,
sohami,
VachaShah,
jainankitk and
linuxpi
as code owners
github-actions
bot
added
enhancement
sandeshkr419
added
backport 2.x
|
❌ Gradle check result for 89310be: 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? |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #17165 +/- ##
============================================
+ Coverage 72.29% 72.43% +0.13%
- Complexity 65699 65783 +84
============================================
Files 5318 5318
Lines 305676 305786 +110
Branches 44350 44374 +24
============================================
+ Hits 220992 221494 +502
+ Misses 66582 66125 -457
- Partials 18102 18167 +65 ☔ View full report in Codecov by Sentry. |
|
❕ Gradle check result for 89310be: UNSTABLE Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure. |
|
@msfroh Rebased against your aggregation restructuring changes and incorporated the new interfaces in this PR as well. :D |
|
❌ Gradle check result for 06a2ba3: 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: Sandesh Kumar <[email protected]>
Signed-off-by: Sandesh Kumar <[email protected]>
Signed-off-by: Sandesh Kumar <[email protected]>
sandeshkr419
force-pushed
the
k1
branch
2 times, most recently
from
1674736 to
1b0a6b9
Compare
Signed-off-by: Sandesh Kumar <[email protected]>
|
❕ Gradle check result for e1bebaa: UNSTABLE Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure. |
|
@msfroh @bharath-techie Rebased from main again since #17239 is merged. |
| @@ -228,6 +243,10 @@ protected boolean tryPrecomputeAggregationForLeaf(LeafReaderContext ctx) throws | |||
| (ord, docCount) -> incrementBucketDocCount(collectionStrategy.globalOrdToBucketOrd(0, ord), docCount) | |||
| ); | |||
| } | |||
| CompositeIndexFieldInfo supportedStarTree = StarTreeQueryHelper.getSupportedStarTree(this.context.getQueryShardContext()); | |||
| if (supportedStarTree != null) { | |||
| return preComputeWithStarTree(ctx, supportedStarTree); | |||
There was a problem hiding this comment.
precomputeWithStarTree always returns true.
|
|
||
| String metricName = StarTreeUtils.fullyQualifiedFieldNameForStarTreeMetricsDocValues( | ||
| starTree.getField(), | ||
| "_doc_count", |
There was a problem hiding this comment.
Should this be hard-coded? Should it be DocCountFieldMapper.NAME?
| public void setSubCollectors() throws IOException { | ||
| for (Aggregator aggregator : subAggregators) { | ||
| this.subCollectors.add(((StarTreePreComputeCollector) aggregator).getStarTreeBucketCollector(ctx, starTree, this)); | ||
| } | ||
| } |
There was a problem hiding this comment.
Can you move this below the fields defined below? Fields within an anonymous class are kind of confusing, but when they are interspersed with methods, it hurts my brain.
Actually, if you move the fields out of the anonymous class declaration and into the getStarTreeBucketCollector method body, they'll get captured by the anonymous StarTreeBucketCollector anyway (and IMO, the code would be easier to read). Maybe give that a shot and see how it looks to you.
| }; | ||
| } | ||
|
|
||
| private boolean preComputeWithStarTree(LeafReaderContext ctx, CompositeIndexFieldInfo supportedStarTree) throws IOException { |
There was a problem hiding this comment.
Should probably return void, since we've already decided that we will precompute.
| ) throws IOException { | ||
| assert parent == null; | ||
| StarTreeValues starTreeValues = StarTreeQueryHelper.getStarTreeValues(ctx, starTree); | ||
| return new StarTreeBucketCollector( |
There was a problem hiding this comment.
This is pretty similar to the one in GlobalOrdinalsStringTermsAggregator. Is there opportunity to pull the common logic into an abstract base class?
| // don't defer when StarTreeContext is set, don't defer when collectMode == SubAggCollectionMode.BREADTH_FIRST | ||
| // this boolean condition can be further simplified but affects readability. | ||
| return (context.getQueryShardContext().getStarTreeQueryContext() == null || collectMode != SubAggCollectionMode.BREADTH_FIRST) | ||
| && collectMode == SubAggCollectionMode.BREADTH_FIRST | ||
| && !aggsUsedForSorting.contains(aggregator); |
There was a problem hiding this comment.
Could this be:
if (context.getQueryShardContext().getStarTreeQueryContext() == null) {
return false;
} else {
return collectMode == SubAggCollectionMode.BREADTH_FIRST && !aggsUsedForSorting.contains(aggregator);
}
That is, make it even more complicated for readability. 😁
Description
Fix for timestamp field to be fetched from request/valueSource instead of hard-coded value(this change is already merged Fix date hardcoding in date aggregator #17239)Related Issues
Resolves #16551
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.