Skip to content

Commit

Permalink
Change InternalSignificantTerms to only sum shard level counts in fin…
Browse files Browse the repository at this point in the history
…al reduce

Signed-off-by: Jay Deng <jayd0104@gmail.com>
  • Loading branch information
jed326 authored and Jay Deng committed Jul 18, 2023
1 parent 8bd6b7c commit 7566b29
Show file tree
Hide file tree
Showing 4 changed files with 27 additions and 3 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
- Migrate client transports to Apache HttpClient / Core 5.x ([#4459](https://github.com/opensearch-project/OpenSearch/pull/4459))
- Change http code on create index API with bad input raising NotXContentException from 500 to 400 ([#4773](https://github.com/opensearch-project/OpenSearch/pull/4773))
- Improve summary error message for invalid setting updates ([#4792](https://github.com/opensearch-project/OpenSearch/pull/4792))
- Change InternalSignificantTerms to sum shard-level superset counts only in final reduce ([#8735](https://github.com/opensearch-project/OpenSearch/pull/8735))

### Deprecated

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -918,7 +918,9 @@ public ReaderContext readerContext() {

@Override
public InternalAggregation.ReduceContext partial() {
return requestToAggReduceContextBuilder.apply(request.source()).forPartialReduction();
InternalAggregation.ReduceContext rc = requestToAggReduceContextBuilder.apply(request.source()).forPartialReduction();
rc.setSliceLevel(true);
return rc;

Check warning on line 923 in server/src/main/java/org/opensearch/search/DefaultSearchContext.java

View check run for this annotation

Codecov / codecov/patch

server/src/main/java/org/opensearch/search/DefaultSearchContext.java#L921-L923

Added lines #L921 - L923 were not covered by tests
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,8 @@ public static class ReduceContext {
private final ScriptService scriptService;
private final IntConsumer multiBucketConsumer;
private final PipelineTree pipelineTreeRoot;

private boolean isSliceLevel;
/**
* Supplies the pipelines when the result of the reduce is serialized
* to node versions that need pipeline aggregators to be serialized
Expand Down Expand Up @@ -138,6 +140,7 @@ private ReduceContext(
this.multiBucketConsumer = multiBucketConsumer;
this.pipelineTreeRoot = pipelineTreeRoot;
this.pipelineTreeForBwcSerialization = pipelineTreeForBwcSerialization;
this.isSliceLevel = false;
}

/**
Expand All @@ -149,6 +152,14 @@ public boolean isFinalReduce() {
return pipelineTreeRoot != null;
}

public void setSliceLevel(boolean b) {
this.isSliceLevel = b;
}

Check warning on line 157 in server/src/main/java/org/opensearch/search/aggregations/InternalAggregation.java

View check run for this annotation

Codecov / codecov/patch

server/src/main/java/org/opensearch/search/aggregations/InternalAggregation.java#L156-L157

Added lines #L156 - L157 were not covered by tests

public boolean getSliceLevel() {
return this.isSliceLevel;
}

public BigArrays bigArrays() {
return bigArrays;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -232,7 +232,12 @@ public InternalAggregation reduce(List<InternalAggregation> aggregations, Reduce
@SuppressWarnings("unchecked")
InternalSignificantTerms<A, B> terms = (InternalSignificantTerms<A, B>) aggregation;
globalSubsetSize += terms.getSubsetSize();
globalSupersetSize += terms.getSupersetSize();
// supersetSize is a shard level count, so we don't sum it for segment slices
if (!reduceContext.getSliceLevel()) {
globalSupersetSize += terms.getSupersetSize();
} else {
globalSupersetSize = terms.getSupersetSize();

Check warning on line 239 in server/src/main/java/org/opensearch/search/aggregations/bucket/terms/InternalSignificantTerms.java

View check run for this annotation

Codecov / codecov/patch

server/src/main/java/org/opensearch/search/aggregations/bucket/terms/InternalSignificantTerms.java#L239

Added line #L239 was not covered by tests
}
}
Map<String, List<B>> buckets = new HashMap<>();
for (InternalAggregation aggregation : aggregations) {
Expand Down Expand Up @@ -291,7 +296,12 @@ protected B reduceBucket(List<B> buckets, ReduceContext context) {
List<InternalAggregations> aggregationsList = new ArrayList<>(buckets.size());
for (B bucket : buckets) {
subsetDf += bucket.subsetDf;
supersetDf += bucket.supersetDf;
// supersetDf is a shard level count, so we don't sum it for segment slices
if (!context.getSliceLevel()) {
supersetDf += bucket.supersetDf;
} else {
supersetDf = bucket.supersetDf;

Check warning on line 303 in server/src/main/java/org/opensearch/search/aggregations/bucket/terms/InternalSignificantTerms.java

View check run for this annotation

Codecov / codecov/patch

server/src/main/java/org/opensearch/search/aggregations/bucket/terms/InternalSignificantTerms.java#L303

Added line #L303 was not covered by tests
}
aggregationsList.add(bucket.aggregations);
}
InternalAggregations aggs = InternalAggregations.reduce(aggregationsList, context);
Expand Down

0 comments on commit 7566b29

Please sign in to comment.