Fix equals and hashCode in AggregationBuilder to be order independent

[hendrikmuhs]

Fixes the equals and hash function to ignore the order of aggregations. Mainly for testing purposes.

Note:

The impact of this issue afaik only affects tests (-> non-issue), I am using aggregations and need this fix in order to get other tests green.

[elasticmachine]

Pinging @elastic/es-search-aggs

[cbuescher]

I don't think this will satisfy the "equals" contract: !(A, B).constainsAll(A, B, C) but (A, B, C).constainsAll(A, B). I think and additional size check will solve this though.

[jimczi]

I wonder if it wouldn't be simpler to use HashSets instead of Lists in the Builder. Even if the number of aggs should be low I don't like the fact that we call containsAll on a list on every equals call.

[hendrikmuhs]

Good catch @cbuescher! - stupid mistake, happens if you think 'it's simple' ;-)

@jimczi I was careful not to touch to the inner workings. I think the suggestion is good but - as I would replace the List with the Set - means changing an interface and therefore bigger changes all over the place. Nevertheless, I happily do it if everybody agrees.

[hendrikmuhs]

retest this please

[colings86]

Note that for pipeline aggregations the order matters so we need to be careful turning Sets to Lists and make sure it's kept as a list after the order of execution of the pipeline aggregators is resolved

[hendrikmuhs]

I created an alternative fix which uses Sets: #34005

As you can see this is a large re-factoring, but should work.

@colings86 Regarding order of pipeline aggs: as far as I see it, this change does not touch the ordering but runs before aggregations are resolved. The order get's defined during build() - I am not touching this, so I think this should be fine.

Nevertheless we need to decide for one way or the other.

[cbuescher]

I am using aggregations and need this fix in order to get other tests green.

Could you explain how the currenty "ordered" hashCode/equals affects the tests you are trying to get to pass? I'm a bit reluctant to complicate the existing hashCode/equals implementation if it is not motivated by requirements to the class itself. Maybe there are other ways to solve this that don't involve changing the interface or adding additional complexity to the two functions. Even it they currently seem only to be used by tests, these methods are an integral part of the classes behaviour and are likely to be used outside of tests at some point.

[hendrikmuhs]

@cbuescher

In a nutshell: I have configuration objects which hold aggregations (similar to what rollup does). This configuration is serializeable. The test failures happen when testing end2end serializing/de-serializing. I can of course workaround it in my tests, but it feels odd to me. However, I am on the same page with you regarding the interface changes, this is a high price to pay for this fix.

FWIW: AggregatorFactories.Builder implements Writeable, ToXContentObject and the corresponding read parts, so is serializable according to this. For me it is part of the contract that you can serialize/deserialize and end up with an equal object.

[hendrikmuhs]

Closing in favor of #34005