Jackie-Jiang commented on code in PR #19795:
URL: https://github.com/apache/pinot/pull/19795#discussion_r4251215753
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/customobject/tdigest/SerializedTDigest.java:
##########
@@ -16,32 +16,34 @@
* specific language governing permissions and limitations
* under the License.
*/
-package org.apache.pinot.segment.local.customobject;
+package org.apache.pinot.segment.local.customobject.tdigest;
-import com.tdunning.math.stats.TDigest;
-import org.apache.pinot.segment.local.utils.CustomSerDeUtils;
import org.apache.pinot.spi.utils.BytesUtils;
import static com.google.common.base.Preconditions.checkArgument;
-/// Serialized and comparable version of TDigest. Compares TDigest for a
specific percentile value.
+/// Immutable serialized result and comparison key for a specific percentile
value.
+/// Uses Double.compare ordering: NaN quantiles sort last ascending and first
descending, ahead of positive infinity.
public class SerializedTDigest implements Comparable<SerializedTDigest> {
private final double _percentile;
- private final TDigest _tDigest;
+ private final byte[] _serializedBytes;
+ private final double _quantile;
public SerializedTDigest(TDigest tDigest, double percentile) {
- _tDigest = tDigest;
_percentile = percentile / 100.0;
+ // Finalize once before obtaining the sort key; serialization also owns
historical byte-retention policy.
+ _serializedBytes = tDigest.serialize();
+ _quantile =
PercentileTDigestAccumulator.fromBytes(_serializedBytes).quantile(_percentile);
Review Comment:
**[P2] Avoid rebuilding a digest for every RAW sort key.**
`TableResizer.AggregationFunctionExtractor` calls `extractFinalResult` for
every ORDER BY trim candidate, including discarded groups and surviving groups
on subsequent resizes. This constructor now serializes the digest and then
clones, parses, and materializes another digest solely to calculate its
comparison key.
A warmed allocation probe using the exact dependency baseline and this head,
with identical compression-100, 69-centroid inputs (1,136 serialized bytes),
measured **24 versus 5,512 allocated bytes per construction plus one
comparison**. That is about **549 MB of additional allocation per 100,000
candidates**, even when the input is already compressed. The input was 10,000
uniform values from `new Random(17)`; allocation was measured with
`ThreadMXBean` over 100,000 iterations after warmup. Cached comparisons can
save centroid scans, so this is an allocation finding, not an end-to-end
latency claim.
Please preserve the immutable bytes and stable comparison key, but derive
the key from the actual finalized encoding without reconstructing a mutable
digest, or return the bytes and key together from native finalization. Do not
substitute the source's quantile blindly: compatibility rewriting or compact
encoding can change the emitted distribution.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/customobject/tdigest/SerializedTDigest.java:
##########
@@ -16,32 +16,34 @@
* specific language governing permissions and limitations
* under the License.
*/
-package org.apache.pinot.segment.local.customobject;
+package org.apache.pinot.segment.local.customobject.tdigest;
-import com.tdunning.math.stats.TDigest;
-import org.apache.pinot.segment.local.utils.CustomSerDeUtils;
import org.apache.pinot.spi.utils.BytesUtils;
import static com.google.common.base.Preconditions.checkArgument;
-/// Serialized and comparable version of TDigest. Compares TDigest for a
specific percentile value.
+/// Immutable serialized result and comparison key for a specific percentile
value.
+/// Uses Double.compare ordering: NaN quantiles sort last ascending and first
descending, ahead of positive infinity.
public class SerializedTDigest implements Comparable<SerializedTDigest> {
private final double _percentile;
- private final TDigest _tDigest;
+ private final byte[] _serializedBytes;
+ private final double _quantile;
public SerializedTDigest(TDigest tDigest, double percentile) {
- _tDigest = tDigest;
_percentile = percentile / 100.0;
+ // Finalize once before obtaining the sort key; serialization also owns
historical byte-retention policy.
+ _serializedBytes = tDigest.serialize();
Review Comment:
**Accuracy concern: RAW sort-key extraction recompresses live
intermediates.** This is also called during group trimming, before aggregation
is finished. `tDigest.serialize()` applies public compression to the live
accumulator; surviving groups are subsequently merged again. Previously,
comparison called `quantile()`, which only flushed at working compression.
Freezing the comparison key fixes the earlier comparison/rendering
inconsistency, but now repeated trims introduce additional lossy compression
into the continuing aggregation.
A focused probe on the same latest kernel isolates this effect: compression
100; 32 shards of 2,000 values from one `new Random(7)`, transformed with
`StrictMath.pow(u, 8)`; each shard is serialized and independently restored
with `forReduction(100)` before merging into two reducers. After every fourth
shard except the last, the control calls `quantile(.75)` and the treatment
constructs `new SerializedTDigest(reducer, 75)`. Both reducers are finally
serialized and decoded before measuring rank against the sorted original
values. **P50 absolute rank error increases from 0.001703125 to 0.00421875.**
This simulates the repeated extraction/merge lifecycle; it is not an end-to-end
query benchmark.
I have not established an existing accuracy-tolerance violation, so I am
raising this as an observed accuracy concern rather than an additional
correctness blocker. Please consider obtaining the finalized comparison result
from a non-mutating snapshot/view and add a repeated trim-and-merge regression
while retaining the stable comparison/serialized-output contract. Removing the
decode on the next line alone will not address this source mutation.
--
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]
