Skip to content
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

Make sure non-collecting aggs include sub-aggs (backport of #64214) #64244

Merged
merged 5 commits into from
Oct 29, 2020

Conversation

nik9000
Copy link
Member

@nik9000 nik9000 commented Oct 27, 2020

Now that we're consistently using cat_match to filter which shards we
run on we can get this confusing case:

  1. You have a search with, say, a range and a sub-agg.
  2. That search has a query that can_match can recognize will match no
    docs. On any shard.
  3. So we dutifully run it on a single shard so it can produce the
    "empty" aggs.
  4. The shard we pick happens to not have the target of the range mapped.
  5. This kicks in the special range aggregator that doesn't collect any
    documents.
  6. Before this commit, that range aggregator also never produced any
    sub-aggs.

So, without this change, it was quite possible for a search that
happened to match no documents to "throw away" the sub-aggs of a range
and a few other aggs.

We've had this problem for a long, long time but it is more confusing
now because can_match is really kicking in and causing us to see cases
where it looks like you are targeting a lot of shards but you really are
only targeting a couple. It used to be that to get the "no sub-aggs"
behavior you had to explicitly target only shards that didn't map the
target field of the range agg. And, like, in that case it isn't too
bad because you targeted a sort of degenerate shard. But now that
can_match is doing its thing you can end up with the confusing steps
above. It took me several hours to track down what what happening I know
how the individual pieces of all of this works. It took four hours to
figure out how they fit together in this case....

Anyway! This replaces all the aggregator implementations that throw out
the sub-aggregators with ones that keep them. I think this'll be less
confusing in the future.

Closes #64142

…4214)

Now that we're consistently using `cat_match` to filter which shards we
run on we can get this confusing case:
1. You have a search with, say, a range and a sub-agg.
2. That search has a query that `can_match` can recognize will match no
   docs. On *any* shard.
3. So we dutifully run it on a single shard so it can produce the
   "empty" aggs.
4. The shard we pick happens to not have the target of the range mapped.
5. This kicks in the special range aggregator that doesn't collect any
   documents.
6. Before this commit, that range aggregator *also* never produced any
   sub-aggs.

So, without this change, it was quite possible for a search that
happened to match no documents to "throw away" the sub-aggs of a range
and a few other aggs.

We've had this problem for a long, long time but it is more confusing
now because `can_match` is really kicking in and causing us to see cases
where it looks like you are targeting a lot of shards but you really are
only targeting a couple. It used to be that to get the "no sub-aggs"
behavior you had to explicitly target only shards that didn't map the
target field of the `range` agg. And, like, in that case it isn't too
bad because you targeted a sort of degenerate shard. But now that
`can_match` is doing its thing you can end up with the confusing steps
above. It took me several hours to track down what what happening I know
how the individual pieces of all of this works. It took four hours to
figure out how they fit together in this case....

Anyway! This replaces all the aggregator implementations that throw out
the sub-aggregators with ones that keep them. I think this'll be less
confusing in the future.

Closes elastic#64142
Backport is coming. Please wait.
@nik9000
Copy link
Member Author

nik9000 commented Oct 28, 2020

run elasticsearch-ci/default-distro

@nik9000
Copy link
Member Author

nik9000 commented Oct 28, 2020

@elasticmachine, update branch

@nik9000
Copy link
Member Author

nik9000 commented Oct 28, 2020

@elasticmachine update branch

@nik9000
Copy link
Member Author

nik9000 commented Oct 28, 2020

run elasticsearch-ci/2

@nik9000
Copy link
Member Author

nik9000 commented Oct 28, 2020

@elasticmachine update branch

@nik9000
Copy link
Member Author

nik9000 commented Oct 29, 2020

run elasticsearch-ci/packaging-sample-unix

@nik9000 nik9000 merged commit cc693e4 into elastic:7.x Oct 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants