Jackie-Jiang commented on PR #19227:
URL: https://github.com/apache/pinot/pull/19227#issuecomment-5273087858
@yashmayya thank you — the `DataBlockCache` analysis is the valuable part of
this review, and I had not realised the change fixed a correctness bug.
## The wrong-results fix
Verified: `initNewBlock` clears only when the block grows.
```java
if (length > _length) { _dictIdsMap.clear(); _valuesMap.clear();
_numValuesMap.clear(); }
```
So a short block after a full one reuses the longer array with only its
first `length` entries refreshed, and the old `for (Long val :
valueSet.getLongValuesSV())` counted the previous block's tail again. Every
segment with more than one block inflated its counts, and the `BYTES` paths had
the same shape through `deserializeSketches`.
The description now carries this as its own section, and says explicitly
that the corrected counts are visible with the option **off** — this is not
only a null-handling change, and it deserves a release note on that footing.
`testValuesBeyondLengthAreNotCounted` pins it. I checked it fails for the
right reason: restoring the array-iterating loop gives `expected [1] but found
[2]`, which is the double count in miniature.
## The stale doc line
Fixed in both files. This is the same mistake you caught in #19217 — I
replaced the method and left its old documentation above the new line, so the
plural first sentence contradicted the singular second one. I fixed it there
and did not add the check when I repeated the edit here.
## Also added
From the Copilot review: the contract test only drives one synthetic shape
through `aggregate`, so the `BYTES` and group-by paths were unexercised.
`FrequentSketchNullHandlingTest` covers those, the zero-length block, and both
option settings.
--
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]
