Jackie-Jiang commented on PR #19217:
URL: https://github.com/apache/pinot/pull/19217#issuecomment-5260068186
@yashmayya thanks for the review — both notes were right, and both are fixed.
**The stale doc line.** You are right about which comment attached: the old
`/// Deserializes the sketches from the bytes.` sat above the annotation and my
accurate line went below it, so the stale one was the Javadoc and mine was an
ordinary comment. I replaced the method and left its old documentation in
place. The stale line is gone and mine is above the signature.
`@SuppressWarnings({"unchecked"})` is gone too — it covered the `new
TupleSketch[length]` generic array creation, which the per-row deserializer no
longer does. Good catch that it had become dead.
**The group-by disabled-mode answer.** Also right, and the description was
wrong to say the disabled mode renders exactly what it rendered before. It does
on the aggregation path; on the group-by path a group holding no rows — which a
filtered aggregation creates — moves `SUMVALUESINTEGERSUMTUPLESKETCH` from
`NULL` to `0` and the raw variant from `NULL` to the serialized empty sketch.
The description now states this, and that it resolves the group-versus-no-group
inconsistency in favour of the aggregation path's answer.
**Two more from the Copilot review, both real.** A zero-length block still
reaches the range callback as `(0, 0)`, so the accumulator was being created
for an input that aggregated nothing; the range now returns early and a new
test pins it. And moving deserialization into the range accidentally put it
inside the group-key loop on the multi-value path, heapifying a row once per
group key instead of once — now hoisted back out.
The mixed-version note is in the description as well: a new server sends
`null` where it used to send an empty accumulator, an old broker renders that
as `NULL`, and upgrading brokers first avoids it.
--
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]
