Skip to content

Commit

Permalink
[7.x] Revert "Remove aggregation's postCollect phase (elastic#68942)
Browse files Browse the repository at this point in the history 
This partially reverts elastic#64016 and and adds elastic#67839 and adds
additional tests that would have caught issues with the changes
in elastic#64016. It's mostly Nik's code, I am just cleaning things up
a bit.

Co-authored-by: Nik Everett nik9000@gmail.com
  • Loading branch information
@imotov
imotov committed Feb 16, 2021
1 parent e4bbce3 commit d662f36
Show file tree
Hide file tree
Showing 44 changed files with 703 additions and 59 deletions.
12 changes: 9 additions & 3 deletions docs/reference/search/profile.asciidoc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -799,7 +799,9 @@ This yields the following aggregation profile output:
"collect": 45786,
"collect_count": 4,
"build_leaf_collector": 18211,
"build_leaf_collector_count": 1
"build_leaf_collector_count": 1,
"post_collection": 929,
"post_collection_count": 1
},
"debug": {
"total_buckets": 1,
Expand All @@ -820,7 +822,9 @@ This yields the following aggregation profile output:
"collect": 69401,
"collect_count": 4,
"build_leaf_collector": 8150,
"build_leaf_collector_count": 1
"build_leaf_collector_count": 1,
"post_collection": 1584,
"post_collection_count": 1
},
"children": [
{
Expand All @@ -837,7 +841,9 @@ This yields the following aggregation profile output:
"collect": 61611,
"collect_count": 4,
"build_leaf_collector": 5564,
"build_leaf_collector_count": 1
"build_leaf_collector_count": 1,
"post_collection": 471,
"post_collection_count": 1
},
"debug": {
"total_buckets": 1,
Expand Down
14 changes: 10 additions & 4 deletions ...s/parent-join/src/main/java/org/elasticsearch/join/aggregations/ParentJoinAggregator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,12 @@ public void collect(int docId, long owningBucketOrd) throws IOException {
}

@Override
protected void prepareSubAggs(long[] bucketOrdsToCollect) throws IOException {
public void postCollection() throws IOException {
// Delaying until beforeBuildingBuckets
}

@Override
protected void prepareSubAggs(long[] ordsToCollect) throws IOException {
IndexReader indexReader = searcher().getIndexReader();
for (LeafReaderContext ctx : indexReader.leaves()) {
Scorer childDocsScorer = outFilter.scorer(ctx);
Expand Down Expand Up @@ -144,13 +149,14 @@ public int docID() {
* structure that maps a primitive long to a list of primitive
* longs.
*/
for (long o: bucketOrdsToCollect) {
if (collectionStrategy.exists(o, globalOrdinal)) {
collectBucket(sub, docId, o);
for (long owningBucketOrd: ordsToCollect) {
if (collectionStrategy.exists(owningBucketOrd, globalOrdinal)) {
collectBucket(sub, docId, owningBucketOrd);
}
}
}
}
super.postCollection(); // Run post collection after collecting the sub-aggs
}

@Override
Expand Down
238 changes: 238 additions & 0 deletions modules/parent-join/src/yamlRestTest/resources/rest-api-spec/test/50_order_by.yml
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,238 @@
---
"order by sub agg containing join":
- skip:
reason: "It was fixed it 7.11.2"
version: " - 7.11.2"
- do:
indices.create:
index: test_1
body:
settings:
number_of_shards: 1
number_of_replicas: 0
mappings:
properties:
name:
type: keyword
join:
type: join
relations:
animal: feature

- do:
bulk:
index: test_1
refresh: true
body: |
{ "index": {"_id": "sheep"} }
{ "name": "sheep", "join": {"name": "animal"} }
{ "index": {"_id": "cow"} }
{ "name": "cow", "join": {"name": "animal"} }
{ "index": {"_id": "pig"} }
{ "name": "pig", "join": {"name": "animal"} }
{ "index": {"routing": "sheep"} }
{ "join": {"name": "feature", "parent": "sheep"}, "tag": "danger", "number": 1 }
{ "index": {"routing": "sheep"} }
{ "join": {"name": "feature", "parent": "sheep"}, "tag": "fluffiness", "number": 10 }
{ "index": {"routing": "cow"} }
{ "join": {"name": "feature", "parent": "cow"}, "tag": "danger", "number": 3 }
{ "index": {"routing": "cow"} }
{ "join": {"name": "feature", "parent": "cow"}, "tag": "fluffiness", "number": 1 }
{ "index": {"routing": "pig"} }
{ "join": {"name": "feature", "parent": "pig"}, "tag": "danger", "number": 100 }
{ "index": {"routing": "pig"} }
{ "join": {"name": "feature", "parent": "pig"}, "tag": "fluffiness", "number": 1 }
- do:
search:
index: test_1
body:
size: 0
aggs:
name:
terms:
size: 1
shard_size: 1
field: name
order:
- "children>max_number": desc
aggs:
children:
children:
type: feature
aggs:
max_number:
max:
field: number
- length: { aggregations.name.buckets: 1 }
- match: { aggregations.name.buckets.0.key: pig }
- match: { aggregations.name.buckets.0.doc_count: 1 }
- match: { aggregations.name.buckets.0.children.max_number.value: 100.0 }

---
"order by sub agg containing join and nested":
- skip:
reason: "It was fixed it 7.11.2"
version: " - 7.11.2"
- do:
indices.create:
index: test_1
body:
settings:
number_of_shards: 1
number_of_replicas: 0
mappings:
properties:
name:
type: keyword
join:
type: join
relations:
animal: feature
nested:
type: nested
properties:
number:
type: long

- do:
bulk:
index: test_1
refresh: true
body: |
{ "index": {"_id": "sheep"} }
{ "name": "sheep", "join": {"name": "animal"} }
{ "index": {"_id": "cow"} }
{ "name": "cow", "join": {"name": "animal"} }
{ "index": {"_id": "pig"} }
{ "name": "pig", "join": {"name": "animal"} }
{ "index": {"routing": "sheep"} }
{ "join": {"name": "feature", "parent": "sheep"}, "tag": "danger", "nested": [{"number": 1}] }
{ "index": {"routing": "sheep"} }
{ "join": {"name": "feature", "parent": "sheep"}, "tag": "fluffiness", "nested": [{"number": 10}] }
{ "index": {"routing": "cow"} }
{ "join": {"name": "feature", "parent": "cow"}, "tag": "danger", "nested": [{"number": 3}] }
{ "index": {"routing": "cow"} }
{ "join": {"name": "feature", "parent": "cow"}, "tag": "fluffiness", "nested": [{"number": 1}] }
{ "index": {"routing": "pig"} }
{ "join": {"name": "feature", "parent": "pig"}, "tag": "danger", "nested": [{"number": 100}, {"number": 1}] }
{ "index": {"routing": "pig"} }
{ "join": {"name": "feature", "parent": "pig"}, "tag": "fluffiness", "nested": [{"number": 1}] }
- do:
search:
index: test_1
body:
size: 0
aggs:
name:
terms:
size: 1
shard_size: 1
field: name
order:
- "children>nested>max_number": desc
aggs:
children:
children:
type: feature
aggs:
nested:
nested:
path: nested
aggs:
max_number:
max:
field: nested.number
- length: { aggregations.name.buckets: 1 }
- match: { aggregations.name.buckets.0.key: pig }
- match: { aggregations.name.buckets.0.doc_count: 1 }
- match: { aggregations.name.buckets.0.children.nested.max_number.value: 100.0 }

---
"order by sub agg containing join and nested and filter":
- skip:
reason: "It was fixed it 7.11.2"
version: " - 7.11.2"
- do:
indices.create:
index: test_1
body:
settings:
number_of_shards: 1
number_of_replicas: 0
mappings:
properties:
name:
type: keyword
join:
type: join
relations:
animal: feature
nested:
type: nested
properties:
code:
type:
keyword
number:
type: long

- do:
bulk:
index: test_1
refresh: true
body: |
{ "index": {"_id": "sheep"} }
{ "name": "sheep", "join": {"name": "animal"} }
{ "index": {"_id": "cow"} }
{ "name": "cow", "join": {"name": "animal"} }
{ "index": {"_id": "pig"} }
{ "name": "pig", "join": {"name": "animal"} }
{ "index": {"routing": "sheep"} }
{ "join": {"name": "feature", "parent": "sheep"}, "tag": "danger", "nested": [{"code": "mighty", "number": 1}, {"code": "meek", "number": 100}] }
{ "index": {"routing": "sheep"} }
{ "join": {"name": "feature", "parent": "sheep"}, "tag": "fluffiness", "nested": [{"code": "mighty", "number": 10}, {"code": "meek", "number": 10}] }
{ "index": {"routing": "cow"} }
{ "join": {"name": "feature", "parent": "cow"}, "tag": "danger", "nested": [{"code": "mighty", "number": 3}, {"code": "meek", "number": 3}] }
{ "index": {"routing": "cow"} }
{ "join": {"name": "feature", "parent": "cow"}, "tag": "fluffiness", "nested": [{"code": "mighty", "number": 1}, {"code": "meek", "number": 1}] }
{ "index": {"routing": "pig"} }
{ "join": {"name": "feature", "parent": "pig"}, "tag": "danger", "nested": [{"code": "mighty", "number": 100}, {"code": "meek", "number": 1}] }
{ "index": {"routing": "pig"} }
{ "join": {"name": "feature", "parent": "pig"}, "tag": "fluffiness", "nested": [{"code": "mighty", "number": 1}, {"code": "meek", "number": 1}] }
- do:
search:
index: test_1
body:
size: 0
aggs:
name:
terms:
size: 1
shard_size: 1
field: name
order:
- "children>nested>filter>max_number": desc
aggs:
children:
children:
type: feature
aggs:
nested:
nested:
path: nested
aggs:
filter:
filter:
match:
nested.code: mighty
aggs:
max_number:
max:
field: nested.number
- length: { aggregations.name.buckets: 1 }
- match: { aggregations.name.buckets.0.key: pig }
- match: { aggregations.name.buckets.0.doc_count: 1 }
- match: { aggregations.name.buckets.0.children.nested.filter.max_number.value: 100.0 }
3 changes: 3 additions & 0 deletions ...-spec/src/main/resources/rest-api-spec/test/search.aggregation/170_cardinality_metric.yml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -232,6 +232,7 @@ setup:
- gt: { profile.shards.0.aggregations.0.breakdown.build_leaf_collector: 0 }
- gt: { profile.shards.0.aggregations.0.breakdown.collect: 0 }
- gt: { profile.shards.0.aggregations.0.breakdown.build_aggregation: 0 }
- gt: { profile.shards.0.aggregations.0.breakdown.post_collection: 0 }
- match: { profile.shards.0.aggregations.0.debug.empty_collectors_used: 0 }
- gt: { profile.shards.0.aggregations.0.debug.numeric_collectors_used: 0 }
- match: { profile.shards.0.aggregations.0.debug.ordinals_collectors_used: 0 }
Expand All @@ -257,6 +258,7 @@ setup:
- gt: { profile.shards.0.aggregations.0.breakdown.build_leaf_collector: 0 }
- gt: { profile.shards.0.aggregations.0.breakdown.collect: 0 }
- gt: { profile.shards.0.aggregations.0.breakdown.build_aggregation: 0 }
- gt: { profile.shards.0.aggregations.0.breakdown.post_collection: 0 }
- match: { profile.shards.0.aggregations.0.debug.empty_collectors_used: 0 }
- gt: { profile.shards.0.aggregations.0.debug.numeric_collectors_used: 0 }
- match: { profile.shards.0.aggregations.0.debug.ordinals_collectors_used: 0 }
Expand All @@ -283,3 +285,4 @@ setup:
- gt: { profile.shards.0.aggregations.0.breakdown.build_leaf_collector: 0 }
- gt: { profile.shards.0.aggregations.0.breakdown.collect: 0 }
- gt: { profile.shards.0.aggregations.0.breakdown.build_aggregation: 0 }
- gt: { profile.shards.0.aggregations.0.breakdown.post_collection: 0 }
Loading

0 comments on commit d662f36

Please sign in to comment.