Jackie-Jiang commented on code in PR #9848:
URL: https://github.com/apache/pinot/pull/9848#discussion_r1036349202
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/AggregationResultHolder.java:
##########
@@ -30,6 +30,12 @@ public interface AggregationResultHolder {
*/
void setValue(double value);
+ /**
+ * Set the 'primitive long' aggregation result.
Review Comment:
(minor) same for other places
```suggestion
* Set the 'primitive int' aggregation result.
```
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/DoubleAggregationResultHolder.java:
##########
@@ -42,6 +42,11 @@ public void setValue(double value) {
_value = value;
}
+ @Override
+ public void setValue(int value) {
+ throw new RuntimeException("Method 'setValue' (with long value) not
supported for class " + getClass().getName());
Review Comment:
(minor) same for other places
```suggestion
throw new RuntimeException("Method 'setValue' (with int value) not
supported for class " + getClass().getName());
```
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/BaseBooleanAggregateFunction.java:
##########
@@ -0,0 +1,252 @@
+/**
+ * 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.pinot.core.query.aggregation.function;
+
+import java.util.Map;
+import org.apache.pinot.common.request.context.ExpressionContext;
+import org.apache.pinot.common.utils.DataSchema;
+import org.apache.pinot.core.common.BlockValSet;
+import org.apache.pinot.core.query.aggregation.AggregationResultHolder;
+import org.apache.pinot.core.query.aggregation.IntAggregateResultHolder;
+import org.apache.pinot.core.query.aggregation.ObjectAggregationResultHolder;
+import org.apache.pinot.core.query.aggregation.groupby.GroupByResultHolder;
+import org.apache.pinot.core.query.aggregation.groupby.IntGroupByResultHolder;
+import
org.apache.pinot.core.query.aggregation.groupby.ObjectGroupByResultHolder;
+import org.apache.pinot.spi.data.FieldSpec;
+import org.roaringbitmap.RoaringBitmap;
+
+
+// TODO: change this to implement BaseSingleInputAggregationFunction<Boolean,
Boolean> when we get proper
+// handling of booleans in serialization - today this would fail because
ColumnDataType#convert assumes
+// that the boolean is encoded as its stored type (an integer)
+public abstract class BaseBooleanAggregateFunction extends
BaseSingleInputAggregationFunction<Integer, Integer> {
+
+ private final BooleanMerge _merger;
+ private final boolean _nullHandlingEnabled;
+
+ protected enum BooleanMerge {
+ AND {
+ @Override
+ int merge(int left, int right) {
+ return left & right;
+ }
+
+ @Override
+ boolean isTerminal(int agg) {
+ return agg == 0;
+ }
+
+ @Override
+ int getDefaultValue() {
+ return 1;
+ }
+ },
+ OR {
+ @Override
+ int merge(int left, int right) {
+ return left | right;
+ }
+
+ @Override
+ boolean isTerminal(int agg) {
+ return agg > 0;
+ }
+
+ @Override
+ int getDefaultValue() {
+ return 0;
+ }
+ };
+
+ abstract int merge(int left, int right);
+
+ abstract boolean isTerminal(int agg);
+
+ abstract int getDefaultValue();
+ }
+
+ protected BaseBooleanAggregateFunction(ExpressionContext expression, boolean
nullHandlingEnabled,
+ BooleanMerge merger) {
+ super(expression);
+ _nullHandlingEnabled = nullHandlingEnabled;
+ _merger = merger;
+ }
+
+ @Override
+ public AggregationResultHolder createAggregationResultHolder() {
+ return _nullHandlingEnabled
+ ? new ObjectAggregationResultHolder()
+ : new IntAggregateResultHolder(_merger.getDefaultValue());
+ }
+
+ @Override
+ public GroupByResultHolder createGroupByResultHolder(int initialCapacity,
int maxCapacity) {
+ return _nullHandlingEnabled
+ ? new ObjectGroupByResultHolder(initialCapacity, maxCapacity)
+ : new IntGroupByResultHolder(initialCapacity, maxCapacity,
_merger.getDefaultValue());
+ }
+
+ @Override
+ public void aggregate(int length, AggregationResultHolder
aggregationResultHolder,
+ Map<ExpressionContext, BlockValSet> blockValSetMap) {
+ BlockValSet blockValSet = blockValSetMap.get(_expression);
+
+ if (blockValSet.getValueType() != FieldSpec.DataType.BOOLEAN) {
+ throw new IllegalArgumentException(
+ String.format("Unsupported data type %s for %s",
getType().getName(), blockValSet.getValueType()));
+ }
+
+ int[] bools = blockValSet.getIntValuesSV();
+ if (_nullHandlingEnabled) {
+ int agg = getInt(aggregationResultHolder.getResult());
+
+ RoaringBitmap nullBitmap = blockValSet.getNullBitmap();
+ if (nullBitmap == null) {
+ nullBitmap = new RoaringBitmap();
+ } else if (nullBitmap.getCardinality() > length) {
+ return;
+ }
+
+ // early terminate on a per-block level to allow the
+ // loop below to be more tightly optimized (avoid a branch)
+ if (_merger.isTerminal(agg)) {
Review Comment:
(minor) suggest moving this above the nullBitmap check, which is much more
costly
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/AggregationFunctionType.java:
##########
@@ -79,7 +79,11 @@ public enum AggregationFunctionType {
PERCENTILERAWESTMV("percentileRawEstMV"),
PERCENTILETDIGESTMV("percentileTDigestMV"),
PERCENTILERAWTDIGESTMV("percentileRawTDigestMV"),
- DISTINCT("distinct");
+ DISTINCT("distinct"),
+
+ // boolean aggregate functions
+ BOOLAND("bool_and"),
Review Comment:
Keep the name camel case (`boolAnd` and `boolOr`)
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/DoubleAggregationResultHolder.java:
##########
@@ -61,6 +66,16 @@ public double getDoubleResult() {
return _value;
}
+ /**
+ * {@inheritDoc}
+ *
+ * @return
+ */
+ @Override
+ public int getIntResult() {
+ throw new RuntimeException("Method 'getLongResult' not supported for class
" + getClass().getName());
Review Comment:
(minor) same for other places
```suggestion
throw new RuntimeException("Method 'getIntResult' not supported for
class " + getClass().getName());
```
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/BaseBooleanAggregateFunction.java:
##########
@@ -0,0 +1,252 @@
+/**
+ * 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.pinot.core.query.aggregation.function;
+
+import java.util.Map;
+import org.apache.pinot.common.request.context.ExpressionContext;
+import org.apache.pinot.common.utils.DataSchema;
+import org.apache.pinot.core.common.BlockValSet;
+import org.apache.pinot.core.query.aggregation.AggregationResultHolder;
+import org.apache.pinot.core.query.aggregation.IntAggregateResultHolder;
+import org.apache.pinot.core.query.aggregation.ObjectAggregationResultHolder;
+import org.apache.pinot.core.query.aggregation.groupby.GroupByResultHolder;
+import org.apache.pinot.core.query.aggregation.groupby.IntGroupByResultHolder;
+import
org.apache.pinot.core.query.aggregation.groupby.ObjectGroupByResultHolder;
+import org.apache.pinot.spi.data.FieldSpec;
+import org.roaringbitmap.RoaringBitmap;
+
+
+// TODO: change this to implement BaseSingleInputAggregationFunction<Boolean,
Boolean> when we get proper
+// handling of booleans in serialization - today this would fail because
ColumnDataType#convert assumes
+// that the boolean is encoded as its stored type (an integer)
+public abstract class BaseBooleanAggregateFunction extends
BaseSingleInputAggregationFunction<Integer, Integer> {
Review Comment:
(nit) We usually call it `AggregationFunction` instead of
`AggregateFunction`. Keeping them consistent can help with the code search.
Same for the sub-classes
--
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: commits-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
