jaideeppyne opened a new pull request, #170:
URL: https://github.com/apache/datasketches-go/pull/170
`req.NewSketch()` leaves `minItem`/`maxItem` at Go's zero value, but `Merge`
and `Reset` both use NaN as the "unset" sentinel. So merging into a freshly
created sketch computes `min(0, other.minItem)` and `max(0, other.maxItem)`,
and the 0 wins whenever the stream doesn't straddle it.
```go
src, _ := req.NewSketch()
for i := 1001; i <= 1100; i++ { src.Update(float32(i)) }
dst, _ := req.NewSketch()
dst.Merge(src)
dst.MinItem() // 0, want 1001
dst.Quantile(0) // 0, want 1001
dst.Rank(500) // 0.01, want 0
```
It's not just the getters. The bogus 0 is reinserted into the sorted view as
a retained quantile, so `Quantile`, `Rank`, `CDF` and `PMF` all shift, and it
survives `MarshalBinary` + `Decode`. Chained merges into an empty accumulator,
which is the usual map-reduce shape, hit it too. `NewSketch()` then `Reset()`
then `Merge` works fine, which is the tell.
`ReqSketch.java` initialises the fields at declaration:
```java
private float minItem = Float.NaN;
private float maxItem = Float.NaN;
```
Go has no field initialisers so the port dropped it. I set them in
`NewSketch` instead. That covers every construction site: the decoder's empty
and raw-items paths both go through `NewSketch`, and the exact and estimation
paths read min/max out of the buffer. `Update` guards on `IsEmpty()` so it was
never affected, and every reader of the fields (`MinItem`, `MaxItem`,
`refreshSortedView`, the estimation-format encoder) is already behind an
emptiness check, so no path can now observe the NaN.
How I found it: I built an invariant harness over the quantile families
asserting that min/max after merging into a fresh sketch equals min/max after
merging into a reset sketch, and equals the source's. 5 value patterns across
REQ, KLL and t-digest. KLL and t-digest are clean; REQ fails 4 of 5. The one
that passes is the case whose data straddles zero, which is why the existing
`Merge Multiple` test misses it. I checked the intended semantics against
`ReqSketch.java` rather than guessing at it. The same harness also ran
200-trial randomised sweeps per family for quantile monotonicity in rank, CDF
within [0,1] and non-decreasing, PMF summing to 1, and serialize/deserialize
identity; nothing else came out of it, and I confirmed `tdigest.Quantile`
matches `tdigest_impl.hpp` line for line.
7 subtests added in `TestSketchMergeIntoEmptyMinMax`. All 7 fail on `main`
with only the test file applied, all 7 pass with the two-line change. `make
lint-check` and `go test -race ./req/` are clean and the full suite is green.
Written with AI assistance (Claude). I reviewed the change and ran
everything above myself.
--
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]
