clintropolis commented on code in PR #18307:
URL: https://github.com/apache/druid/pull/18307#discussion_r2223837371
##########
processing/src/main/java/org/apache/druid/query/aggregation/NullableNumericAggregatorFactory.java:
##########
@@ -48,31 +50,44 @@
public abstract class NullableNumericAggregatorFactory<T extends
BaseNullableColumnValueSelector>
extends AggregatorFactory
{
+ /**
+ * If this aggregator does not aggregate any values, it will return this
value.
+ */
+ @Nullable
+ public Number getDefaultValue()
+ {
+ return null;
+ }
Review Comment:
since number is not used and really up to the aggregator itself, this should
just be `isNullable()` and return a boolean
##########
processing/src/main/java/org/apache/druid/query/aggregation/LongSumAggregatorFactory.java:
##########
@@ -36,12 +36,33 @@ public class LongSumAggregatorFactory extends
SimpleLongAggregatorFactory
{
private final Supplier<byte[]> cacheKey;
+ /**
+ * The initValue is the value that will be returned when the aggregator
doesn't collect any values.
+ * <p>
+ * For example,
+ * <li> the sum(column) should return null when there's no rows in the
segment, thus initValue is null.
+ * <li> the count(column) should return 0 when there's no rows in the
segment, thus initValue is 0.
+ */
+ @Nullable
+ private final Long initValue;
+
+ public LongSumAggregatorFactory(String name, String fieldName)
+ {
+ this(name, fieldName, null, ExprMacroTable.nil(), null);
+ }
+
+ public LongSumAggregatorFactory(String name, String fieldName, String
expression, ExprMacroTable macroTable)
+ {
+ this(name, fieldName, expression, macroTable, null);
+ }
+
@JsonCreator
public LongSumAggregatorFactory(
@JsonProperty("name") String name,
@JsonProperty("fieldName") final String fieldName,
@JsonProperty("expression") @Nullable String expression,
- @JacksonInject ExprMacroTable macroTable
+ @JacksonInject ExprMacroTable macroTable,
+ @JsonProperty("initValue") @Nullable Long initValue
Review Comment:
i don't think this needs to be on the json at the moment, the count
aggregator only calls this internally, and im not sure it is something we want
to expose to users yet in the current form, we can always add later if we need
it
##########
processing/src/main/java/org/apache/druid/query/aggregation/NullableNumericVectorAggregator.java:
##########
@@ -59,17 +59,25 @@ public class NullableNumericVectorAggregator implements
VectorAggregator
@Nullable
private int[] vAggregationRows = null;
- NullableNumericVectorAggregator(VectorAggregator delegate,
VectorValueSelector selector)
+ private final boolean defaultNull;
+
+ NullableNumericVectorAggregator(VectorAggregator delegate,
VectorValueSelector selector, boolean defaultNull)
Review Comment:
these changes are not needed if we just return the base `VectorAggregator`
##########
processing/src/main/java/org/apache/druid/query/aggregation/NullableNumericAggregatorFactory.java:
##########
@@ -48,31 +50,44 @@
public abstract class NullableNumericAggregatorFactory<T extends
BaseNullableColumnValueSelector>
extends AggregatorFactory
{
+ /**
+ * If this aggregator does not aggregate any values, it will return this
value.
+ */
+ @Nullable
+ public Number getDefaultValue()
+ {
+ return null;
+ }
+
@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.getDefaultValue() != null) {
+ 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.getDefaultValue() != null) {
+ return aggregator;
+ }
+ return new NullableNumericBufferAggregator(aggregator,
makeNullSelector(selector, columnSelectorFactory));
}
@Override
- public final VectorAggregator factorizeVector(VectorColumnSelectorFactory
columnSelectorFactory)
+ public VectorAggregator factorizeVector(VectorColumnSelectorFactory
columnSelectorFactory)
{
Preconditions.checkState(canVectorize(columnSelectorFactory), "Cannot
vectorize");
VectorValueSelector selector = vectorSelector(columnSelectorFactory);
VectorAggregator aggregator = factorizeVector(columnSelectorFactory,
selector);
- return new NullableNumericVectorAggregator(aggregator, selector);
+ return new NullableNumericVectorAggregator(aggregator, selector,
this.getDefaultValue() == null);
Review Comment:
why can't this just return the `VectorAggregator` like the other factorize
methods?
##########
processing/src/main/java/org/apache/druid/query/aggregation/LongSumAggregatorFactory.java:
##########
@@ -36,12 +36,33 @@ public class LongSumAggregatorFactory extends
SimpleLongAggregatorFactory
{
private final Supplier<byte[]> cacheKey;
+ /**
+ * The initValue is the value that will be returned when the aggregator
doesn't collect any values.
+ * <p>
+ * For example,
+ * <li> the sum(column) should return null when there's no rows in the
segment, thus initValue is null.
+ * <li> the count(column) should return 0 when there's no rows in the
segment, thus initValue is 0.
+ */
+ @Nullable
Review Comment:
i think this should just be `isNullable` and a boolean instead of
`initValue` since it isn't used by the aggregator itself
--
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]
