Jackie-Jiang commented on code in PR #19158:
URL: https://github.com/apache/pinot/pull/19158#discussion_r3725226334
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/CountAggregationFunction.java:
##########
@@ -86,12 +86,17 @@ public void aggregate(int length, AggregationResultHolder
aggregationResultHolde
if (blockValSetMap.isEmpty()) {
aggregationResultHolder.setValue(aggregationResultHolder.getDoubleResult() +
length);
} else if (blockValSetMap.containsKey(STAR_TREE_COUNT_STAR_EXPRESSION)) {
- // Star-tree pre-aggregated values
- long[] valueArray =
blockValSetMap.get(STAR_TREE_COUNT_STAR_EXPRESSION).getLongValuesSV();
- long count = 0;
- for (int i = 0; i < length; i++) {
- count += valueArray[i];
- }
+ // Star-tree pre-aggregated values. A null-aware star-tree marks the
groups that aggregated over no non-null
Review Comment:
Agreed, and looking into it properly the answer turned out to be stronger
than "not yet" for most of them — I have dropped all four star-tree changes.
For `COUNT` it is never needed: `__COUNT_STAR` is a row count, and a node
exists precisely because rows mapped to it. The same holds one level up for
`COUNT_MV`, which reads the pre-aggregated `COUNT` for the column — `COUNT` is
a counting function whose answer over no input is `0`, so a null-aware
star-tree would store `0`, never a null marker. `AVG` is also fine as-is,
because `AvgPair` already encodes emptiness in its own count field (`getCount()
!= 0` is on master today) and so needs no null vector.
`SUM_MV` is the only one where a null could genuinely arise later — a
primitive `double` sum cannot distinguish "sum of nothing" from `0` — but as
you say that is untestable until the star-tree side exists, so it belongs with
that work.
Codecov agrees with the outcome: those four accounted for 41 of the 53
uncovered lines in the patch.
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/AggregationFunction.java:
##########
@@ -36,6 +36,53 @@
/// 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 the empty
+/// multiset has no representation in this mode: its 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 the multiset of its **non-null** input
values, and separately defines a result for
+/// the empty multiset. This mode models those as two distinct things:
+/// - A `null` **intermediate result** means the empty multiset: 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 the empty multiset 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
+///
+/// The empty multiset is the identity of merging, and that carries no
per-function meaning, so it is resolved once by
Review Comment:
Fair point, and it prompted a wider fix than the one line — the term is gone
from the change entirely (44 occurrences across 30 files), replaced with the
plainer "nothing was aggregated" that the contract was already using in its own
definition. It is SQL vocabulary that is not otherwise used in Pinot, so it was
buying nothing.
On the substance of your question: the split is that the *representation* is
uniform across every implementation — a `null` intermediate result means
nothing was aggregated — which is what makes it correct to settle the merge
identity once in the caller, and that part does belong on the interface. What
is per-function is the *answer*, and the contract delegates that explicitly to
`extractFinalResult` (`0` for the counting functions, `null` for the value
functions).
The line you flagged is about merging, where there is no per-function
meaning at all, so it now names the concrete thing instead of the concept:
> 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.
--
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]
