xiangfu0 commented on code in PR #19795:
URL: https://github.com/apache/pinot/pull/19795#discussion_r4248113732
##########
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 from corrupted historical
inputs sort after 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);
}
@Override
public int compareTo(SerializedTDigest other) {
checkArgument(other._percentile == _percentile, "Percentile number doesn't
match!");
- return Double.compare(_tDigest.quantile(_percentile),
other._tDigest.quantile(_percentile));
+ return Double.compare(_quantile, other._quantile);
Review Comment:
`Double.compare(_quantile, other._quantile)` ranks NaN above every finite
value and +Inf. Since degraded historical digests now yield NaN quantiles, a
group whose stored bytes are poisoned sorts above every healthy group in
`SELECT ... ORDER BY PERCENTILERAWTDIGEST(m, 95) DESC LIMIT N`, silently
occupying the top slots. The class doc acknowledges the ordering, but is
NaN-first-in-DESC the intended behavior for corrupted groups? Consider ordering
NaN last in both directions, or at least pinning the current choice with a test.
--
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]
