xiangfu0 commented on code in PR #19806:
URL: https://github.com/apache/pinot/pull/19806#discussion_r4278290297
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/PercentileAggregationFunction.java:
##########
@@ -206,10 +211,84 @@ public ColumnDataType getIntermediateResultColumnType() {
return ColumnDataType.OBJECT;
}
+ @Override
+ public DoubleArrayList mergeSerializedIntermediateResult(@Nullable
DoubleArrayList intermediateResult,
+ CustomObject serialized) {
+ if (intermediateResult == null) {
+ return deserializeIntermediateResult(serialized);
+ }
+ // Reads the values in bulk straight into the merged list, in the format of
+ // ObjectSerDeUtils.DOUBLE_ARRAY_LIST_SER_DE: an int count, then the
doubles, in the buffer's byte order. Same
+ // growth as addAll(), without the incoming list.
+ ByteBuffer buffer = serialized.getBuffer();
+ int numValues = buffer.getInt();
+ if (numValues < 0) {
+ throw new IllegalArgumentException("Negative number of values: " +
numValues);
+ }
+ if (numValues > buffer.remaining() / Double.BYTES) {
+ throw new BufferUnderflowException();
+ }
+ int size = intermediateResult.size();
+ int capacity = intermediateResult.elements().length;
+ if (size + numValues > capacity) {
+ intermediateResult.ensureCapacity(
+ (int) Math.min(Math.max((long) size + numValues, (long) capacity +
(capacity >> 1)), Integer.MAX_VALUE - 8));
Review Comment:
nit: in the extreme where `size + numValues` is a valid int but exceeds
`Integer.MAX_VALUE - 8`, this caps `ensureCapacity` below the size you then set
on the next line. It's harmless because `size(...)` re-grows internally, but
the cap is then doing nothing useful — a brief comment (or just letting the
clamp be the target) would make the intent clearer. Not reachable in practice
(needs a ~16GB list). Non-blocking.
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/PercentileAggregationFunction.java:
##########
@@ -206,10 +211,84 @@ public ColumnDataType getIntermediateResultColumnType() {
return ColumnDataType.OBJECT;
}
+ @Override
+ public DoubleArrayList mergeSerializedIntermediateResult(@Nullable
DoubleArrayList intermediateResult,
+ CustomObject serialized) {
+ if (intermediateResult == null) {
+ return deserializeIntermediateResult(serialized);
+ }
+ // Reads the values in bulk straight into the merged list, in the format of
+ // ObjectSerDeUtils.DOUBLE_ARRAY_LIST_SER_DE: an int count, then the
doubles, in the buffer's byte order. Same
+ // growth as addAll(), without the incoming list.
+ ByteBuffer buffer = serialized.getBuffer();
Review Comment:
Confirmed this reads in the buffer's own byte order, exactly like
`DOUBLE_ARRAY_LIST_SER_DE.deserialize`, so it stays consistent with the
serializer regardless of the data block's endianness. Reads the whole value and
leaves `remaining() == 0`, which the test asserts — nice.
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/AggregationFunction.java:
##########
@@ -220,13 +250,31 @@ public SerializedIntermediateResult(int type, byte[]
buffer) {
_bytes = buffer;
}
+ /// Constructor for subclasses that serialize on demand. They must
override [#getBytes()], [#getSize()] and
+ /// [#writeTo(DataOutput)].
+ protected SerializedIntermediateResult(int type) {
+ _type = type;
+ _bytes = null;
+ }
+
public int getType() {
return _type;
}
public byte[] getBytes() {
return _bytes;
}
+
+ /// Returns the number of bytes [#writeTo(DataOutput)] writes, which is
the length of [#getBytes()].
+ public int getSize() {
Review Comment:
nit: when a subclass uses the on-demand `SerializedIntermediateResult(type)`
constructor but forgets to override `getSize()`, this dereferences a null
`_bytes` and throws a bare NPE. Since the contract is spelled out in the
Javadoc, consider an `assert _bytes != null` (or a short message) so a misuse
points straight at the missing override. Non-blocking.
--
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]
