xiangfu0 commented on code in PR #19795:
URL: https://github.com/apache/pinot/pull/19795#discussion_r4244476989
##########
pinot-core/src/test/java/org/apache/pinot/queries/SerializedBytesQueriesTest.java:
##########
@@ -276,7 +276,7 @@ public void testInnerSegmentAggregation()
// PercentileTDigest
TDigest tDigest = (TDigest) aggregationResult.get(4);
- TDigest expectedTDigest =
TDigest.createMergingDigest(PERCENTILE_TDIGEST_COMPRESSION);
+ TDigest expectedTDigest =
PercentileTDigestAccumulator.forLegacyAggregation(PERCENTILE_TDIGEST_COMPRESSION);
Review Comment:
With the tdunning dependency removed, the expected digests in these query
suites (here, and the same pattern in `ObjectSerDeUtilsTest`,
`PercentileTDigestQueriesTest`, `IndexedTableTest`) are now built with
`PercentileTDigestAccumulator` itself, so the implementation under test is its
own oracle. A systematic accuracy or merge-order regression in the K1 kernel
would shift "expected" and "actual" identically and pass these suites; the only
external accuracy guards left are the frozen `tdigest-3.3-k1-rank-errors.csv`
fixture and the pinned constants in `testCustomCompression`, which cover the
reducer-merge cases but not the broader group-by / inter-segment / star-tree
paths exercised here.
Consider extending the frozen-oracle coverage (e.g., pinned quantile
constants or a rank-error bound against the CSV) to at least one group-by and
one star-tree case in these suites, so a future kernel edit can't silently
drift both sides of the assertion.
--
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]
