clintropolis opened a new pull request #11208: URL: https://github.com/apache/druid/pull/11208
### Description This PR fixes a regression I believed was caused by #10135, which refactored the `AVG` and `COUNT` `SqlAggregator` implementations to directly construct the `FilteredAggregatorFactory` instead of [using `Aggregation.create(...).filter(...)`](https://github.com/apache/druid/pull/10135/files#diff-8816fcddac6763b07323c31d729ba5c3991c1fb902b74999b3f3385e407431a5L120), which caused any virtual columns which were created by the filter to no longer be associated with the `Aggregation` and so [not retained when the final query was constructed](https://github.com/apache/druid/blob/master/sql/src/main/java/org/apache/druid/sql/calcite/rel/DruidQuery.java#L639), effectively making these aggregators process nil inputs instead of the correct values. While looking into this issue, I noticed that `FilteredAggregatorFactory.requiredFields` did not include the required columns of the `DimFilter`, but only of the delegate `AggregatorFactory`. This seemed a mistake to me so I have adjusted this so that it will now return the correct set of columns, and am using this, along with a new method on `VirtualColumnRegistry` which can filter a list of columns to only include columns which are virtual so that we can continue to populate the `Aggregation` virtual columns list. I think we can rework `Aggregation` to not require having this list of `VirtualColumn` at all and instead just check the registry against `requiredFields` when constructing the query, but it was a more disruptive change than I wanted to tie up in this fix, so I will save it for a follow-up. <hr> This PR has: - [x] been self-reviewed. - [x] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links. - [x] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader. - [x] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for [code coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met. - [x] been tested in a test Druid cluster. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org For additional commands, e-mail: commits-h...@druid.apache.org
