yashmayya commented on code in PR #20098:
URL: https://github.com/apache/pinot/pull/20098#discussion_r4418600262
##########
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:
Nit: the `///` comment above belongs to
`testMVColumnMatchesFlattenedSVColumn`. Please move this new test above that
comment.
##########
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:
**Blocker:** this merge writes into `finalResult1`. With `useSpools=true`,
MSE can send one leaf stage to two final aggregate stages. `BroadcastExchange`
then gives the same block to both, by reference. It copies only `OBJECT` cells,
because `RowHeapDataBlock#copyObjectColumns()` assumes that downstream
operators do not change other cells. So both final stages keep the same
`double[]`, and both add into it.
Repro (2 servers, 16 rows, data partitioned on `num`). The result is wrong
in 30 of 30 runs:
```sql
SET useSpools=true;
SELECT name, h, 1 AS tag FROM (SELECT /*+
aggOptions(is_leaf_return_final_result='true') */ name, HISTOGRAM(num, 0, 16,
4) AS h FROM t GROUP BY name)
UNION ALL
SELECT name, h, 2 AS tag FROM (SELECT /*+
aggOptions(is_leaf_return_final_result='true') */ name, HISTOGRAM(num, 0, 16,
4) AS h FROM t GROUP BY name)
```
For `a`, the correct result is `[1, 2, 2, 3]`. The query returns `[1, 3, 3,
5]`, because the histogram of one server is added twice. Without spools, or
without the hint, the result is correct. On master, this query fails with
"Cannot merge final results". So this PR replaces an error with a silent wrong
result.
Fix: return a new array, for example `double[] mergedResult =
finalResult1.clone();`. I made this change here and in `SumArray*` (see there).
Then the repro is correct in 30 of 30 runs, and the other probes still pass.
The `mergeFinalResult` Javadoc also says that the result can be
`finalResult1`, modified in place. That is not safe for array final results in
MSE. Please update that sentence, or make the MSE executors copy the first
value that they keep.
##########
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:
Same problem as in `HISTOGRAM` (see that comment). With spools, `[72, 144,
72]` becomes `[112, 224, 72]`. This line fixes it:
```java
long[] mergedResult = Arrays.copyOf(finalResult1,
Math.max(finalResult1.length, finalResult2.length));
```
`SumArrayDoubleAggregationFunction` has the same code (`[36.0]` becomes
`[56.0]`).
--
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]
