Jackie-Jiang commented on code in PR #19158:
URL: https://github.com/apache/pinot/pull/19158#discussion_r3739383124
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/CovarianceAggregationFunction.java:
##########
@@ -193,8 +195,13 @@ public ColumnDataType getFinalResultColumnType() {
return ColumnDataType.DOUBLE;
}
+ @Nullable
@Override
- public Double extractFinalResult(CovarianceTuple covarianceTuple) {
+ public Double extractFinalResult(@Nullable CovarianceTuple covarianceTuple) {
+ // A null intermediate result means nothing was aggregated, and the
covariance of nothing is NULL
Review Comment:
Added, in the same form the two siblings use. It also records why this class
cannot collapse the two states: it never receives the query's null handling
option, so it cannot tell the modes apart and keeps its historical sentinel.
That turned out to be part of a wider inconsistency, now written up as a
fourth known deviation — which of the two extraction paths substitutes an empty
accumulator varies across implementations and sometimes within one. With null
handling disabled `VARPOP` answers `NULL` for a group with no rows while
`MINMAXRANGE` answers `-Infinity`, reachable through a filtered aggregation
since one group key space is shared across every aggregation in the query.
Conforming the family is follow-up: it changes what a server puts on the wire,
and the value to preserve is not always a constant — a raw percentile renders a
serialized empty digest.
##########
pinot-core/src/test/java/org/apache/pinot/core/query/aggregation/function/AggregationFunctionNullContractTest.java:
##########
@@ -0,0 +1,345 @@
+/**
+ * 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.core.query.aggregation.function;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Set;
+import java.util.TreeSet;
+import javax.annotation.Nullable;
+import org.apache.pinot.common.request.context.ExpressionContext;
+import org.apache.pinot.common.request.context.FunctionContext;
+import org.apache.pinot.common.request.context.RequestContextUtils;
+import org.apache.pinot.core.common.BlockValSet;
+import org.apache.pinot.core.common.SyntheticBlockValSets;
+import org.apache.pinot.core.query.aggregation.AggregationResultHolder;
+import org.apache.pinot.segment.spi.AggregationFunctionType;
+import org.roaringbitmap.RoaringBitmap;
+import org.testng.annotations.DataProvider;
+import org.testng.annotations.Test;
+
+import static org.testng.Assert.*;
+
+
+/// Enforces the null contract documented on [AggregationFunction] across
every aggregation function that can be
+/// constructed generically, so that a new function cannot quietly opt out of
it.
+///
+/// The case this guards is the one that has repeatedly reached production: a
query whose segments are all pruned
+/// produces no intermediate result at all, and the broker still has to render
a row for it. Both `EmptyResponseUtils`
+/// (via an untouched result holder) and the multi-stage executors (via an
uninitialized `Object[]` slot) hand that to
+/// [AggregationFunction#extractFinalResult], and a function that dereferences
the argument throws instead of returning
+/// the SQL answer for zero rows.
+@SuppressWarnings({"rawtypes", "unchecked"})
+public class AggregationFunctionNullContractTest {
+
+ private static final int NUM_DOCS = 10;
+
+ /// Argument shapes tried in order until the factory accepts one. Functions
needing an argument list that none of
+ /// these match are reported by [#testEverySkippedTypeIsAccountedFor] rather
than silently skipped.
+ private static final String[] ARGUMENT_SHAPES = {
+ "(column)", "(*)", "(column, 50)", "(column, column2)", "(column, 50,
100)", "(column, column2, column3)",
+ // arrayAgg(dataColumn, 'dataType')
+ "(column, 'LONG')",
+ // firstWithTime / lastWithTime(dataColumn, timeColumn, 'dataType')
+ "(column, column2, 'LONG')",
+ // histogram(column, lower, upper, numBins)
+ "(column, 0, 1000, 10)",
+ // the funnel family: (timestampColumn, windowMillis, numSteps,
stepPredicate...)
+ "(column, '1000', 2, column2 = 'a', column2 = 'b')",
+ // funnelStepDurationStats takes a trailing settings literal
+ "(column, '1000', 2, column2 = 'a', column2 = 'b',
'durationFunctions=count')",
+ // funnelEventsFunctionEval takes a trailing (numExtraFields,
extraColumns...) tail, where the count must match
+ // the number of columns that follow it
+ "(column, '1000', 2, column2 = 'a', column2 = 'b', 2, column, column2)",
+ // funnelCount uses named options rather than positional arguments
+ "(STEPS(column2 = 'a', column2 = 'b'), CORRELATE_BY(column))"
+ };
+
+ /// Types that are not user-facing aggregates, so "what does this return
when nothing was aggregated" is not a
+ /// meaningful question. Keep this list short and justified: anything added
here stops being checked.
+ ///
+ /// - `FOURTHMOMENT` is the shared accumulator behind `SKEWNESS` and
`KURTOSIS` and throws from `extractFinalResult`
+ /// by design. The same class is still covered through those two types.
+ /// - The parent/child `EXPRMIN` / `EXPRMAX` types are produced by the query
rewriter, not written by users. The
+ /// parent returns a nested data block rather than a scalar, and the child
always returns `0`.
+ private static final Set<AggregationFunctionType> INTERNAL_TYPES = Set.of(
+ AggregationFunctionType.FOURTHMOMENT,
+ AggregationFunctionType.PINOTPARENTAGGEXPRMIN,
+ AggregationFunctionType.PINOTPARENTAGGEXPRMAX,
+ AggregationFunctionType.PINOTCHILDAGGEXPRMIN,
+ AggregationFunctionType.PINOTCHILDAGGEXPRMAX
+ );
+
+ /// Types that cannot be constructed from a bare [FunctionContext] at all,
and so cannot be checked here.
+ ///
+ /// - `EXPRMIN` / `EXPRMAX` are rejected by the factory outright; they are
only legal in a selection without an alias,
+ /// and are rewritten into the parent/child pair above before execution.
+ /// - `TIMESERIESAGGREGATE` needs time-series plan context that a bare
function context cannot supply.
+ ///
+ /// [#testEverySkippedTypeIsAccountedFor] pins this exactly, in both
directions, so a newly added function cannot drop
+ /// out of the contract unnoticed.
+ private static final Set<AggregationFunctionType> EXPECTED_UNCONSTRUCTIBLE =
Set.of(
+ AggregationFunctionType.EXPRMIN,
+ AggregationFunctionType.EXPRMAX,
+ AggregationFunctionType.TIMESERIESAGGREGATE
+ );
+
+ @DataProvider(name = "aggregationFunctions")
+ public Object[][] aggregationFunctions() {
+ List<Object[]> cases = new ArrayList<>();
+ for (AggregationFunctionType type : AggregationFunctionType.values()) {
+ if (INTERNAL_TYPES.contains(type)) {
+ continue;
+ }
+ for (boolean nullHandlingEnabled : new boolean[]{false, true}) {
+ AggregationFunction function = tryCreate(type, nullHandlingEnabled);
+ if (function != null) {
+ cases.add(new Object[]{type, nullHandlingEnabled, function});
+ }
+ }
+ }
+ return cases.toArray(new Object[0][]);
+ }
+
+ /// An empty result holder must survive the whole extract path, which is
what an all-pruned query does.
+ @Test(dataProvider = "aggregationFunctions")
+ public void testEmptyHolderExtractsWithoutThrowing(AggregationFunctionType
type, boolean nullHandlingEnabled,
+ AggregationFunction function) {
+ Object intermediate;
+ try {
+ intermediate =
function.extractAggregationResult(function.createAggregationResultHolder());
+ } catch (Exception e) {
+ throw new AssertionError(
+ describe(type, nullHandlingEnabled) + ": extractAggregationResult
failed on an empty holder", e);
+ }
+ try {
+ render(function.extractFinalResult(intermediate));
+ } catch (Exception e) {
+ throw new AssertionError(
+ describe(type, nullHandlingEnabled) + ": extractFinalResult failed
on the empty intermediate result", e);
+ }
+ }
+
+ /// A `null` intermediate result means nothing was aggregated and must be
resolved, never propagated by dereference.
+ @Test(dataProvider = "aggregationFunctions")
+ public void testNullIntermediateResultIsResolved(AggregationFunctionType
type, boolean nullHandlingEnabled,
+ AggregationFunction function) {
+ try {
+ render(function.extractFinalResult(null));
+ } catch (Exception e) {
+ throw new AssertionError(
+ describe(type, nullHandlingEnabled) + ": extractFinalResult(null)
must return the answer for no input", e);
+ }
+ }
+
+ /// Forces the final result into the form the broker response needs.
+ ///
+ /// Returning is not enough to prove the `null` was resolved: a function can
wrap the `null` in a serializer that only
+ /// dereferences it when the value is rendered, which moves the failure out
of this method and into the response path.
+ private static void render(@Nullable Object finalResult) {
+ if (finalResult != null) {
+ finalResult.toString();
+ }
+ }
+
+ /// `null` is the identity of merging, settled once by the caller so no
implementation needs its own null branch.
+ ///
+ /// Both helpers resolve the `null` operand without delegating, so this
holds for every function, including those that
+ /// do not support merging final results at all.
+ @Test(dataProvider = "aggregationFunctions")
+ public void testNullIsTheMergeIdentity(AggregationFunctionType type, boolean
nullHandlingEnabled,
+ AggregationFunction function) {
+ String description = describe(type, nullHandlingEnabled);
+ Comparable value = "value";
+ assertSame(AggregationFunctionUtils.merge(function, null, value), value,
description);
+ assertSame(AggregationFunctionUtils.merge(function, value, null), value,
description);
+ assertNull(AggregationFunctionUtils.merge(function, null, null),
description);
+ assertSame(AggregationFunctionUtils.mergeFinalResult(function, null,
value), value, description);
+ assertSame(AggregationFunctionUtils.mergeFinalResult(function, value,
null), value, description);
+ assertNull(AggregationFunctionUtils.mergeFinalResult(function, null,
null), description);
+ }
+
+ /// The functions that SQL fixes at `0` rather than `NULL` when nothing was
aggregated.
+ @Test
+ public void testCountingFunctionsReturnZeroWhenNothingAggregated() {
+ assertEquals(create("COUNT", "(*)", true).extractFinalResult(null), 0L);
+ assertEquals(create("DISTINCTCOUNT", "(column)",
true).extractFinalResult(null), 0);
+ assertEquals(create("DISTINCTCOUNTHLL", "(column)",
true).extractFinalResult(null), 0L);
+ assertEquals(create("DISTINCTCOUNTBITMAP", "(column)",
true).extractFinalResult(null), 0);
+ }
+
+ /// The functions that return SQL `NULL` when nothing was aggregated.
+ @Test
+ public void testValueFunctionsReturnNullWhenNothingAggregated() {
Review Comment:
Both added. The case now runs over twelve types including all five
percentile families, and asserts `null` for **both** `extractFinalResult(null)`
and `extractFinalResult(<empty accumulator>)`. The second assertion is the one
that catches `PERCENTILETDIGEST` returning `NaN`, exactly as you predicted.
##########
pinot-core/src/test/java/org/apache/pinot/core/query/aggregation/function/AggregationFunctionNullContractTest.java:
##########
@@ -0,0 +1,345 @@
+/**
+ * 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.core.query.aggregation.function;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Set;
+import java.util.TreeSet;
+import javax.annotation.Nullable;
+import org.apache.pinot.common.request.context.ExpressionContext;
+import org.apache.pinot.common.request.context.FunctionContext;
+import org.apache.pinot.common.request.context.RequestContextUtils;
+import org.apache.pinot.core.common.BlockValSet;
+import org.apache.pinot.core.common.SyntheticBlockValSets;
+import org.apache.pinot.core.query.aggregation.AggregationResultHolder;
+import org.apache.pinot.segment.spi.AggregationFunctionType;
+import org.roaringbitmap.RoaringBitmap;
+import org.testng.annotations.DataProvider;
+import org.testng.annotations.Test;
+
+import static org.testng.Assert.*;
+
+
+/// Enforces the null contract documented on [AggregationFunction] across
every aggregation function that can be
+/// constructed generically, so that a new function cannot quietly opt out of
it.
+///
+/// The case this guards is the one that has repeatedly reached production: a
query whose segments are all pruned
+/// produces no intermediate result at all, and the broker still has to render
a row for it. Both `EmptyResponseUtils`
+/// (via an untouched result holder) and the multi-stage executors (via an
uninitialized `Object[]` slot) hand that to
+/// [AggregationFunction#extractFinalResult], and a function that dereferences
the argument throws instead of returning
+/// the SQL answer for zero rows.
+@SuppressWarnings({"rawtypes", "unchecked"})
+public class AggregationFunctionNullContractTest {
+
+ private static final int NUM_DOCS = 10;
+
+ /// Argument shapes tried in order until the factory accepts one. Functions
needing an argument list that none of
+ /// these match are reported by [#testEverySkippedTypeIsAccountedFor] rather
than silently skipped.
+ private static final String[] ARGUMENT_SHAPES = {
+ "(column)", "(*)", "(column, 50)", "(column, column2)", "(column, 50,
100)", "(column, column2, column3)",
+ // arrayAgg(dataColumn, 'dataType')
+ "(column, 'LONG')",
+ // firstWithTime / lastWithTime(dataColumn, timeColumn, 'dataType')
+ "(column, column2, 'LONG')",
+ // histogram(column, lower, upper, numBins)
+ "(column, 0, 1000, 10)",
+ // the funnel family: (timestampColumn, windowMillis, numSteps,
stepPredicate...)
+ "(column, '1000', 2, column2 = 'a', column2 = 'b')",
+ // funnelStepDurationStats takes a trailing settings literal
+ "(column, '1000', 2, column2 = 'a', column2 = 'b',
'durationFunctions=count')",
+ // funnelEventsFunctionEval takes a trailing (numExtraFields,
extraColumns...) tail, where the count must match
+ // the number of columns that follow it
+ "(column, '1000', 2, column2 = 'a', column2 = 'b', 2, column, column2)",
+ // funnelCount uses named options rather than positional arguments
+ "(STEPS(column2 = 'a', column2 = 'b'), CORRELATE_BY(column))"
+ };
+
+ /// Types that are not user-facing aggregates, so "what does this return
when nothing was aggregated" is not a
+ /// meaningful question. Keep this list short and justified: anything added
here stops being checked.
+ ///
+ /// - `FOURTHMOMENT` is the shared accumulator behind `SKEWNESS` and
`KURTOSIS` and throws from `extractFinalResult`
+ /// by design. The same class is still covered through those two types.
+ /// - The parent/child `EXPRMIN` / `EXPRMAX` types are produced by the query
rewriter, not written by users. The
+ /// parent returns a nested data block rather than a scalar, and the child
always returns `0`.
+ private static final Set<AggregationFunctionType> INTERNAL_TYPES = Set.of(
+ AggregationFunctionType.FOURTHMOMENT,
+ AggregationFunctionType.PINOTPARENTAGGEXPRMIN,
+ AggregationFunctionType.PINOTPARENTAGGEXPRMAX,
+ AggregationFunctionType.PINOTCHILDAGGEXPRMIN,
+ AggregationFunctionType.PINOTCHILDAGGEXPRMAX
+ );
+
+ /// Types that cannot be constructed from a bare [FunctionContext] at all,
and so cannot be checked here.
+ ///
+ /// - `EXPRMIN` / `EXPRMAX` are rejected by the factory outright; they are
only legal in a selection without an alias,
+ /// and are rewritten into the parent/child pair above before execution.
+ /// - `TIMESERIESAGGREGATE` needs time-series plan context that a bare
function context cannot supply.
+ ///
+ /// [#testEverySkippedTypeIsAccountedFor] pins this exactly, in both
directions, so a newly added function cannot drop
+ /// out of the contract unnoticed.
+ private static final Set<AggregationFunctionType> EXPECTED_UNCONSTRUCTIBLE =
Set.of(
+ AggregationFunctionType.EXPRMIN,
+ AggregationFunctionType.EXPRMAX,
+ AggregationFunctionType.TIMESERIESAGGREGATE
+ );
+
+ @DataProvider(name = "aggregationFunctions")
+ public Object[][] aggregationFunctions() {
+ List<Object[]> cases = new ArrayList<>();
+ for (AggregationFunctionType type : AggregationFunctionType.values()) {
+ if (INTERNAL_TYPES.contains(type)) {
+ continue;
+ }
+ for (boolean nullHandlingEnabled : new boolean[]{false, true}) {
+ AggregationFunction function = tryCreate(type, nullHandlingEnabled);
+ if (function != null) {
+ cases.add(new Object[]{type, nullHandlingEnabled, function});
+ }
+ }
+ }
+ return cases.toArray(new Object[0][]);
+ }
+
+ /// An empty result holder must survive the whole extract path, which is
what an all-pruned query does.
+ @Test(dataProvider = "aggregationFunctions")
+ public void testEmptyHolderExtractsWithoutThrowing(AggregationFunctionType
type, boolean nullHandlingEnabled,
+ AggregationFunction function) {
+ Object intermediate;
+ try {
+ intermediate =
function.extractAggregationResult(function.createAggregationResultHolder());
+ } catch (Exception e) {
+ throw new AssertionError(
+ describe(type, nullHandlingEnabled) + ": extractAggregationResult
failed on an empty holder", e);
+ }
+ try {
+ render(function.extractFinalResult(intermediate));
+ } catch (Exception e) {
+ throw new AssertionError(
+ describe(type, nullHandlingEnabled) + ": extractFinalResult failed
on the empty intermediate result", e);
+ }
+ }
+
+ /// A `null` intermediate result means nothing was aggregated and must be
resolved, never propagated by dereference.
+ @Test(dataProvider = "aggregationFunctions")
+ public void testNullIntermediateResultIsResolved(AggregationFunctionType
type, boolean nullHandlingEnabled,
+ AggregationFunction function) {
+ try {
+ render(function.extractFinalResult(null));
+ } catch (Exception e) {
+ throw new AssertionError(
+ describe(type, nullHandlingEnabled) + ": extractFinalResult(null)
must return the answer for no input", e);
+ }
+ }
+
+ /// Forces the final result into the form the broker response needs.
+ ///
+ /// Returning is not enough to prove the `null` was resolved: a function can
wrap the `null` in a serializer that only
+ /// dereferences it when the value is rendered, which moves the failure out
of this method and into the response path.
+ private static void render(@Nullable Object finalResult) {
+ if (finalResult != null) {
+ finalResult.toString();
+ }
+ }
+
+ /// `null` is the identity of merging, settled once by the caller so no
implementation needs its own null branch.
+ ///
+ /// Both helpers resolve the `null` operand without delegating, so this
holds for every function, including those that
+ /// do not support merging final results at all.
+ @Test(dataProvider = "aggregationFunctions")
+ public void testNullIsTheMergeIdentity(AggregationFunctionType type, boolean
nullHandlingEnabled,
+ AggregationFunction function) {
+ String description = describe(type, nullHandlingEnabled);
+ Comparable value = "value";
+ assertSame(AggregationFunctionUtils.merge(function, null, value), value,
description);
+ assertSame(AggregationFunctionUtils.merge(function, value, null), value,
description);
+ assertNull(AggregationFunctionUtils.merge(function, null, null),
description);
+ assertSame(AggregationFunctionUtils.mergeFinalResult(function, null,
value), value, description);
+ assertSame(AggregationFunctionUtils.mergeFinalResult(function, value,
null), value, description);
+ assertNull(AggregationFunctionUtils.mergeFinalResult(function, null,
null), description);
+ }
+
+ /// The functions that SQL fixes at `0` rather than `NULL` when nothing was
aggregated.
+ @Test
+ public void testCountingFunctionsReturnZeroWhenNothingAggregated() {
+ assertEquals(create("COUNT", "(*)", true).extractFinalResult(null), 0L);
+ assertEquals(create("DISTINCTCOUNT", "(column)",
true).extractFinalResult(null), 0);
+ assertEquals(create("DISTINCTCOUNTHLL", "(column)",
true).extractFinalResult(null), 0L);
+ assertEquals(create("DISTINCTCOUNTBITMAP", "(column)",
true).extractFinalResult(null), 0);
+ }
+
+ /// The functions that return SQL `NULL` when nothing was aggregated.
+ @Test
+ public void testValueFunctionsReturnNullWhenNothingAggregated() {
+ for (String name : new String[]{"SUM", "MIN", "MAX", "AVG", "MINMAXRANGE",
"VARPOP", "STDDEVPOP"}) {
+ AggregationFunction function = create(name, "(column)", true);
+ assertNull(function.extractFinalResult(null), name + " over no
aggregated values must be NULL");
+ }
+ }
+
+ /// Pins exactly which functions answer differently once null handling is
enabled.
+ ///
+ /// Aggregating a block whose rows are all null separates the two modes:
enabled skips every row and reports
+ /// nothing aggregated, disabled reads each one as the column default and
aggregates it. A function shows up here
+ /// only if it both has null-aware aggregation and receives the query's
option, so the set is a direct read-out of
+ /// which functions the option actually reaches.
+ ///
+ /// The multi-value entries are the interesting ones: they used to hard-code
the option off, so it had no effect on
+ /// them whatever the query asked for.
+ private static final Set<AggregationFunctionType> HONOURS_NULL_HANDLING =
Set.of(
+ // single-value functions, which have always taken the option
+ AggregationFunctionType.COUNT, AggregationFunctionType.MIN,
AggregationFunctionType.MAX,
+ AggregationFunctionType.SUM, AggregationFunctionType.SUM0,
AggregationFunctionType.AVG,
+ AggregationFunctionType.MODE, AggregationFunctionType.ANYVALUE,
AggregationFunctionType.MINMAXRANGE,
+ AggregationFunctionType.DISTINCTCOUNT,
AggregationFunctionType.DISTINCTCOUNTOFFHEAP,
+ AggregationFunctionType.DISTINCTSUM, AggregationFunctionType.DISTINCTAVG,
+ AggregationFunctionType.DISTINCTCOUNTRAWHLL,
AggregationFunctionType.DISTINCTCOUNTRAWHLLPLUS,
+ AggregationFunctionType.DISTINCTCOUNTRAWULL,
AggregationFunctionType.PERCENTILE,
+ AggregationFunctionType.PERCENTILETDIGEST,
AggregationFunctionType.PERCENTILERAWTDIGEST,
+ AggregationFunctionType.PERCENTILESMARTTDIGEST,
AggregationFunctionType.PERCENTILEKLL,
+ AggregationFunctionType.PERCENTILERAWKLL,
AggregationFunctionType.VARPOP, AggregationFunctionType.VARSAMP,
+ AggregationFunctionType.STDDEVPOP, AggregationFunctionType.STDDEVSAMP,
+ // multi-value variants that already took the option
+ AggregationFunctionType.MINMV, AggregationFunctionType.MAXMV,
AggregationFunctionType.SUMMV,
+ AggregationFunctionType.AVGMV, AggregationFunctionType.MINMAXRANGEMV,
+ AggregationFunctionType.DISTINCTCOUNTRAWHLLMV,
AggregationFunctionType.DISTINCTCOUNTRAWHLLPLUSMV,
+ AggregationFunctionType.PERCENTILERAWTDIGESTMV,
AggregationFunctionType.PERCENTILERAWKLLMV,
Review Comment:
Regrouped. Chasing it turned up more than the two: the raw MV variants reach
the option through the function they extend or wrap, so `PERCENTILERAWESTMV`
belongs in that group as well, and this change moves **ten** functions rather
than seven. The description now says ten.
On `PERCENTILETDIGEST` passing for the wrong reason — fixed by the `NaN`
change above, so the test now records `NULL` against a value rather than `NaN`
against `0.0`.
##########
pinot-core/src/test/java/org/apache/pinot/core/query/aggregation/function/AggregationFunctionNullContractTest.java:
##########
@@ -0,0 +1,345 @@
+/**
+ * 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.core.query.aggregation.function;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Set;
+import java.util.TreeSet;
+import javax.annotation.Nullable;
+import org.apache.pinot.common.request.context.ExpressionContext;
+import org.apache.pinot.common.request.context.FunctionContext;
+import org.apache.pinot.common.request.context.RequestContextUtils;
+import org.apache.pinot.core.common.BlockValSet;
+import org.apache.pinot.core.common.SyntheticBlockValSets;
+import org.apache.pinot.core.query.aggregation.AggregationResultHolder;
+import org.apache.pinot.segment.spi.AggregationFunctionType;
+import org.roaringbitmap.RoaringBitmap;
+import org.testng.annotations.DataProvider;
+import org.testng.annotations.Test;
+
+import static org.testng.Assert.*;
+
+
+/// Enforces the null contract documented on [AggregationFunction] across
every aggregation function that can be
+/// constructed generically, so that a new function cannot quietly opt out of
it.
+///
+/// The case this guards is the one that has repeatedly reached production: a
query whose segments are all pruned
+/// produces no intermediate result at all, and the broker still has to render
a row for it. Both `EmptyResponseUtils`
+/// (via an untouched result holder) and the multi-stage executors (via an
uninitialized `Object[]` slot) hand that to
+/// [AggregationFunction#extractFinalResult], and a function that dereferences
the argument throws instead of returning
+/// the SQL answer for zero rows.
+@SuppressWarnings({"rawtypes", "unchecked"})
+public class AggregationFunctionNullContractTest {
+
+ private static final int NUM_DOCS = 10;
+
+ /// Argument shapes tried in order until the factory accepts one. Functions
needing an argument list that none of
+ /// these match are reported by [#testEverySkippedTypeIsAccountedFor] rather
than silently skipped.
+ private static final String[] ARGUMENT_SHAPES = {
+ "(column)", "(*)", "(column, 50)", "(column, column2)", "(column, 50,
100)", "(column, column2, column3)",
+ // arrayAgg(dataColumn, 'dataType')
+ "(column, 'LONG')",
+ // firstWithTime / lastWithTime(dataColumn, timeColumn, 'dataType')
+ "(column, column2, 'LONG')",
+ // histogram(column, lower, upper, numBins)
+ "(column, 0, 1000, 10)",
+ // the funnel family: (timestampColumn, windowMillis, numSteps,
stepPredicate...)
+ "(column, '1000', 2, column2 = 'a', column2 = 'b')",
+ // funnelStepDurationStats takes a trailing settings literal
+ "(column, '1000', 2, column2 = 'a', column2 = 'b',
'durationFunctions=count')",
+ // funnelEventsFunctionEval takes a trailing (numExtraFields,
extraColumns...) tail, where the count must match
+ // the number of columns that follow it
+ "(column, '1000', 2, column2 = 'a', column2 = 'b', 2, column, column2)",
+ // funnelCount uses named options rather than positional arguments
+ "(STEPS(column2 = 'a', column2 = 'b'), CORRELATE_BY(column))"
+ };
+
+ /// Types that are not user-facing aggregates, so "what does this return
when nothing was aggregated" is not a
+ /// meaningful question. Keep this list short and justified: anything added
here stops being checked.
+ ///
+ /// - `FOURTHMOMENT` is the shared accumulator behind `SKEWNESS` and
`KURTOSIS` and throws from `extractFinalResult`
+ /// by design. The same class is still covered through those two types.
+ /// - The parent/child `EXPRMIN` / `EXPRMAX` types are produced by the query
rewriter, not written by users. The
+ /// parent returns a nested data block rather than a scalar, and the child
always returns `0`.
+ private static final Set<AggregationFunctionType> INTERNAL_TYPES = Set.of(
+ AggregationFunctionType.FOURTHMOMENT,
+ AggregationFunctionType.PINOTPARENTAGGEXPRMIN,
+ AggregationFunctionType.PINOTPARENTAGGEXPRMAX,
+ AggregationFunctionType.PINOTCHILDAGGEXPRMIN,
+ AggregationFunctionType.PINOTCHILDAGGEXPRMAX
+ );
+
+ /// Types that cannot be constructed from a bare [FunctionContext] at all,
and so cannot be checked here.
+ ///
+ /// - `EXPRMIN` / `EXPRMAX` are rejected by the factory outright; they are
only legal in a selection without an alias,
+ /// and are rewritten into the parent/child pair above before execution.
+ /// - `TIMESERIESAGGREGATE` needs time-series plan context that a bare
function context cannot supply.
+ ///
+ /// [#testEverySkippedTypeIsAccountedFor] pins this exactly, in both
directions, so a newly added function cannot drop
+ /// out of the contract unnoticed.
+ private static final Set<AggregationFunctionType> EXPECTED_UNCONSTRUCTIBLE =
Set.of(
+ AggregationFunctionType.EXPRMIN,
+ AggregationFunctionType.EXPRMAX,
+ AggregationFunctionType.TIMESERIESAGGREGATE
+ );
+
+ @DataProvider(name = "aggregationFunctions")
+ public Object[][] aggregationFunctions() {
+ List<Object[]> cases = new ArrayList<>();
+ for (AggregationFunctionType type : AggregationFunctionType.values()) {
+ if (INTERNAL_TYPES.contains(type)) {
+ continue;
+ }
+ for (boolean nullHandlingEnabled : new boolean[]{false, true}) {
+ AggregationFunction function = tryCreate(type, nullHandlingEnabled);
+ if (function != null) {
+ cases.add(new Object[]{type, nullHandlingEnabled, function});
+ }
+ }
+ }
+ return cases.toArray(new Object[0][]);
+ }
+
+ /// An empty result holder must survive the whole extract path, which is
what an all-pruned query does.
+ @Test(dataProvider = "aggregationFunctions")
+ public void testEmptyHolderExtractsWithoutThrowing(AggregationFunctionType
type, boolean nullHandlingEnabled,
+ AggregationFunction function) {
+ Object intermediate;
+ try {
+ intermediate =
function.extractAggregationResult(function.createAggregationResultHolder());
+ } catch (Exception e) {
+ throw new AssertionError(
+ describe(type, nullHandlingEnabled) + ": extractAggregationResult
failed on an empty holder", e);
+ }
+ try {
+ render(function.extractFinalResult(intermediate));
+ } catch (Exception e) {
+ throw new AssertionError(
+ describe(type, nullHandlingEnabled) + ": extractFinalResult failed
on the empty intermediate result", e);
+ }
+ }
+
+ /// A `null` intermediate result means nothing was aggregated and must be
resolved, never propagated by dereference.
+ @Test(dataProvider = "aggregationFunctions")
+ public void testNullIntermediateResultIsResolved(AggregationFunctionType
type, boolean nullHandlingEnabled,
+ AggregationFunction function) {
+ try {
+ render(function.extractFinalResult(null));
+ } catch (Exception e) {
+ throw new AssertionError(
+ describe(type, nullHandlingEnabled) + ": extractFinalResult(null)
must return the answer for no input", e);
+ }
+ }
+
+ /// Forces the final result into the form the broker response needs.
+ ///
+ /// Returning is not enough to prove the `null` was resolved: a function can
wrap the `null` in a serializer that only
+ /// dereferences it when the value is rendered, which moves the failure out
of this method and into the response path.
+ private static void render(@Nullable Object finalResult) {
+ if (finalResult != null) {
+ finalResult.toString();
+ }
+ }
+
+ /// `null` is the identity of merging, settled once by the caller so no
implementation needs its own null branch.
+ ///
+ /// Both helpers resolve the `null` operand without delegating, so this
holds for every function, including those that
+ /// do not support merging final results at all.
+ @Test(dataProvider = "aggregationFunctions")
+ public void testNullIsTheMergeIdentity(AggregationFunctionType type, boolean
nullHandlingEnabled,
+ AggregationFunction function) {
+ String description = describe(type, nullHandlingEnabled);
+ Comparable value = "value";
+ assertSame(AggregationFunctionUtils.merge(function, null, value), value,
description);
+ assertSame(AggregationFunctionUtils.merge(function, value, null), value,
description);
+ assertNull(AggregationFunctionUtils.merge(function, null, null),
description);
+ assertSame(AggregationFunctionUtils.mergeFinalResult(function, null,
value), value, description);
+ assertSame(AggregationFunctionUtils.mergeFinalResult(function, value,
null), value, description);
+ assertNull(AggregationFunctionUtils.mergeFinalResult(function, null,
null), description);
+ }
+
+ /// The functions that SQL fixes at `0` rather than `NULL` when nothing was
aggregated.
+ @Test
+ public void testCountingFunctionsReturnZeroWhenNothingAggregated() {
+ assertEquals(create("COUNT", "(*)", true).extractFinalResult(null), 0L);
+ assertEquals(create("DISTINCTCOUNT", "(column)",
true).extractFinalResult(null), 0);
+ assertEquals(create("DISTINCTCOUNTHLL", "(column)",
true).extractFinalResult(null), 0L);
+ assertEquals(create("DISTINCTCOUNTBITMAP", "(column)",
true).extractFinalResult(null), 0);
+ }
+
+ /// The functions that return SQL `NULL` when nothing was aggregated.
+ @Test
+ public void testValueFunctionsReturnNullWhenNothingAggregated() {
+ for (String name : new String[]{"SUM", "MIN", "MAX", "AVG", "MINMAXRANGE",
"VARPOP", "STDDEVPOP"}) {
+ AggregationFunction function = create(name, "(column)", true);
+ assertNull(function.extractFinalResult(null), name + " over no
aggregated values must be NULL");
+ }
+ }
+
+ /// Pins exactly which functions answer differently once null handling is
enabled.
+ ///
+ /// Aggregating a block whose rows are all null separates the two modes:
enabled skips every row and reports
+ /// nothing aggregated, disabled reads each one as the column default and
aggregates it. A function shows up here
+ /// only if it both has null-aware aggregation and receives the query's
option, so the set is a direct read-out of
+ /// which functions the option actually reaches.
+ ///
+ /// The multi-value entries are the interesting ones: they used to hard-code
the option off, so it had no effect on
+ /// them whatever the query asked for.
+ private static final Set<AggregationFunctionType> HONOURS_NULL_HANDLING =
Set.of(
+ // single-value functions, which have always taken the option
+ AggregationFunctionType.COUNT, AggregationFunctionType.MIN,
AggregationFunctionType.MAX,
+ AggregationFunctionType.SUM, AggregationFunctionType.SUM0,
AggregationFunctionType.AVG,
+ AggregationFunctionType.MODE, AggregationFunctionType.ANYVALUE,
AggregationFunctionType.MINMAXRANGE,
+ AggregationFunctionType.DISTINCTCOUNT,
AggregationFunctionType.DISTINCTCOUNTOFFHEAP,
+ AggregationFunctionType.DISTINCTSUM, AggregationFunctionType.DISTINCTAVG,
+ AggregationFunctionType.DISTINCTCOUNTRAWHLL,
AggregationFunctionType.DISTINCTCOUNTRAWHLLPLUS,
+ AggregationFunctionType.DISTINCTCOUNTRAWULL,
AggregationFunctionType.PERCENTILE,
+ AggregationFunctionType.PERCENTILETDIGEST,
AggregationFunctionType.PERCENTILERAWTDIGEST,
+ AggregationFunctionType.PERCENTILESMARTTDIGEST,
AggregationFunctionType.PERCENTILEKLL,
+ AggregationFunctionType.PERCENTILERAWKLL,
AggregationFunctionType.VARPOP, AggregationFunctionType.VARSAMP,
+ AggregationFunctionType.STDDEVPOP, AggregationFunctionType.STDDEVSAMP,
+ // multi-value variants that already took the option
+ AggregationFunctionType.MINMV, AggregationFunctionType.MAXMV,
AggregationFunctionType.SUMMV,
+ AggregationFunctionType.AVGMV, AggregationFunctionType.MINMAXRANGEMV,
+ AggregationFunctionType.DISTINCTCOUNTRAWHLLMV,
AggregationFunctionType.DISTINCTCOUNTRAWHLLPLUSMV,
+ AggregationFunctionType.PERCENTILERAWTDIGESTMV,
AggregationFunctionType.PERCENTILERAWKLLMV,
+ // multi-value variants this change threads the option into; they
hard-coded it off before
+ AggregationFunctionType.DISTINCTCOUNTMV,
AggregationFunctionType.DISTINCTSUMMV,
+ AggregationFunctionType.DISTINCTAVGMV,
AggregationFunctionType.PERCENTILEMV,
+ AggregationFunctionType.PERCENTILEESTMV,
AggregationFunctionType.PERCENTILEKLLMV,
+ AggregationFunctionType.PERCENTILETDIGESTMV
+ );
+
+ /// Checks every function rather than a fixed list, so that a function
gaining or losing null awareness is caught.
+ ///
+ /// Failing with something extra means a function started honouring the
option and should be added to
+ /// [#HONOURS_NULL_HANDLING]; failing with something missing means one
stopped, which is a regression unless the
+ /// entry is stale. Functions the synthetic block cannot feed — because they
read a value type it does not
+ /// implement, or reject the column outright — are not counted either way.
+ @Test
+ public void testNullHandlingOptionReachesEveryFunctionThatHonoursIt() {
+ Set<AggregationFunctionType> honours = new TreeSet<>();
+ for (AggregationFunctionType type : AggregationFunctionType.values()) {
+ if (INTERNAL_TYPES.contains(type) || tryCreate(type, false) == null) {
+ continue;
+ }
+ Object enabled;
+ Object disabled;
+ try {
+ enabled = aggregateAllNulls(type, true);
+ disabled = aggregateAllNulls(type, false);
+ } catch (RuntimeException e) {
Review Comment:
Both changes made. The block type is keyed on a `READS_LONGS` set rather
than on one multi-value enum, which brought `PERCENTILEEST`, `PERCENTILERAWEST`
and `PERCENTILERAWESTMV` back into the check — and all three do honour the
option, so they joined `HONOURS_NULL_HANDLING`. Your diagnosis of why they were
absent was exactly right.
The skipped types are now collected and asserted against a pinned set in
both directions, like the sibling test. Both assertions name the differing
types on failure, so a drop-out reports itself.
--
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]
