Fix failure in nested groupBy with multiple aggregators with same fie… #1781
Conversation
| for (final String fieldName : outerAggregator.requiredFields()) { | ||
| if (!innerFieldNames.contains(fieldName)) { | ||
| throw new IllegalArgumentException( | ||
| String.format("Subquery must have an aggregator or post aggregator with name '%s'", fieldName) |
There was a problem hiding this comment.
The subquery needs to have an aggregator with a name that corresponds to the fieldName in the outer query's aggregator function, otherwise when the outer query runs it'll fail.
There was a problem hiding this comment.
what happens when the outer query needs to access a column that's present in the data and has nothing to do with the sub query?
There was a problem hiding this comment.
As discussed, the outer query doesn't have visibility into the data, only the results returned from the sub query which have been materialized in the intermediate incremental index.
There was a problem hiding this comment.
why would it? once the inner query is done, it IS the data
There was a problem hiding this comment.
We should make this error message more helpful and explain why the subquery must have the field, e.g. say it is required because it is referenced by the outer query.
|
👍 |
…ldName Version 2 - Throws an exception if an outer query references an aggregator that doesn't exist in the inner query, and then uses the inner query aggregator names to form the columns for the intermediate incremental index. Also deleted all the getRequiredColumns() methods which are no longer being used. We do something wacky by adding an aggregator factory for the post aggregators when building the intermediate incremental index, otherwise queries on post aggregate results fail because the data isn't in the incremental index. Closes #1419
… test for non-integers
| @@ -51,6 +51,7 @@ | |||
| import io.druid.query.aggregation.JavaScriptAggregatorFactory; | |||
| import io.druid.query.aggregation.LongSumAggregatorFactory; | |||
| import io.druid.query.aggregation.PostAggregator; | |||
| import io.druid.query.aggregation.cardinality.CardinalityAggregatorFactory; | |||
There was a problem hiding this comment.
I think this import is unused?
|
👍 looks good to me, would appreciate a second set of eyes |
…egator-fix-v2 Fix failure in nested groupBy with multiple aggregators with same fie…
| for (AggregatorFactory outerAggregator : query.getAggregatorSpecs()) { | ||
| for (final String fieldName : outerAggregator.requiredFields()) { | ||
| if (!innerFieldNames.contains(fieldName)) { | ||
| throw new IllegalArgumentException( |
|
reverted in e3bb93e |
…ldName
Version 2 - Throws an exception if an outer query references an
aggregator that doesn't exist in the inner query, and then uses the
inner query aggregator names to form the columns for the intermediate
incremental index.
Also deleted all the getRequiredColumns() methods which are no longer
being used.
We do something wacky by adding an aggregator factory for the post
aggregators when building the intermediate incremental index, otherwise
queries on post aggregate results fail because the data isn't in the
incremental index.
Closes #1419