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 #64214

Merged
merged 1 commit into from
Oct 27, 2020

Commits on Oct 27, 2020

  1. Make sure non-collecting aggs include sub-aggs

    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
    nik9000 committed Oct 27, 2020
    Configuration menu
    Copy the full SHA
    4bee8fe View commit details
    Browse the repository at this point in the history