clintropolis commented on code in PR #18307:
URL: https://github.com/apache/druid/pull/18307#discussion_r2223987340
##########
processing/src/main/java/org/apache/druid/query/aggregation/LongSumAggregatorFactory.java:
##########
@@ -36,12 +36,28 @@ public class LongSumAggregatorFactory extends
SimpleLongAggregatorFactory
{
private final Supplier<byte[]> cacheKey;
- @JsonCreator
+ /**
+ * If true, the aggregator will return null when there are no values to
aggregate. This is the default behavior.
+ * Otherwise, the aggregator must return a default non-null value. This is
only used internally and not user-facing.
+ */
+ private final boolean isNullable;
Review Comment:
i think this should also be `forceNotNullable` like
`NullableNumericAggregatorFactory`
##########
processing/src/main/java/org/apache/druid/query/timeseries/TimeseriesQueryEngine.java:
##########
@@ -181,7 +181,8 @@ private Sequence<Result<TimeseriesResultValue>>
processVectorized(
.simple(granularizer.getBucketIterable())
.map(
bucketInterval -> {
- // Whether or not the current bucket is empty
+ // If current bucket is empty and skipEmptyBuckets is
true, return null.
+ // Otherwise, always call aggregators.init, then
aggregators.aggregateVector if there's any data.
Review Comment:
this comment probably seems more useful split up and down where we init the
aggs if we haven't already?
##########
processing/src/main/java/org/apache/druid/query/aggregation/NullableNumericAggregatorFactory.java:
##########
@@ -48,43 +48,65 @@
public abstract class NullableNumericAggregatorFactory<T extends
BaseNullableColumnValueSelector>
extends AggregatorFactory
{
+ /**
+ * If true, the aggregator will not return null values, even if there's no
data to aggregate. In this case, it would
+ * rely on the concrete implementation to return a non-null value.
+ */
+ public boolean forceNotNullable()
+ {
+ return false;
+ }
+
@Override
public final Aggregator factorize(ColumnSelectorFactory
columnSelectorFactory)
{
T selector = selector(columnSelectorFactory);
- BaseNullableColumnValueSelector nullSelector = makeNullSelector(selector,
columnSelectorFactory);
Aggregator aggregator = factorize(columnSelectorFactory, selector);
- return new NullableNumericAggregator(aggregator, nullSelector);
+ if (this.forceNotNullable()) {
+ return aggregator;
+ }
+ return new NullableNumericAggregator(aggregator,
makeNullSelector(selector, columnSelectorFactory));
}
@Override
public final BufferAggregator factorizeBuffered(ColumnSelectorFactory
columnSelectorFactory)
{
T selector = selector(columnSelectorFactory);
- BaseNullableColumnValueSelector nullSelector = makeNullSelector(selector,
columnSelectorFactory);
BufferAggregator aggregator = factorizeBuffered(columnSelectorFactory,
selector);
- return new NullableNumericBufferAggregator(aggregator, nullSelector);
+ if (this.forceNotNullable()) {
+ return aggregator;
+ }
+ return new NullableNumericBufferAggregator(aggregator,
makeNullSelector(selector, columnSelectorFactory));
}
@Override
- public final VectorAggregator factorizeVector(VectorColumnSelectorFactory
columnSelectorFactory)
+ public VectorAggregator factorizeVector(VectorColumnSelectorFactory
columnSelectorFactory)
Review Comment:
why not final?
##########
processing/src/test/java/org/apache/druid/query/groupby/GroupByQueryRunnerTest.java:
##########
@@ -3399,26 +3399,70 @@ public void
testGroupByWithUniquesAndPostAggWithSameName()
@Test
public void testGroupByWithCardinality()
{
- GroupByQuery query = makeQueryBuilder()
+ // arrange
Review Comment:
i guess these comments are like to help the reader or something? i haven't
really seen this pattern before so i looked it up 😅 . I don't personally think
they add much value, but i suppose can leave if you want :shrug:
--
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]
