Skip to content

Commit

Permalink
address comments
Browse files Browse the repository at this point in the history 
Signed-off-by: Peng Huo <penghuo@gmail.com>
  • Loading branch information
@penghuo
penghuo committed Apr 20, 2022
1 parent a35ef76 commit 8003493
Show file tree
Hide file tree
Showing 5 changed files with 99 additions and 44 deletions.
84 changes: 74 additions & 10 deletions rest-api-spec/src/main/resources/rest-api-spec/test/search.aggregation/370_multi_terms.yml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,14 +23,30 @@ setup:
date:
type: date

- do:
indices.create:
index: test_2
body:
settings:
number_of_shards: 2
number_of_replicas: 0
mappings:
properties:
str:
type: keyword
integer:
type: long
boolean:
type: boolean

- do:
cluster.health:
wait_for_status: green

---
"Basic test":
- skip:
version: "- 3.0.0"
version: "- 2.9.99"
reason: multi_terms aggregation is introduced in 3.0.0

- do:
Expand Down Expand Up @@ -80,7 +96,7 @@ setup:
---
"IP test":
- skip:
version: "- 3.0.0"
version: "- 2.9.99"
reason: multi_terms aggregation is introduced in 3.0.0

- do:
Expand Down Expand Up @@ -130,7 +146,7 @@ setup:
---
"Boolean test":
- skip:
version: "- 3.0.0"
version: "- 2.9.99"
reason: multi_terms aggregation is introduced in 3.0.0

- do:
Expand Down Expand Up @@ -180,7 +196,7 @@ setup:
---
"Double test":
- skip:
version: "- 3.0.0"
version: "- 2.9.99"
reason: multi_terms aggregation is introduced in 3.0.0

- do:
Expand Down Expand Up @@ -223,7 +239,7 @@ setup:
---
"Date test":
- skip:
version: "- 3.0.0"
version: "- 2.9.99"
reason: multi_terms aggregation is introduced in 3.0.0

- do:
Expand Down Expand Up @@ -266,7 +282,7 @@ setup:
---
"Unmapped keywords":
- skip:
version: "- 3.0.0"
version: "- 2.9.99"
reason: multi_terms aggregation is introduced in 3.0.0

- do:
Expand Down Expand Up @@ -306,7 +322,7 @@ setup:
---
"Null value":
- skip:
version: "- 3.0.0"
version: "- 2.9.99"
reason: multi_terms aggregation is introduced in 3.0.0

- do:
Expand Down Expand Up @@ -341,7 +357,7 @@ setup:
---
"multiple multi_terms bucket":
- skip:
version: "- 3.0.0"
version: "- 2.9.99"
reason: multi_terms aggregation is introduced in 3.0.0

- do:
Expand Down Expand Up @@ -441,7 +457,7 @@ setup:
---
"top 1 ordered by metrics ":
- skip:
version: "- 3.0.0"
version: "- 2.9.99"
reason: multi_terms aggregation is introduced in 3.0.0

- do:
Expand Down Expand Up @@ -486,7 +502,7 @@ setup:
---
"min_doc_count":
- skip:
version: "- 3.0.0"
version: "- 2.9.99"
reason: multi_terms aggregation is introduced in 3.0.0

- do:
Expand Down Expand Up @@ -554,3 +570,51 @@ setup:
- match: { aggregations.m_terms.buckets.2.key: ["c", 1] }
- match: { aggregations.m_terms.buckets.2.key_as_string: "c|1" }
- match: { aggregations.m_terms.buckets.2.doc_count: 0 }

---
"sum_other_doc_count":
- skip:
version: "- 2.9.99"
reason: multi_terms aggregation is introduced in 3.0.0

- do:
bulk:
index: test_2
refresh: true
body:
- '{"index": {"routing": "s1"}}'
- '{"str": "a", "integer": 1}'
- '{"index": {"routing": "s1"}}'
- '{"str": "a", "integer": 1}'
- '{"index": {"routing": "s1"}}'
- '{"str": "a", "integer": 1}'
- '{"index": {"routing": "s1"}}'
- '{"str": "a", "integer": 1}'
- '{"index": {"routing": "s2"}}'
- '{"str": "b", "integer": 1}'
- '{"index": {"routing": "s2"}}'
- '{"str": "b", "integer": 1}'
- '{"index": {"routing": "s2"}}'
- '{"str": "b", "integer": 1}'
- '{"index": {"routing": "s2"}}'
- '{"str": "a", "integer": 1}'

- do:
search:
index: test_2
size: 0
body:
aggs:
m_terms:
multi_terms:
size: 1
shard_size: 1
terms:
- field: str
- field: integer

