Aggregation refactor: make aggregationFactory implement NamedWritable#12830
Aggregation refactor: make aggregationFactory implement NamedWritable#12830colings86 merged 1 commit intoelastic:feature/aggs-refactoringfrom colings86:enhancement/AggFactoryRefactor
Conversation
There was a problem hiding this comment.
a bit weird to create an array list with a known size, just to convert it to an array. Let's build an array directly?
|
I gave it a quick look and I think the fact that most aggregation objects track their parent is going to make the refactoring a bit less straightforward than for queries (see comments about serialization and checking for parents in equals) |
|
@jpountz I've pushed a commit addressing your comments. If you could especially check the equals and hashCode methods in AggregatoryFactory, ValuesSourceAggregatorFactory, PipelineAggregatorFactory and AggregatorFactories, that would be great |
There was a problem hiding this comment.
I wonder if we should start already sharing some common code between our BaseQueryTestCase and this class....wouldn't want to complicate things though. Also our base test class in not in master of course so that woul already complicate things...
There was a problem hiding this comment.
One more thing, maybe you want to have a testEqualsAndHashcode like we recently introduced in the query-refactoring branch, just to make sure that equals and hashcode are properly implemented.
|
I had a look and this looks good to me, I think the main concern around equality checks has been addressed, all good besides that |
There was a problem hiding this comment.
duplicated empty javadocs? :)
|
LGTM |
7cbf5e5 to
cffd2d2
Compare
Also makes AggregatorFactories implement Writable
Also makes AggregatorFactories implement Writable