Copilot commented on code in PR #19211:
URL: https://github.com/apache/pinot/pull/19211#discussion_r3754272900
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/CovarianceAggregationFunction.java:
##########
@@ -135,31 +180,31 @@ public void aggregateGroupBySV(int length, int[]
groupKeyArray, GroupByResultHol
Map<ExpressionContext, BlockValSet> blockValSetMap) {
double[] values1 =
StatisticalAggregationFunctionUtils.getValSet(blockValSetMap, _expression1);
double[] values2 =
StatisticalAggregationFunctionUtils.getValSet(blockValSetMap, _expression2);
- for (int i = 0; i < length; i++) {
- setGroupByResult(groupKeyArray[i], groupByResultHolder, values1[i],
values2[i], values1[i] * values2[i], 1L);
- }
+ forEachNotNull(length, blockValSetMap.get(_expression1),
blockValSetMap.get(_expression2), (from, to) -> {
+ for (int i = from; i < to; i++) {
+ setGroupByResult(groupKeyArray[i], groupByResultHolder, values1[i],
values2[i], values1[i] * values2[i], 1L);
+ }
+ });
}
@Override
public void aggregateGroupByMV(int length, int[][] groupKeysArray,
GroupByResultHolder groupByResultHolder,
Map<ExpressionContext, BlockValSet> blockValSetMap) {
double[] values1 =
StatisticalAggregationFunctionUtils.getValSet(blockValSetMap, _expression1);
double[] values2 =
StatisticalAggregationFunctionUtils.getValSet(blockValSetMap, _expression2);
- for (int i = 0; i < length; i++) {
- for (int groupKey : groupKeysArray[i]) {
- setGroupByResult(groupKey, groupByResultHolder, values1[i],
values2[i], values1[i] * values2[i], 1L);
+ forEachNotNull(length, blockValSetMap.get(_expression1),
blockValSetMap.get(_expression2), (from, to) -> {
+ for (int i = from; i < to; i++) {
+ for (int groupKey : groupKeysArray[i]) {
+ setGroupByResult(groupKey, groupByResultHolder, values1[i],
values2[i], values1[i] * values2[i], 1L);
+ }
}
- }
+ });
}
+ @Nullable
@Override
public CovarianceTuple extractAggregationResult(AggregationResultHolder
aggregationResultHolder) {
- CovarianceTuple covarianceTuple = aggregationResultHolder.getResult();
- if (covarianceTuple == null) {
- return new CovarianceTuple(0.0, 0.0, 0.0, 0L);
- } else {
- return covarianceTuple;
- }
+ return aggregationResultHolder.getResult();
Review Comment:
This changes the disabled-mode wire value in a mixed-version cluster.
Previously an untouched new-server holder produced a zero-count tuple, which an
old broker rendered as `-Infinity`; it will now receive `null`, and the old
`extractFinalResult(null)` returns `NULL`. That contradicts the stated “option
disabled, nothing changes” behavior and Pinot’s mixed-version requirement.
Preserve the zero-count tuple for disabled mode until old brokers are no longer
supported, while returning `null` only when null handling is enabled.
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/CovarianceAggregationFunction.java:
##########
@@ -54,11 +56,45 @@ public class CovarianceAggregationFunction implements
AggregationFunction<Covari
protected final ExpressionContext _expression1;
protected final ExpressionContext _expression2;
protected final boolean _isSample;
+ protected final boolean _nullHandlingEnabled;
- public CovarianceAggregationFunction(List<ExpressionContext> arguments,
boolean isSample) {
+ public CovarianceAggregationFunction(List<ExpressionContext> arguments,
boolean isSample,
+ boolean nullHandlingEnabled) {
_expression1 = arguments.get(0);
_expression2 = arguments.get(1);
_isSample = isSample;
+ _nullHandlingEnabled = nullHandlingEnabled;
+ }
+
+ /// Runs `consumer` over the row ranges where **both** input columns are
non-null.
+ ///
+ /// A covariance pairs two values per row, so a row contributes only when
neither is null; the ranges are therefore
+ /// taken from the union of the two null bitmaps. With null handling
disabled the whole block is consumed, matching
+ /// the single-input helper on [NullableSingleInputAggregationFunction].
+ private void forEachNotNull(int length, BlockValSet blockValSet1,
BlockValSet blockValSet2,
+ RoaringBitmapUtils.BatchConsumer consumer) {
+ if (!_nullHandlingEnabled) {
+ consumer.consume(0, length);
+ return;
+ }
+ RoaringBitmap nullBitmap1 = blockValSet1.getNullBitmap();
+ RoaringBitmap nullBitmap2 = blockValSet2.getNullBitmap();
+ RoaringBitmap nullBitmap;
+ if (nullBitmap1 == null) {
+ nullBitmap = nullBitmap2;
+ } else if (nullBitmap2 == null) {
+ nullBitmap = nullBitmap1;
+ } else {
+ // a new bitmap, so neither block's own bitmap is mutated
+ nullBitmap = RoaringBitmap.or(nullBitmap1, nullBitmap2);
Review Comment:
`RoaringBitmap.or` materializes and copies the complete union for every
scanned value block whenever both columns contain nulls. This is on the
aggregation/group-by hot path and can add substantial allocation and bitmap
traversal for nullable columns. Stream the ordered union into `forEachUnset`
instead (the existing `MinIntIterator` pattern already merges two null
iterators without materializing a bitmap), ideally after moving that iterator
helper to `RoaringBitmapUtils`.
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/AggregationFunctionFactory.java:
##########
@@ -458,9 +458,9 @@ public static AggregationFunction
getAggregationFunction(FunctionContext functio
case HISTOGRAM:
return new HistogramAggregationFunction(arguments);
case COVARPOP:
- return new CovarianceAggregationFunction(arguments, false);
+ return new CovarianceAggregationFunction(arguments, false,
nullHandlingEnabled);
case COVARSAMP:
- return new CovarianceAggregationFunction(arguments, true);
+ return new CovarianceAggregationFunction(arguments, true,
nullHandlingEnabled);
Review Comment:
This query-semantics change has only direct function/synthetic-block tests.
It does not verify that a real query propagates `enableNullHandling` through
the factory or that server-to-broker rendering carries the new nullable
intermediate correctly; the existing `StatisticalQueriesTest` covariance cases
use non-null data and default handling only. Add end-to-end aggregation and
group-by cases with nulls for both option modes, including an all-null result.
--
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]
