xiangfu0 commented on code in PR #20074: URL: https://github.com/apache/pinot/pull/20074#discussion_r4409294361
########## pinot-spi/src/main/java/org/apache/pinot/spi/utils/StoredValueComparator.java: ########## @@ -0,0 +1,59 @@ +/** + * 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.spi.utils; + +import java.util.Arrays; +import java.util.Comparator; + + +/// Compares two non-null values of the same type in their stored form (e.g. `int` for `BOOLEAN`, [ByteArray] for +/// `BYTES`, `int[]` for `INT_ARRAY`), in the order of that type: +/// - A single value is ordered by its natural order. +/// - An array is ordered element by element, and a proper prefix comes first. +/// +/// The ordering depends only on the type, not on what produced the value, so every operator that sorts on a column +/// agrees on it. The comparator is stateless and thread-safe. +public final class StoredValueComparator implements Comparator<Object> { + public static final StoredValueComparator INSTANCE = new StoredValueComparator(); + + private StoredValueComparator() { + } + + @Override + public int compare(Object value1, Object value2) { Review Comment: Minor (testing): this comparator now orders values in both SSE `TableResizer` and MSE `SortUtils`, but nothing tests it directly. Codecov shows about 17% patch coverage, with only the `double[]` path hit, through the histogram trim test. I checked locally that it matches the old fastutil `compareTo` (1M random comparisons, 0 mismatches). A small `StoredValueComparatorTest` covering prefix ordering, NaN and -0.0 for float/double, and the `Comparable[]` branch would lock that behavior in. ########## pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/MultistageAggregationExecutor.java: ########## @@ -182,8 +182,7 @@ private void processMergeFinalResults(MseBlock.Data block) { Object[] finalResults = AggregateOperator.getIntermediateResults(aggFunction, block); for (Object finalResult : finalResults) { _mergeResultHolder[i] = - AggregationFunctionUtils.mergeFinalResult(aggFunction, (Comparable) _mergeResultHolder[i], - (Comparable) finalResult); + AggregationFunctionUtils.mergeFinalResult(aggFunction, _mergeResultHolder[i], finalResult); Review Comment: FYI: an aggregate with no group-by and `is_leaf_return_final_result` still fails when more than one leaf worker returns `ARRAYAGG`, `HISTOGRAM` or `SUMARRAY*`. These functions have no `mergeFinalResult`, so the query fails with `Cannot merge final results for function: ARRAYAGG`. On base it failed earlier with a CCE, so this isn't a regression, and the partitioned group-by case works. It might be worth stating in the PR description that this path is still unsupported for these functions. -- 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]
