Jackie-Jiang commented on code in PR #20074: URL: https://github.com/apache/pinot/pull/20074#discussion_r4418064838
########## 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: Added `StoredValueComparatorTest`. It covers the `Comparable` branch, prefix ordering and empty arrays, `-0.0` / `NaN` / `±Inf` for `float[]` and `double[]`, and the `Comparable[]` branch for `BigDecimal[]`, `String[]` and `ByteArray[]`. ########## 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: Thanks, I noted it under "Follow-up" in the description. Their final results are not more compact than their intermediate results, so returning final results gains little for these functions. A separate PR will add `mergeFinalResult` to them and to the other functions whose final results can be merged. -- 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]
