Jackie-Jiang commented on code in PR #19158:
URL: https://github.com/apache/pinot/pull/19158#discussion_r3739382596
##########
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:
Added the branch rather than listing them as deviations. `PERCENTILETDIGEST`
now returns `NULL` over an empty digest with the option on, matching
`PERCENTILE`, `PERCENTILEEST` and `PERCENTILEKLL`, and `PERCENTILETDIGESTMV`
inherits it. `PERCENTILESMARTTDIGEST` got the same test on its t-digest branch
so it agrees with its own value-list branch. Both are gated on the option, so
the disabled mode is unchanged.
##########
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:
Recorded. The paragraph now says `null` is not the only representation,
names the empty-accumulator forms — set, value list, digest, sketch, map — and
says why `extractFinalResult` tests for both. It also gives the reason testing
both is the rule rather than substituting everywhere: `MAXSTRING`, `MINSTRING`
and `ANYVALUE` have no empty value, so `null` is the only representation always
available.
##########
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:
Stated in the note, as you suggested. Deviation 3 now records that this
change widens it deliberately, that eleven functions used to throw on a `null`
intermediate result and now return `null` for it, and that a loud failure
became a silent one.
I derived the affected list independently rather than copying yours, and got
a wider one: it also includes `COVAR_POP`, `PERCENTILETDIGEST`, `IDSET` and
`SUMVALUESINTEGERTUPLESKETCH`. Fixing the writer is follow-up work.
##########
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:
Fixed exactly as written. `PERCENTILETDIGESTMV` inherits the method, so it
changes with it.
##########
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:
Fixed. The t-digest branch now tests `size() == 0` under the option, so both
branches answer `NULL` and the answer no longer depends on whether the
accumulator crossed the conversion threshold.
##########
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:
Pushing back on this one.
`mergeFinalResult(null, null)` is not reachable for this function. Its
`extractFinalResult` returns `0` for a `null` intermediate result, and its
return is not annotated `@Nullable`, so it never produces a `null` final result
and no `null` ever enters the merge.
That is the rule the contract in this PR adds: nullability is declared by
the annotation rather than discovered, and everything downstream may rely on
it. Restoring the branch would be dead code contradicting the declaration.
If you think the annotation is wrong — that it *can* return `null` — that is
a different and more interesting bug, and the fix belongs at
`extractFinalResult` rather than in the merge. Happy to look again if you have
a path in mind.
--
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]
