-
Notifications
You must be signed in to change notification settings - Fork 24.4k
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
Merge remaining sig_terms into terms #57397
Conversation
Merges the remaining implementation of `significant_terms` into `terms` so that we can more easilly make them work properly without `asMultiBucketAggregator` which *should* save memory and speed them up. Relates elastic#56487
@elasticsearchmachine run elasticsearch-ci/default-distro |
run elasticsearch-ci/1 |
1 similar comment
run elasticsearch-ci/1 |
Pinging @elastic/es-analytics-geo (:Analytics/Aggregations) |
@@ -648,7 +647,6 @@ void updateBucket(OrdBucket spare, long globalOrd, long bucketOrd, long docCount | |||
spare.globalOrd = globalOrd; | |||
spare.bucketOrd = bucketOrd; | |||
spare.docCount = docCount; | |||
otherDocCount += docCount; |
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 causing inaccurate doc counts when shard_min_doc_count
was more than 1.
@@ -164,17 +164,18 @@ public void collectDebugInfo(BiConsumer<String, Object> add) { | |||
BucketOrdsEnum ordsEnum = bucketOrds.ordsEnum(owningBucketOrds[ordIdx]); | |||
Supplier<B> emptyBucketBuilder = emptyBucketBuilder(owningBucketOrds[ordIdx]); | |||
while (ordsEnum.next()) { | |||
long docCount = bucketDocCount(ordsEnum.ord()); |
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.
I flip the if
statement here and moved the spare initialization down to right before I need it to line up with the way the map
version works.
/** | ||
* An aggregator of string values. | ||
*/ | ||
public class StringTermsAggregator extends AbstractStringTermsAggregator { |
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.
Really just renamed this class to MapStringTermsAggregator
but a lot changed.
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.
Looks good to me. Much easier to see the relationship between these two aggs now.
Merges the remaining implementation of `significant_terms` into `terms` so that we can more easilly make them work properly without `asMultiBucketAggregator` which *should* save memory and speed them up. Relates elastic#56487
Merges the remaining implementation of
significant_terms
intoterms
so that we can more easilly make them work properly without
asMultiBucketAggregator
which should save memory and speed them up.Relates #56487