[GitHub] [druid] clintropolis commented on a change in pull request #9317: ANY Aggregator should not skip null values implementation
clintropolis commented on a change in pull request #9317: ANY Aggregator should not skip null values implementation URL: https://github.com/apache/druid/pull/9317#discussion_r378150348 ## File path: docs/querying/aggregations.md ## @@ -238,7 +238,7 @@ Note that queries with first/last aggregators on a segment created with rollup e (Double/Float/Long/String) ANY aggregator cannot be used in ingestion spec, and should only be specified as part of queries. -If `druid.generic.useDefaultValueForNull=true` aggregation can returns the default value for null and does not prefer "non-null" values over the default value for null. If `druid.generic.useDefaultValueForNull=false`, then aggregation will returns any value including null. +Returns any value including null. This aggregator simplify and optimize the performance by returning the first encoutnered value (including null) Review comment: typo 'encoutnered' is causing CI failure 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 With regards, Apache Git Services - To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org For additional commands, e-mail: commits-h...@druid.apache.org
[GitHub] [druid] clintropolis commented on a change in pull request #9317: ANY Aggregator should not skip null values implementation
clintropolis commented on a change in pull request #9317: ANY Aggregator should not skip null values implementation URL: https://github.com/apache/druid/pull/9317#discussion_r378151135 ## File path: docs/querying/sql.md ## @@ -205,7 +205,7 @@ Only the COUNT aggregation can accept DISTINCT. |`EARLIEST(expr, maxBytesPerString)`|Like `EARLIEST(expr)`, but for strings. The `maxBytesPerString` parameter determines how much aggregation space to allocate per string. Strings longer than this limit will be truncated. This parameter should be set as low as possible, since high values will lead to wasted memory.| |`LATEST(expr)`|Returns the latest value of `expr`, which must be numeric. If `expr` comes from a relation with a timestamp column (like a Druid datasource) then "latest" is the value last encountered with the maximum overall timestamp of all values being aggregated. If `expr` does not come from a relation with a timestamp, then it is simply the last value encountered.| |`LATEST(expr, maxBytesPerString)`|Like `LATEST(expr)`, but for strings. The `maxBytesPerString` parameter determines how much aggregation space to allocate per string. Strings longer than this limit will be truncated. This parameter should be set as low as possible, since high values will lead to wasted memory.| -|`ANY_VALUE(expr)`|Returns any value of `expr`, which must be numeric. If `druid.generic.useDefaultValueForNull=true` this can return the default value for null and does not prefer "non-null" values over the default value for null. If `druid.generic.useDefaultValueForNull=false`, then this will return any value of `expr` including null| +|`ANY_VALUE(expr)`|Returns any value of `expr` including null. `expr` must be numeric. This aggregator simplify and optimize the performance by returning the first encoutnered value (including null)| Review comment: nit: should be 'This aggregator _can_ simplify..' Should this note also be added to the description of string any value? 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 With regards, Apache Git Services - To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org For additional commands, e-mail: commits-h...@druid.apache.org
[GitHub] [druid] clintropolis commented on a change in pull request #9317: ANY Aggregator should not skip null values implementation
clintropolis commented on a change in pull request #9317: ANY Aggregator should
not skip null values implementation
URL: https://github.com/apache/druid/pull/9317#discussion_r378152939
##
File path:
processing/src/main/java/org/apache/druid/query/aggregation/any/NumericAnyBufferAggregator.java
##
@@ -32,11 +32,11 @@
public abstract class NumericAnyBufferAggregator
implements BufferAggregator
{
- private static final byte BYTE_FLAG_IS_NOT_SET = 0;
- private static final byte BYTE_FLAG_IS_SET = 1;
- private static final int IS_FOUND_FLAG_OFFSET_POSITION = 0;
- private static final int IS_NULL_FLAG_OFFSET_POSITION =
IS_FOUND_FLAG_OFFSET_POSITION + Byte.BYTES;
- private static final int FOUND_VALUE_OFFSET_POSITION =
IS_NULL_FLAG_OFFSET_POSITION + Byte.BYTES;
+ // Rightmost bit for is null check (0 for is null and 1 for not null)
+ // Second rightmost bit for is found check (0 for not found and 1 for found)
+ private static final byte BYTE_FLAG_FOUND_MASK = 0b0010;
Review comment:
super super nit, but can you use hex? (`0x02` and `0x01` for
`BYTE_FLAG_NULL_MASK`)
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
With regards,
Apache Git Services
-
To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org
For additional commands, e-mail: commits-h...@druid.apache.org
[GitHub] [druid] clintropolis commented on a change in pull request #9317: ANY Aggregator should not skip null values implementation
clintropolis commented on a change in pull request #9317: ANY Aggregator should
not skip null values implementation
URL: https://github.com/apache/druid/pull/9317#discussion_r377944007
##
File path:
processing/src/main/java/org/apache/druid/query/aggregation/any/NumericAnyBufferAggregator.java
##
@@ -0,0 +1,103 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.query.aggregation.any;
+
+import org.apache.druid.common.config.NullHandling;
+import org.apache.druid.query.aggregation.BufferAggregator;
+import org.apache.druid.query.monomorphicprocessing.RuntimeShapeInspector;
+import org.apache.druid.segment.BaseNullableColumnValueSelector;
+
+import java.nio.ByteBuffer;
+
+/**
+ * Base type for buffer based 'any' aggregator for primitive numeric column
selectors
+ */
+public abstract class NumericAnyBufferAggregator
+implements BufferAggregator
+{
+ private static final byte BYTE_FLAG_IS_NOT_SET = 0;
+ private static final byte BYTE_FLAG_IS_SET = 1;
+ private static final int IS_FOUND_FLAG_OFFSET_POSITION = 0;
Review comment:
Further, since is null is only going to be either 0 or 1, you save a byte
per agg result and could use any of the high bits of this null byte to set
whether or not you have found a value. Instead of an offset for null flag, you
define a bit mask and your found check becomes something like
`buf.get(position) & FOUND_MASK == FOUND_MASK` or whatever.
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
With regards,
Apache Git Services
-
To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org
For additional commands, e-mail: commits-h...@druid.apache.org
[GitHub] [druid] clintropolis commented on a change in pull request #9317: ANY Aggregator should not skip null values implementation
clintropolis commented on a change in pull request #9317: ANY Aggregator should
not skip null values implementation
URL: https://github.com/apache/druid/pull/9317#discussion_r377931492
##
File path:
processing/src/main/java/org/apache/druid/query/aggregation/any/DoubleAnyAggregator.java
##
@@ -19,44 +19,35 @@
package org.apache.druid.query.aggregation.any;
-import org.apache.druid.query.aggregation.Aggregator;
-import org.apache.druid.query.aggregation.NullableNumericAggregator;
-import org.apache.druid.query.aggregation.NullableNumericAggregatorFactory;
import org.apache.druid.segment.BaseDoubleColumnValueSelector;
+import javax.annotation.Nullable;
+
/**
- * This Aggregator is created by the {@link DoubleAnyAggregatorFactory} which
extends from
- * {@link NullableNumericAggregatorFactory}. If null needs to be handle, then
{@link NullableNumericAggregatorFactory}
- * will wrap this aggregator in {@link NullableNumericAggregator} and can
handle all null in that class.
- * Hence, no null will ever be pass into this aggregator from the
valueSelector.
+ * This Aggregator is created by the {@link DoubleAnyAggregatorFactory} which
has no special null handling logic.
Review comment:
I'm not sure this is really worth mentioning. This is the standard case for
aggregator implementations, since it is less common for an agg implementation
to have magic null handling wrapped around it than just handling the nulls
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org
For additional commands, e-mail: commits-h...@druid.apache.org
[GitHub] [druid] clintropolis commented on a change in pull request #9317: ANY Aggregator should not skip null values implementation
clintropolis commented on a change in pull request #9317: ANY Aggregator should
not skip null values implementation
URL: https://github.com/apache/druid/pull/9317#discussion_r377944007
##
File path:
processing/src/main/java/org/apache/druid/query/aggregation/any/NumericAnyBufferAggregator.java
##
@@ -0,0 +1,103 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.query.aggregation.any;
+
+import org.apache.druid.common.config.NullHandling;
+import org.apache.druid.query.aggregation.BufferAggregator;
+import org.apache.druid.query.monomorphicprocessing.RuntimeShapeInspector;
+import org.apache.druid.segment.BaseNullableColumnValueSelector;
+
+import java.nio.ByteBuffer;
+
+/**
+ * Base type for buffer based 'any' aggregator for primitive numeric column
selectors
+ */
+public abstract class NumericAnyBufferAggregator
+implements BufferAggregator
+{
+ private static final byte BYTE_FLAG_IS_NOT_SET = 0;
+ private static final byte BYTE_FLAG_IS_SET = 1;
+ private static final int IS_FOUND_FLAG_OFFSET_POSITION = 0;
Review comment:
Further, since is null is only going to be either 0 or 1, you save a byte
per result and could use any of the high bits of this null byte to set whether
or not you have found a value. Instead of an offset for null flag, you define a
bit mask and your found check becomes something like `buf.get(position) &
FOUND_MASK == FOUND_MASK` or whatever.
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
With regards,
Apache Git Services
-
To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org
For additional commands, e-mail: commits-h...@druid.apache.org
[GitHub] [druid] clintropolis commented on a change in pull request #9317: ANY Aggregator should not skip null values implementation
clintropolis commented on a change in pull request #9317: ANY Aggregator should
not skip null values implementation
URL: https://github.com/apache/druid/pull/9317#discussion_r376653240
##
File path:
processing/src/main/java/org/apache/druid/query/aggregation/any/DoubleAnyAggregatorFactory.java
##
@@ -19,103 +19,214 @@
package org.apache.druid.query.aggregation.any;
-import com.fasterxml.jackson.annotation.JacksonInject;
import com.fasterxml.jackson.annotation.JsonCreator;
import com.fasterxml.jackson.annotation.JsonProperty;
-import org.apache.druid.math.expr.ExprMacroTable;
+import com.google.common.base.Preconditions;
+import org.apache.druid.java.util.common.UOE;
+import org.apache.druid.query.aggregation.AggregateCombiner;
import org.apache.druid.query.aggregation.Aggregator;
import org.apache.druid.query.aggregation.AggregatorFactory;
import org.apache.druid.query.aggregation.AggregatorUtil;
import org.apache.druid.query.aggregation.BufferAggregator;
-import org.apache.druid.query.aggregation.SimpleDoubleAggregatorFactory;
+import org.apache.druid.query.aggregation.DoubleSumAggregator;
import org.apache.druid.query.cache.CacheKeyBuilder;
import org.apache.druid.segment.BaseDoubleColumnValueSelector;
+import org.apache.druid.segment.ColumnSelectorFactory;
+import org.apache.druid.segment.NilColumnValueSelector;
+import org.apache.druid.segment.column.ColumnHolder;
import javax.annotation.Nullable;
+import java.nio.ByteBuffer;
import java.util.Collections;
+import java.util.Comparator;
import java.util.List;
+import java.util.Objects;
-public class DoubleAnyAggregatorFactory extends SimpleDoubleAggregatorFactory
+public class DoubleAnyAggregatorFactory extends AggregatorFactory
{
+ private static final Comparator VALUE_COMPARATOR =
Comparator.nullsFirst(Double::compare);
+
+ private static final Aggregator NIL_AGGREGATOR = new DoubleAnyAggregator(
+ NilColumnValueSelector.instance()
+ )
+ {
+@Override
+public void aggregate()
+{
+ // no-op
+}
+ };
+
+ private static final BufferAggregator NIL_BUFFER_AGGREGATOR = new
DoubleAnyBufferAggregator(
+ NilColumnValueSelector.instance()
+ )
+ {
+@Override
+public void aggregate(ByteBuffer buf, int position)
+{
+ // no-op
+}
+ };
+
+ private final String fieldName;
+ private final String name;
+ private final boolean storeDoubleAsFloat;
+
@JsonCreator
public DoubleAnyAggregatorFactory(
@JsonProperty("name") String name,
- @JsonProperty("fieldName") final String fieldName,
- @JsonProperty("expression") @Nullable String expression,
- @JacksonInject ExprMacroTable macroTable
+ @JsonProperty("fieldName") final String fieldName
)
{
-super(macroTable, name, fieldName, expression);
- }
+Preconditions.checkNotNull(name, "Must have a valid, non-null aggregator
name");
+Preconditions.checkNotNull(fieldName, "Must have a valid, non-null
fieldName");
- public DoubleAnyAggregatorFactory(String name, String fieldName)
- {
-this(name, fieldName, null, ExprMacroTable.nil());
+this.name = name;
+this.fieldName = fieldName;
+this.storeDoubleAsFloat = ColumnHolder.storeDoubleAsFloat();
}
@Override
- protected double nullValue()
+ public Aggregator factorize(ColumnSelectorFactory metricFactory)
{
-return Double.NaN;
+final BaseDoubleColumnValueSelector valueSelector =
metricFactory.makeColumnValueSelector(fieldName);
+if (valueSelector instanceof NilColumnValueSelector) {
+ return NIL_AGGREGATOR;
+} else {
+ return new DoubleAnyAggregator(
+ valueSelector
+ );
+}
}
@Override
- protected Aggregator buildAggregator(BaseDoubleColumnValueSelector selector)
+ public BufferAggregator factorizeBuffered(ColumnSelectorFactory
metricFactory)
{
-return new DoubleAnyAggregator(selector);
+final BaseDoubleColumnValueSelector valueSelector =
metricFactory.makeColumnValueSelector(fieldName);
+if (valueSelector instanceof NilColumnValueSelector) {
+ return NIL_BUFFER_AGGREGATOR;
+} else {
+ return new DoubleAnyBufferAggregator(
+ valueSelector
+ );
+}
}
@Override
- protected BufferAggregator
buildBufferAggregator(BaseDoubleColumnValueSelector selector)
+ public Comparator getComparator()
{
-return new DoubleAnyBufferAggregator(selector);
+return VALUE_COMPARATOR;
}
@Override
@Nullable
public Object combine(@Nullable Object lhs, @Nullable Object rhs)
{
-if (lhs != null) {
- return lhs;
-} else {
- return rhs;
-}
+return lhs;
+ }
+
+ @Override
+ public AggregateCombiner makeAggregateCombiner()
+ {
+throw new UOE("DoubleAnyAggregatorFactory is not supported during
ingestion for rollup");
}
@Override
public AggregatorFactory getCombiningFactory()
{
-return new DoubleAnyAggregatorFactory(name, name, null,
[GitHub] [druid] clintropolis commented on a change in pull request #9317: ANY Aggregator should not skip null values implementation
clintropolis commented on a change in pull request #9317: ANY Aggregator should
not skip null values implementation
URL: https://github.com/apache/druid/pull/9317#discussion_r377938220
##
File path:
processing/src/main/java/org/apache/druid/query/aggregation/any/NumericAnyBufferAggregator.java
##
@@ -0,0 +1,103 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.query.aggregation.any;
+
+import org.apache.druid.common.config.NullHandling;
+import org.apache.druid.query.aggregation.BufferAggregator;
+import org.apache.druid.query.monomorphicprocessing.RuntimeShapeInspector;
+import org.apache.druid.segment.BaseNullableColumnValueSelector;
+
+import java.nio.ByteBuffer;
+
+/**
+ * Base type for buffer based 'any' aggregator for primitive numeric column
selectors
+ */
+public abstract class NumericAnyBufferAggregator
+implements BufferAggregator
+{
+ private static final byte BYTE_FLAG_IS_NOT_SET = 0;
+ private static final byte BYTE_FLAG_IS_SET = 1;
+ private static final int IS_FOUND_FLAG_OFFSET_POSITION = 0;
+ private static final int IS_NULL_FLAG_OFFSET_POSITION =
IS_FOUND_FLAG_OFFSET_POSITION + Byte.BYTES;
+ private static final int FOUND_VALUE_OFFSET_POSITION =
IS_NULL_FLAG_OFFSET_POSITION + Byte.BYTES;
Review comment:
nit: suggest just making this package private and having subclasses use this
directly in their putValue implementations instead of
`getFoundValueStoredPosition` function call, and maybe just calling it
`VALUE_OFFSET` or `FOUND_VALUE_OFFSET` since the position seems redundant.
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
With regards,
Apache Git Services
-
To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org
For additional commands, e-mail: commits-h...@druid.apache.org
[GitHub] [druid] clintropolis commented on a change in pull request #9317: ANY Aggregator should not skip null values implementation
clintropolis commented on a change in pull request #9317: ANY Aggregator should
not skip null values implementation
URL: https://github.com/apache/druid/pull/9317#discussion_r377942115
##
File path: server/src/main/java/org/apache/druid/server/QueryLifecycle.java
##
@@ -318,14 +318,12 @@ public void emitLogsAndMetrics(
if (e != null) {
statsMap.put("exception", e.toString());
-
-if (e instanceof QueryInterruptedException) {
- // Mimic behavior from QueryResource, where this code was originally
taken from.
- log.noStackTrace().warn(e, "Exception while processing queryId
[%s]", baseQuery.getId());
- statsMap.put("interrupted", true);
- statsMap.put("reason", e.toString());
-}
+// Mimic behavior from QueryResource, where this code was originally
taken from.
Review comment:
Why this change? I don't think it is correct, since this stuff should only
be set if it was a `QueryInterruptedException`
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
With regards,
Apache Git Services
-
To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org
For additional commands, e-mail: commits-h...@druid.apache.org
[GitHub] [druid] clintropolis commented on a change in pull request #9317: ANY Aggregator should not skip null values implementation
clintropolis commented on a change in pull request #9317: ANY Aggregator should not skip null values implementation URL: https://github.com/apache/druid/pull/9317#discussion_r377929896 ## File path: docs/querying/sql.md ## @@ -205,7 +205,7 @@ Only the COUNT aggregation can accept DISTINCT. |`EARLIEST(expr, maxBytesPerString)`|Like `EARLIEST(expr)`, but for strings. The `maxBytesPerString` parameter determines how much aggregation space to allocate per string. Strings longer than this limit will be truncated. This parameter should be set as low as possible, since high values will lead to wasted memory.| |`LATEST(expr)`|Returns the latest value of `expr`, which must be numeric. If `expr` comes from a relation with a timestamp column (like a Druid datasource) then "latest" is the value last encountered with the maximum overall timestamp of all values being aggregated. If `expr` does not come from a relation with a timestamp, then it is simply the last value encountered.| |`LATEST(expr, maxBytesPerString)`|Like `LATEST(expr)`, but for strings. The `maxBytesPerString` parameter determines how much aggregation space to allocate per string. Strings longer than this limit will be truncated. This parameter should be set as low as possible, since high values will lead to wasted memory.| -|`ANY_VALUE(expr)`|Returns any value of `expr`, which must be numeric. If `druid.generic.useDefaultValueForNull=true` this can return the default value for null and does not prefer "non-null" values over the default value for null. If `druid.generic.useDefaultValueForNull=false`, then this will return any non-null value of `expr`| +|`ANY_VALUE(expr)`|Returns any value of `expr`, which must be numeric. If `druid.generic.useDefaultValueForNull=true` this can return the default value for null and does not prefer "non-null" values over the default value for null. If `druid.generic.useDefaultValueForNull=false`, then this will return any value of `expr` including null| Review comment: I sort of think this should just be simplified so it is less confusing, maybe something like: > Returns any value of `expr`, including null. Also, expr does not need to be numeric since you implemented a string any aggregator in the previous PR. 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 With regards, Apache Git Services - To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org For additional commands, e-mail: commits-h...@druid.apache.org
[GitHub] [druid] clintropolis commented on a change in pull request #9317: ANY Aggregator should not skip null values implementation
clintropolis commented on a change in pull request #9317: ANY Aggregator should
not skip null values implementation
URL: https://github.com/apache/druid/pull/9317#discussion_r376649135
##
File path:
processing/src/main/java/org/apache/druid/query/aggregation/any/DoubleAnyAggregatorFactory.java
##
@@ -19,103 +19,214 @@
package org.apache.druid.query.aggregation.any;
-import com.fasterxml.jackson.annotation.JacksonInject;
import com.fasterxml.jackson.annotation.JsonCreator;
import com.fasterxml.jackson.annotation.JsonProperty;
-import org.apache.druid.math.expr.ExprMacroTable;
+import com.google.common.base.Preconditions;
+import org.apache.druid.java.util.common.UOE;
+import org.apache.druid.query.aggregation.AggregateCombiner;
import org.apache.druid.query.aggregation.Aggregator;
import org.apache.druid.query.aggregation.AggregatorFactory;
import org.apache.druid.query.aggregation.AggregatorUtil;
import org.apache.druid.query.aggregation.BufferAggregator;
-import org.apache.druid.query.aggregation.SimpleDoubleAggregatorFactory;
+import org.apache.druid.query.aggregation.DoubleSumAggregator;
import org.apache.druid.query.cache.CacheKeyBuilder;
import org.apache.druid.segment.BaseDoubleColumnValueSelector;
+import org.apache.druid.segment.ColumnSelectorFactory;
+import org.apache.druid.segment.NilColumnValueSelector;
+import org.apache.druid.segment.column.ColumnHolder;
import javax.annotation.Nullable;
+import java.nio.ByteBuffer;
import java.util.Collections;
+import java.util.Comparator;
import java.util.List;
+import java.util.Objects;
-public class DoubleAnyAggregatorFactory extends SimpleDoubleAggregatorFactory
+public class DoubleAnyAggregatorFactory extends AggregatorFactory
{
+ private static final Comparator VALUE_COMPARATOR =
Comparator.nullsFirst(Double::compare);
+
+ private static final Aggregator NIL_AGGREGATOR = new DoubleAnyAggregator(
+ NilColumnValueSelector.instance()
+ )
+ {
+@Override
+public void aggregate()
+{
+ // no-op
+}
+ };
+
+ private static final BufferAggregator NIL_BUFFER_AGGREGATOR = new
DoubleAnyBufferAggregator(
+ NilColumnValueSelector.instance()
+ )
+ {
+@Override
+public void aggregate(ByteBuffer buf, int position)
+{
+ // no-op
+}
+ };
+
+ private final String fieldName;
+ private final String name;
+ private final boolean storeDoubleAsFloat;
+
@JsonCreator
public DoubleAnyAggregatorFactory(
@JsonProperty("name") String name,
- @JsonProperty("fieldName") final String fieldName,
- @JsonProperty("expression") @Nullable String expression,
- @JacksonInject ExprMacroTable macroTable
+ @JsonProperty("fieldName") final String fieldName
)
{
-super(macroTable, name, fieldName, expression);
- }
+Preconditions.checkNotNull(name, "Must have a valid, non-null aggregator
name");
+Preconditions.checkNotNull(fieldName, "Must have a valid, non-null
fieldName");
- public DoubleAnyAggregatorFactory(String name, String fieldName)
- {
-this(name, fieldName, null, ExprMacroTable.nil());
+this.name = name;
+this.fieldName = fieldName;
+this.storeDoubleAsFloat = ColumnHolder.storeDoubleAsFloat();
}
@Override
- protected double nullValue()
+ public Aggregator factorize(ColumnSelectorFactory metricFactory)
{
-return Double.NaN;
+final BaseDoubleColumnValueSelector valueSelector =
metricFactory.makeColumnValueSelector(fieldName);
+if (valueSelector instanceof NilColumnValueSelector) {
+ return NIL_AGGREGATOR;
+} else {
+ return new DoubleAnyAggregator(
+ valueSelector
+ );
+}
}
@Override
- protected Aggregator buildAggregator(BaseDoubleColumnValueSelector selector)
+ public BufferAggregator factorizeBuffered(ColumnSelectorFactory
metricFactory)
{
-return new DoubleAnyAggregator(selector);
+final BaseDoubleColumnValueSelector valueSelector =
metricFactory.makeColumnValueSelector(fieldName);
+if (valueSelector instanceof NilColumnValueSelector) {
+ return NIL_BUFFER_AGGREGATOR;
+} else {
+ return new DoubleAnyBufferAggregator(
+ valueSelector
+ );
+}
}
@Override
- protected BufferAggregator
buildBufferAggregator(BaseDoubleColumnValueSelector selector)
+ public Comparator getComparator()
{
-return new DoubleAnyBufferAggregator(selector);
+return VALUE_COMPARATOR;
}
@Override
@Nullable
public Object combine(@Nullable Object lhs, @Nullable Object rhs)
{
-if (lhs != null) {
- return lhs;
-} else {
- return rhs;
-}
+return lhs;
Review comment:
I think from like a .. user satisfaction perspective, it might still be nice
to prefer non-null values since it is still legitimate.
This is an automated message from the Apache Git Service.
To respond to the message, please
[GitHub] [druid] clintropolis commented on a change in pull request #9317: ANY Aggregator should not skip null values implementation
clintropolis commented on a change in pull request #9317: ANY Aggregator should
not skip null values implementation
URL: https://github.com/apache/druid/pull/9317#discussion_r377935040
##
File path:
processing/src/main/java/org/apache/druid/query/aggregation/any/NumericAnyBufferAggregator.java
##
@@ -0,0 +1,103 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.query.aggregation.any;
+
+import org.apache.druid.common.config.NullHandling;
+import org.apache.druid.query.aggregation.BufferAggregator;
+import org.apache.druid.query.monomorphicprocessing.RuntimeShapeInspector;
+import org.apache.druid.segment.BaseNullableColumnValueSelector;
+
+import java.nio.ByteBuffer;
+
+/**
+ * Base type for buffer based 'any' aggregator for primitive numeric column
selectors
+ */
+public abstract class NumericAnyBufferAggregator
+implements BufferAggregator
+{
+ private static final byte BYTE_FLAG_IS_NOT_SET = 0;
Review comment:
Please use `NullHandling.IS_NULL_BYTE` and `NullHandling.IS_NOT_NULL_BYTE`
to be consistent with other aggregators, at least for the 'is null' byte.
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
With regards,
Apache Git Services
-
To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org
For additional commands, e-mail: commits-h...@druid.apache.org
[GitHub] [druid] clintropolis commented on a change in pull request #9317: ANY Aggregator should not skip null values implementation
clintropolis commented on a change in pull request #9317: ANY Aggregator should
not skip null values implementation
URL: https://github.com/apache/druid/pull/9317#discussion_r377934075
##
File path:
processing/src/main/java/org/apache/druid/query/aggregation/any/NumericAnyBufferAggregator.java
##
@@ -0,0 +1,103 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.query.aggregation.any;
+
+import org.apache.druid.common.config.NullHandling;
+import org.apache.druid.query.aggregation.BufferAggregator;
+import org.apache.druid.query.monomorphicprocessing.RuntimeShapeInspector;
+import org.apache.druid.segment.BaseNullableColumnValueSelector;
+
+import java.nio.ByteBuffer;
+
+/**
+ * Base type for buffer based 'any' aggregator for primitive numeric column
selectors
+ */
+public abstract class NumericAnyBufferAggregator
+implements BufferAggregator
+{
+ private static final byte BYTE_FLAG_IS_NOT_SET = 0;
+ private static final byte BYTE_FLAG_IS_SET = 1;
+ private static final int IS_FOUND_FLAG_OFFSET_POSITION = 0;
Review comment:
nit: I think this `0` offset variable makes the put and get operations more
complicated than they need to be, suggest just dropping this and not adding or
removing anything.
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
With regards,
Apache Git Services
-
To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org
For additional commands, e-mail: commits-h...@druid.apache.org
