yashmayya commented on code in PR #19158:
URL: https://github.com/apache/pinot/pull/19158#discussion_r3722653261
##########
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:
Should we skip this for now considering we don't yet have a null-aware
star-tree?
##########
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:
"empty multiset" refers to specific aggregation function implementations
right? Should that be described in the generic `AggregationFunction` interface?
--
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]
