yashmayya commented on code in PR #19158:
URL: https://github.com/apache/pinot/pull/19158#discussion_r3732498977
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/AggregationFunction.java:
##########
@@ -36,6 +36,61 @@
/// The implementation should be stateless, and can be shared among multiple
segments in multiple threads. The result
/// for each segment should be stored and passed in via the result holder.
///
+/// ## Null contract
+///
+/// Null handling is a per-query flag, and the two modes place different
requirements on an implementation.
+///
+/// ### Null handling disabled
+///
+/// Null values are read as the column's default, so no input value is ever
null. An implementation keeps a primitive
+/// result holder, performs no null tracking, and needs no null check while
aggregating.
+///
+/// An untouched accumulator is indistinguishable from one that aggregated to
the type's identity, so this mode cannot
+/// tell "nothing was aggregated" apart from a real result: the answer is
whatever the accumulator's initial state
+/// renders to, `0` for `SUM` or `+Infinity` for `MIN`. Where the type has no
identity to render, the intermediate
+/// result is `null` instead: `MAXSTRING`, `MINSTRING` and `ANYVALUE` are
object-backed and have no empty value to
+/// return. So [#extractFinalResult] must accept `null` in this mode as well.
+///
+/// ### Null handling enabled
+///
+/// SQL evaluates an aggregate over its **non-null** input values only, and
separately defines what the result is when
+/// there are none. This mode models those as two distinct things:
+/// - A `null` **intermediate result** means nothing was aggregated, either
because no row matched or because every
+/// matching value was null. It carries no per-function meaning, which is
what makes it correct for the aggregation
+/// methods to skip null rows outright rather than fold them in.
+/// - [#extractFinalResult] decides what that means for this aggregation, and
is the only method that does. `COUNT`
+/// and the distinct counts return `0`; `SUM`, `MIN`, `MAX`, `AVG` and the
percentiles return `null`.
+///
+/// An implementation switches to a nullable result holder only in this mode,
which keeps the boxing cost on the opt-in
+/// path.
+///
+/// ### Both modes
+///
+/// A `null` operand is the identity of merging, and that carries no
per-function meaning, so it is resolved once by the
+/// caller rather than in every implementation: merge intermediate results
through [AggregationFunctionUtils#merge] and
+/// final results through [AggregationFunctionUtils#mergeFinalResult], which
settle `null` operands before delegating.
+/// [#merge] and [#mergeFinalResult] are therefore handed two real values and
must not be called with `null`.
+///
+/// TODO: Known deviations from the above.
+/// 1. Several aggregation methods do not skip null rows yet when null
handling is enabled, and still fold the
+/// column's default null value into the aggregate: the distinct-count
family, the tuple and frequency sketches,
+/// the statistical functions, the first/last-with-time functions, and
the funnel family.
+/// 2. The multi-stage engine constructs every aggregation function with
null handling enabled and never consults the
+/// query's null handling option, so a query that disables it still gets
enabled-mode semantics there. The two
+/// engines can therefore answer the same query differently: with null
handling disabled, `SUM` over a query
+/// whose segments are all pruned is `NULL` on the multi-stage engine and
`0` on the single-stage engine. This
+/// may be intended, the multi-stage engine being the SQL-conformant one,
but it means the mode described above
+/// is not actually per-query everywhere.
+/// 3. **With null handling disabled, every `null` that reaches the data
table is mis-serialized unless the column
+/// type is `OBJECT`.** This mode is not meant to produce nulls at all,
but the object-backed accumulators
+/// described above do, and each one is corrupted on the way out. The
writer falls back to the encoding
+/// reserved for `OBJECT` whatever the column type is; with null handling
enabled it instead writes a
+/// placeholder alongside a null bitmap, which is correct for every type.
The damage varies by type: an array
+/// column has the right width but reads back as an empty array,
`STRING`, `INT` and `FLOAT` have their
+/// narrower fixed-size slot overrun, and `LONG` and `DOUBLE` read back
as a value. Both the intermediate and
+/// the final result reach this path, the latter from more functions
because their reported types are narrower.
+/// Aggregate values need the placeholder and null bitmap that group-by
keys already use.
Review Comment:
This deviation is accurate. I traced it. `BaseDataTableBuilder.setNull`
writes two ints at the column offset, whatever the column type is. A `STRING`
column has a narrower fixed-size slot in V4, so the write overruns into the
next column.
**This PR makes the deviation newly reachable.**
`AggregationResultsBlock.getDataTable()` calls `setNull(i)` when
`serverReturnFinalResult` is true and null handling is disabled. These
functions now return `null` in the disabled mode, and their final result column
types are narrow:
- `PERCENTILERAWKLL`, `PERCENTILERAWKLLMV`, `IDSET`, `FREQUENTLONGSSKETCH`,
`FREQUENTSTRINGSSKETCH`: `STRING`, so the slot overruns
- `STUNION`: `BYTES`, so the value reads back as an empty array
On master each one threw an NPE on the server. Now the server writes a
corrupt data table and reports no error. A loud failure became a silent one,
which is the wrong direction for a refactor.
The path is narrow. It needs the opt-in option and zero aggregated rows. But
the result class is wrong results, not a crash.
Two options. Keep the failure loud on that branch, and throw a clear error
for a `null` on a non-`OBJECT` column. Or write the placeholder and the null
bitmap that the null-handling-enabled branch already writes. The second option
needs the bitmap plumbing in the disabled branch, because a bare placeholder
turns `NULL` into a value.
If you prefer to keep this out of scope, state in this note that the PR
makes the deviation reachable.
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/PercentileSmartTDigestAggregationFunction.java:
##########
@@ -330,8 +323,15 @@ public ColumnDataType getFinalResultColumnType() {
return ColumnDataType.DOUBLE;
}
+ @Nullable
@Override
- public Double extractFinalResult(Object intermediateResult) {
+ public Double extractFinalResult(@Nullable Object intermediateResult) {
+ // A null intermediate result means nothing was aggregated, and a
percentile of nothing is NULL. An empty value list
+ // is a different thing: it is what an untouched single-stage result
holder produces, and it keeps its historical
+ // sentinel below so that path is not silently changed.
+ if (intermediateResult == null) {
+ return null;
+ }
if (intermediateResult instanceof TDigest) {
Review Comment:
The two branches disagree for one query.
The value-list branch below returns `null` when the list is empty and the
option is on. This `TDigest` branch returns `NaN` for an empty digest.
Both branches hold the same state: nothing was aggregated. So the answer
depends on whether the accumulator crossed the threshold into `TDigest` mode.
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/AggregationFunction.java:
##########
@@ -36,6 +36,61 @@
/// The implementation should be stateless, and can be shared among multiple
segments in multiple threads. The result
/// for each segment should be stored and passed in via the result holder.
///
+/// ## Null contract
+///
+/// Null handling is a per-query flag, and the two modes place different
requirements on an implementation.
+///
+/// ### Null handling disabled
+///
+/// Null values are read as the column's default, so no input value is ever
null. An implementation keeps a primitive
+/// result holder, performs no null tracking, and needs no null check while
aggregating.
+///
+/// An untouched accumulator is indistinguishable from one that aggregated to
the type's identity, so this mode cannot
+/// tell "nothing was aggregated" apart from a real result: the answer is
whatever the accumulator's initial state
+/// renders to, `0` for `SUM` or `+Infinity` for `MIN`. Where the type has no
identity to render, the intermediate
+/// result is `null` instead: `MAXSTRING`, `MINSTRING` and `ANYVALUE` are
object-backed and have no empty value to
+/// return. So [#extractFinalResult] must accept `null` in this mode as well.
+///
+/// ### Null handling enabled
+///
+/// SQL evaluates an aggregate over its **non-null** input values only, and
separately defines what the result is when
+/// there are none. This mode models those as two distinct things:
+/// - A `null` **intermediate result** means nothing was aggregated, either
because no row matched or because every
Review Comment:
This paragraph reads as though `null` is the only representation of the
state. Five families use a non-null empty sentinel instead:
- `BaseDistinctAggregateAggregationFunction` returns `EMPTY_PLACEHOLDER`
- `PercentileAggregationFunction` returns an empty `DoubleArrayList`
- `PercentileEstAggregationFunction` returns an empty `QuantileDigest`
- `PercentileTDigestAggregationFunction` returns an empty `TDigest`
- `ModeAggregationFunction` returns an empty map
This is why `extractFinalResult` now carries two emptiness tests in those
classes. It is also why the change is safe: the new `null` branch is dead there
on the single-stage engine.
The deviation list covers null-row skipping only, so a reader takes the
`null` representation as universal. Record this second representation as well.
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/AggregationFunction.java:
##########
@@ -128,13 +211,25 @@ default IntermediateResult
deserializeIntermediateResult(CustomObject customObje
ColumnDataType getFinalResultColumnType();
/// Extracts the final result used in the broker response from the given
intermediate result.
+ ///
+ /// A `null` intermediate result means nothing was aggregated, and this
method decides what that means for this
+ /// particular aggregation. It is the only place where that per-function
answer is expressed, so it must never
+ /// propagate the `null` blindly: `COUNT` and the distinct counts return
`0`, while `SUM`, `MIN`, `MAX`, `AVG` and the
+ /// percentiles return `null`.
Review Comment:
Three percentile functions do not obey this rule.
`PERCENTILETDIGEST` and `PERCENTILETDIGESTMV`: `extractAggregationResult`
builds an empty digest in both modes. `extractFinalResult` has no `isEmpty() &&
_nullHandlingEnabled` branch. `TDigest.quantile()` returns `NaN` for an empty
digest. I ran this against t-digest 3.3 for both `MergingDigest` and
`AVLTreeDigest`, and both return `NaN`.
So `PERCENTILETDIGEST` over an all-null column with
`enableNullHandling=true` gives `NaN`. `PERCENTILE`, `PERCENTILEEST` and
`PERCENTILEKLL` give `NULL` for the same query.
`PERCENTILESMARTTDIGEST` has the same split inside one function. See my
separate comment there.
This is not new. But this PR is the one that writes the rule down, and it
already adds that branch to the sibling functions. Add the branch, or list
these functions under the known deviations.
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/DistinctCountSmartHLLPlusAggregationFunction.java:
##########
@@ -338,13 +331,7 @@ public Integer extractFinalResult(@Nullable Object
intermediateResult) {
}
@Override
- public Integer mergeFinalResult(@Nullable Integer finalResult1, @Nullable
Integer finalResult2) {
- if (finalResult1 == null) {
- return finalResult2 == null ? 0 : finalResult2;
- }
- if (finalResult2 == null) {
- return finalResult1;
- }
+ public Integer mergeFinalResult(Integer finalResult1, Integer finalResult2) {
Review Comment:
The old code returned `0` when both operands were `null`.
`AggregationFunctionUtils.mergeFinalResult` returns `null` for that case.
`DISTINCTCOUNTSMARTHLLPLUS` is a counting function. The identity in the
final-result domain is `0`, not `null`. The shared helper cannot know that, so
the centralization loses this one answer. This is a small hole in "settle the
identity once in the caller".
Restore the branch, or record the change in the description.
##########
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:
This method now splits the two states the same way
`PercentileAggregationFunction` and `ModeAggregationFunction` do. A `null`
gives `NULL`, and a zero count gives `DEFAULT_FINAL_RESULT`. Those two classes
carry a comment that explains the split. Add the same explanation here.
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/PercentileTDigestAggregationFunction.java:
##########
@@ -330,9 +331,11 @@ public ColumnDataType getFinalResultColumnType() {
return ColumnDataType.DOUBLE;
}
+ @Nullable
@Override
- public Double extractFinalResult(TDigest intermediateResult) {
- return intermediateResult.quantile(_percentile / 100.0);
+ public Double extractFinalResult(@Nullable TDigest intermediateResult) {
Review Comment:
This is the point where the contract breaks.
`extractAggregationResult` returns
`TDigestUtils.createMergingDigest(_compressionFactor)` for an untouched holder,
in both modes. The `null` branch here is therefore dead on the single-stage
engine. An empty digest arrives instead, and `quantile()` returns `NaN`.
The fix matches what this PR already does for
`PercentileEstAggregationFunction`:
```java
if (intermediateResult == null || (intermediateResult.size() == 0 &&
_nullHandlingEnabled)) {
return null;
}
```
`TDigest.size()` returns 0 for an empty digest. I ran this against t-digest
3.3.
`PERCENTILETDIGESTMV` inherits this method, so it changes too.
##########
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:
This `catch` drops a function with no record.
`testEverySkippedTypeIsAccountedFor` exists to close that same hole.
The skip is active today. `allNullBlock` gives a `Double` block to every
type except `PERCENTILEESTMV`. `PERCENTILEEST`, `PERCENTILERAWEST` and
`PERCENTILERAWESTMV` read longs, so they throw here and drop out. That is why
they are absent from `HONOURS_NULL_HANDLING` while `PERCENTILEESTMV` is present.
Two changes. Key the block type on the digest type, not on one multi-value
enum. Then collect the skipped types and assert them against an expected set,
as the sibling test does.
##########
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:
This test covers `SUM`, `MIN`, `MAX`, `AVG`, `MINMAXRANGE`, `VARPOP` and
`STDDEVPOP`. It covers no percentile.
The contract names the percentiles in the same sentence as these functions.
The percentiles are also the family that breaks the rule. Add them here.
One case is not enough on its own. `extractFinalResult(null)` returns `null`
for every percentile, so that case passes today. Add a second case that feeds
an **empty accumulator** rather than a `null`. That case fails for
`PERCENTILETDIGEST`, which returns `NaN`.
##########
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:
The comment above groups `PERCENTILERAWTDIGESTMV` and `PERCENTILERAWKLLMV`
with the variants that already took the option. Both hard-coded `false` on
master. This PR threads the option into them, so they belong in the group below.
`PERCENTILETDIGEST` and `PERCENTILETDIGESTMV` also pass this test for the
wrong reason. The two modes differ as `NaN` against `0.0`. Neither answer is
`NULL`, which is what the contract promises. The test records a difference
between the modes, not the value that the contract names.
--
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]
