Jackie-Jiang commented on PR #19158:
URL: https://github.com/apache/pinot/pull/19158#issuecomment-5245249504
@yashmayya — I replied to your line comments at the time but never responded
to the review summary itself, so its three points sat unaddressed. All three
are now done, and the PR has moved on enough since `17ff99e` that several of my
earlier replies no longer describe the code. Both below.
## The three points from your summary
**The description understated the blast radius.** You were right, and the
correction was still missing from the description until now. It claimed the
disabled path was unaffected; that holds only for the single-stage engine.
`AggregateOperator` builds every function with null handling enabled, so on the
multi-stage engine these functions change unconditionally with default options
and no query option controlling it. Your table is now in the description, and
flagged for the release notes:
| function | before | after |
|---|---|---|
| `percentileMV(mv, 50)` | `-Infinity` | `NULL` |
| `distinctSumMV(mv)` | `0.0` | `NULL` |
| `distinctAvgMV(mv)` | `NaN` | `NULL` |
The count is corrected to ten as well — `PERCENTILERAWESTMV`,
`PERCENTILERAWKLLMV` and `PERCENTILERAWTDIGESTMV` reach the option through the
function they extend or wrap.
**The regression test was missing, and your diagnosis of why my test would
not have caught it was exact.** `EmptyResponseUtilsTest` now covers
`PERCENTILERAWKLL(a, 50)`, `MAXSTRING(b)` and `MINSTRING(b)` over an empty
response.
I validated it by removing the null guard from
`PercentileRawKLLAggregationFunction` and re-running. It fails with:
```
java.lang.NullPointerException: Cannot invoke
"org.apache.datasketches.kll.KllDoublesSketch.toByteArray()" because
"this._sketch" is null
```
Note `toByteArray()`, reached through `ColumnDataType.convert` — not
`toString()`. So the render helper in the contract test was exercising a
different step from the one that throws, exactly as you said. It now calls both:
```java
finalResult.toString();
function.getFinalResultColumnType().convert(finalResult);
```
**No test covers multi-stage multi-value aggregates over an empty input.**
Still true. Not addressed here; it is a distinct gap from the one above and
wants a multi-stage test rather than a unit test.
## Where my earlier replies have gone stale
The scope grew after your review, and #19201 landed underneath it. Five of
my replies now describe code that is not there:
- **The serialization deviation.** I said fixing the writer was follow-up
work. #19201 made the writer support nulls for every column type independent of
the query option, so that deviation is deleted rather than deferred. It is also
what makes returning `null` instead of substituting an empty accumulator free
of fidelity cost.
- **The three percentile fixes.** I said I had applied your suggested branch
verbatim. Those functions are now fully conformed: the extractors return `null`
and `extractFinalResult` renders the disabled-mode value, so the code reads `if
(result == null || result.size() == 0L) return _nullHandlingEnabled ? null :
Double.NaN;`. Same answers, different shape.
- **The test harness.** I described keying the block type on a `READS_LONGS`
set. That set is gone; the harness now probes five block value types until one
drives the function and feeds every declared input column. It pulled 18 more
functions into the census, 14 of which honour the option and had never been
checked.
- **`Covariance`.** My reply cited "the fourth known deviation". Four
deviations are now two: every function that still substitutes an empty
accumulator is one that never receives the option, so the substitution is a
consequence of that rather than a defect of its own.
Beyond your review, the PR now also conforms every function that *does*
receive the option — `AVG`, `MIN_MAX_RANGE`, the variance family, the exact
distinct functions, `DISTINCT_COUNT_OFF_HEAP` and the percentiles, with their
raw and multi-value variants. Each disabled-mode value was derived from what
that function's substituted accumulator rendered, so answers are unchanged; the
description has the details.
Worth a fresh look at the diff rather than the resolved threads.
--
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]
