clintropolis commented on a change in pull request #12320:
URL: https://github.com/apache/druid/pull/12320#discussion_r826318278
##########
File path:
processing/src/test/java/org/apache/druid/query/groupby/GroupByQueryRunnerTest.java
##########
@@ -5843,6 +5845,29 @@ public void testDifferentIntervalSubquery()
TestHelper.assertExpectedObjects(expectedResults, results,
"subquery-different-intervals");
}
+ @Test
+ public void testDoubleMeanQuery()
+ {
+ GroupByQuery query = new GroupByQuery.Builder()
+ .setDataSource(QueryRunnerTestHelper.DATA_SOURCE)
+ .setGranularity(Granularities.ALL)
+ .setQuerySegmentSpec(QueryRunnerTestHelper.FIRST_TO_THIRD)
+ .setAggregatorSpecs(
+ new DoubleMeanAggregatorFactory("meanOnDouble", "index")
Review comment:
I think if using column `doubleNumericNull` instead of `index` you would
get better coverage on the null values
##########
File path:
processing/src/main/java/org/apache/druid/query/aggregation/mean/DoubleMeanVectorAggregator.java
##########
@@ -45,11 +46,28 @@ public void init(final ByteBuffer buf, final int position)
public void aggregate(final ByteBuffer buf, final int position, final int
startRow, final int endRow)
{
final double[] vector = selector.getDoubleVector();
- for (int i = startRow; i < endRow; i++) {
- DoubleMeanHolder.update(buf, position, vector[i]);
+ final boolean[] nulls = selector.getNullVector();
+
+ if (nulls != null) {
+ if (NullHandling.replaceWithDefault()) {
Review comment:
i don't think that `getNullVector` will return a non-null value when
this is true, but maybe worth double checking, and if you can't confirm then is
safer to leave like this
##########
File path:
processing/src/main/java/org/apache/druid/query/aggregation/mean/DoubleMeanBufferAggregator.java
##########
@@ -51,6 +52,9 @@ public void aggregate(ByteBuffer buf, int position)
{
Object update = selector.getObject();
+ if (update == null && NullHandling.replaceWithDefault() == false) {
Review comment:
yeah, this agg is a bit strange in that it supports both numeric
primitives and the complex merging; isNull only works for numeric selectors,
other selector types need to check that the value itself is null.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