- length: { aggregations.m_terms.buckets: 1 }
- match: { aggregations.m_terms.sum_other_doc_count: 4 }
- match: { aggregations.m_terms.buckets.0.key: ["a", 1] }
- match: { aggregations.m_terms.buckets.0.key_as_string: "a|1" }
- match: { aggregations.m_terms.buckets.0.doc_count: 4 }
4 changes: 2 additions & 2 deletions server/src/main/java/org/opensearch/search/aggregations/bucket/terms/InternalMultiTerms.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,8 +56,8 @@ public static class Bucket extends InternalTerms.AbstractInternalBucket implemen
/**
* Create default {@link Bucket}.
*/
public static Bucket EMPTY(List<DocValueFormat> formats) {
return new Bucket(null, 0, null, false, 0, formats);
public static Bucket EMPTY(boolean showTermDocCountError, List<DocValueFormat> formats) {
return new Bucket(null, 0, null, showTermDocCountError, 0, formats);
}

public Bucket(
Expand Down
43 changes: 15 additions & 28 deletions ...in/java/org/opensearch/search/aggregations/bucket/terms/MultiTermsAggregationFactory.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,38 +60,25 @@ public static void registerAggregators(ValuesSourceRegistry.Builder builder) {
true
);

builder.register(REGISTRY_KEY, org.opensearch.common.collect.List.of(CoreValuesSourceType.NUMERIC), config -> {
ValuesSourceConfig valuesSourceConfig = config.v1();
IncludeExclude includeExclude = config.v2();
ValuesSource.Numeric valuesSource = ((ValuesSource.Numeric) valuesSourceConfig.getValuesSource());
IncludeExclude.LongFilter longFilter = null;
if (valuesSource.isFloatingPoint()) {
if (includeExclude != null) {
longFilter = includeExclude.convertToDoubleFilter();
}
return MultiTermsAggregator.InternalValuesSourceFactory.doubleValueSource(valuesSource, longFilter);
} else {
if (includeExclude != null) {
longFilter = includeExclude.convertToLongFilter(valuesSourceConfig.format());
}
return MultiTermsAggregator.InternalValuesSourceFactory.longValuesSource(valuesSource, longFilter);
}
}, true);

builder.register(
REGISTRY_KEY,
org.opensearch.common.collect.List.of(CoreValuesSourceType.BOOLEAN, CoreValuesSourceType.DATE),
builder.register(REGISTRY_KEY,
org.opensearch.common.collect.List.of(CoreValuesSourceType.NUMERIC, CoreValuesSourceType.BOOLEAN, CoreValuesSourceType.DATE),
config -> {
ValuesSourceConfig valuesSourceConfig = config.v1();
IncludeExclude includeExclude = config.v2();
ValuesSource.Numeric valuesSource = ((ValuesSource.Numeric) valuesSourceConfig.getValuesSource());
IncludeExclude.LongFilter longFilter = includeExclude == null
? null
: includeExclude.convertToLongFilter(valuesSourceConfig.format());
return MultiTermsAggregator.InternalValuesSourceFactory.longValuesSource(valuesSource, longFilter);
},
true
);
IncludeExclude.LongFilter longFilter = null;
if (valuesSource.isFloatingPoint()) {
if (includeExclude != null) {
longFilter = includeExclude.convertToDoubleFilter();
}
return MultiTermsAggregator.InternalValuesSourceFactory.doubleValueSource(valuesSource, longFilter);
} else {
if (includeExclude != null) {
longFilter = includeExclude.convertToLongFilter(valuesSourceConfig.format());
}
return MultiTermsAggregator.InternalValuesSourceFactory.longValuesSource(valuesSource, longFilter);
}
}, true);

builder.registerUsage(MultiTermsAggregationBuilder.NAME);
}
Expand Down
3 changes: 2 additions & 1 deletion ...r/src/main/java/org/opensearch/search/aggregations/bucket/terms/MultiTermsAggregator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,8 @@ public InternalAggregation[] buildAggregations(long[] owningBucketOrds) throws I
InternalMultiTerms.Bucket spare = null;
BytesRef dest = null;
BytesKeyedBucketOrds.BucketOrdsEnum ordsEnum = bucketOrds.ordsEnum(owningBucketOrds[ordIdx]);
CheckedSupplier<InternalMultiTerms.Bucket, IOException> emptyBucketBuilder = () -> InternalMultiTerms.Bucket.EMPTY(formats);
CheckedSupplier<InternalMultiTerms.Bucket, IOException> emptyBucketBuilder =
() -> InternalMultiTerms.Bucket.EMPTY(showTermDocCountError, formats);
while (ordsEnum.next()) {
long docCount = bucketDocCount(ordsEnum.ord());
otherDocCounts[ordIdx] += docCount;
Expand Down
9 changes: 6 additions & 3 deletions server/src/main/java/org/opensearch/search/aggregations/bucket/terms/ParsedTerms.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -139,13 +139,16 @@ static <B extends ParsedBucket> B parseTermsBucketXContent(
XContentParser.Token token;
String currentFieldName = parser.currentName();
while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) {
// field value could be list, e.g. multi_terms aggregation.
if ((token.isValue() || token == XContentParser.Token.START_ARRAY)
&& CommonFields.KEY.getPreferredName().equals(currentFieldName)) {
keyConsumer.accept(parser, bucket);
}
if (token == XContentParser.Token.FIELD_NAME) {
currentFieldName = parser.currentName();
} else if (token.isValue() || token == XContentParser.Token.START_ARRAY) {
} else if (token.isValue()) {
if (CommonFields.KEY_AS_STRING.getPreferredName().equals(currentFieldName)) {
bucket.setKeyAsString(parser.text());
} else if (CommonFields.KEY.getPreferredName().equals(currentFieldName)) {
keyConsumer.accept(parser, bucket);
} else if (CommonFields.DOC_COUNT.getPreferredName().equals(currentFieldName)) {
bucket.setDocCount(parser.longValue());
} else if (DOC_COUNT_ERROR_UPPER_BOUND_FIELD_NAME.getPreferredName().equals(currentFieldName)) {
Expand Down

0 comments on commit 8003493

Please sign in to comment.