Support multiple outer aggregators of same type and provide more help… #1799
Conversation
…ful exception when the same inner aggregator is referenced by multiple types of outer aggregators
| for (AggregatorFactory aggregatorFactory : query.getAggregatorSpecs()) { | ||
| aggs.addAll(aggregatorFactory.getRequiredColumns()); |
There was a problem hiding this comment.
wouldn't it solve the problem if we did..
for (AggregatorFactory aggregatorFactory : subquery.getAggregatorSpeces()) {
aggs.add(aggregatorFactory.getCombiningFactory());
}
There was a problem hiding this comment.
also, probably set innerQuery.setPostAggregatorSpecs(subquery.getPostAggregatorSpecs())
There was a problem hiding this comment.
@himanshug Using the combining factories from the subquery works for the aggregators, but setting innerQuery.setPostAggregatorSpecs(subquery.getPostAggregatorSpecs()) doesn't do anything since that field isn't read when we create the intermediate incremental index from the subquery results. The result is that any aggregator in the outer query that is operating on a subquery post aggregator doesn't work correctly.
|
LGTM |
| for (AggregatorFactory aggregatorFactory : query.getAggregatorSpecs()) { | ||
| aggs.addAll(aggregatorFactory.getRequiredColumns()); | ||
| for (final AggregatorFactory transferAgg : aggregatorFactory.getRequiredColumns()) { |
There was a problem hiding this comment.
can we add some comments to explain the logic here? Outside of the context of this PR it might be hard to understand what this is doing.
There was a problem hiding this comment.
@xvrl I added a comment, hopefully it clears things up. Thanks for the feedback X.
|
👍 |
Support multiple outer aggregators of same type and provide more help…
…ful exception when the same inner aggregator is referenced by multiple types of outer aggregators
Temporary bandage for issue with nested group by where an inner aggregator is referenced by more than one outer aggregator. This fix allows multiple references if the outer aggregators are the same (based on 'equals' method), otherwise it throws an exception with a more helpful message than previously. To fully support this would require a larger re-work of the implementation.