Jackie-Jiang commented on code in PR #20098:
URL: https://github.com/apache/pinot/pull/20098#discussion_r4427713422
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/HistogramAggregationFunction.java:
##########
@@ -258,6 +258,14 @@ public double[] extractFinalResult(@Nullable
DoubleArrayList doubleArrayList) {
return ArrayListUtils.toDoubleArray(doubleArrayList);
}
+ @Override
+ public double[] mergeFinalResult(double[] finalResult1, double[]
finalResult2) {
+ for (int i = 0; i < finalResult1.length; i++) {
+ finalResult1[i] += finalResult2[i];
Review Comment:
Good catch, and thanks for the repro. `HISTOGRAM` and `SUMARRAY*` now return
a new array. So does the existing `FUNNELCOUNT` merge, which had the same
problem on master. The `mergeFinalResult` Javadoc now says that the merge must
not modify either final result, because a final result can be shared like any
other column value. That matches what `RowHeapDataBlock#copyObjectColumns`
assumes. `FinalResultMergeTest` and `FunnelFinalResultTest` now check that both
operands stay unchanged.
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/array/SumArrayLongAggregationFunction.java:
##########
@@ -177,4 +178,14 @@ public DataSchema.ColumnDataType
getFinalResultColumnType() {
public long[] extractFinalResult(@Nullable LongArrayList result) {
return result != null ? ArrayListUtils.toLongArray(result) : null;
}
+
+ @Override
+ public long[] mergeFinalResult(long[] finalResult1, long[] finalResult2) {
+ long[] mergedResult =
+ finalResult1.length >= finalResult2.length ? finalResult1 :
Arrays.copyOf(finalResult1, finalResult2.length);
Review Comment:
Fixed with that `Arrays.copyOf`, in both `SumArrayLongAggregationFunction`
and `SumArrayDoubleAggregationFunction`.
##########
pinot-core/src/test/java/org/apache/pinot/core/query/aggregation/function/DistinctCountULLAggregationFunctionTest.java:
##########
@@ -92,6 +94,19 @@ private static Map<ExpressionContext, BlockValSet> mvBlock()
{
/// Aggregating a multi-value column gives exactly what aggregating its
flattened values as a single-value column
/// gives. Comparing the two rather than asserting an estimate keeps this
exact for a sketch.
+ @Test
Review Comment:
Moved the test above that comment.
--
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]
