xiangfu0 commented on code in PR #19795:
URL: https://github.com/apache/pinot/pull/19795#discussion_r4216267277
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/customobject/PercentileTDigestAccumulator.java:
##########
@@ -939,10 +957,11 @@ private boolean mergeSorted(double[] incomingMeans,
double[] incomingWeights, in
return false;
}
+ double newTotalWeight = _totalWeight + incomingWeight;
Review Comment:
**[duplication] The ~90-line K1 merge kernel appears twice in this file.**
`mergeSorted()` is a near-verbatim copy of the merge loop inside
`flush(double)` — same q0/q2 eligibility math, runBackwards indexing, and
`inputIndex == 1 || inputIndex == numInputs - 1` boundary pinning; the only
difference is the raw path reads `_rawValues` with implicit weight 1.0.
Similarly, the static `normalizeBoundaries(means, weights, count, min, max)`
duplicates the instance `normalizeBoundaryCentroids()` algorithm, and
`addSerializedTDigestDirect` repeats `addSerializedTDigest`'s pending/legacy
prologue plus `mergeSerializedTDigest`'s initialize block. A future fix to the
merge criterion or boundary-repair rule applied to one copy would make
raw-value ingestion and digest-merge ingestion (or in-memory vs deserialized
digests) produce subtly different centroid layouts with no test to catch the
drift. Consider one shared kernel (weights-or-null for unit weight) and
delegating the instance normalizer to the static one.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/aggregator/PercentileTDigestValueAggregator.java:
##########
@@ -184,34 +197,37 @@ private static final class NonFiniteAwareTDigest extends
TDigest {
private static final int VERBOSE_CENTROID_SIZE = 16;
private final TDigest _finiteDigest;
- private long _negativeInfinityWeight;
- private long _positiveInfinityWeight;
+ private double _negativeInfinityWeight;
Review Comment:
**[duplication] Wire-format knowledge is now spread across four private
copies.**
The encoding constants (VERBOSE/SMALL encoding ids, header/centroid sizes)
and the centroid-capacity formula are re-declared in TDigestUtils,
PercentileTDigestAccumulator, this class's outer constants +
`getDefaultCentroidCapacity`, and NonFiniteAwareTDigest (plus
TDigestBenchmarkUtils) — and NonFiniteAwareTDigest additionally hand-parses the
verbose/small header three times (`fromBytes`, `getFiniteCentroids`,
`getSerializedBytes`) although `TDigestUtils.inspectSerialized` already returns
a `SerializedTDigestMetadata` with encoding, centroidCount, centroidOffset and
centroidSize (`fromBytes` even has it in hand via `SerializedTDigestInput`).
The capacity formula is a wire-compat contract (it bounds what a legacy reader
allocates); with Pinot now owning the format, one shared constants/metadata
holder in TDigestUtils would prevent a header or padding tweak from missing a
copy and reintroducing the legacy-reader AIOOBE class of bug this PR's comments
describe.
--
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]
