Stop persisting the full-size flag in serialization flags bit 32 (0x20)

[leerho]

Serialization flags bit 32 (0x20) was written as FULL_SIZE_FLAG_MASK here and is read as REBUILD_CURMIN_NUM_KXQ_MASK by datasketches-java, so the two implementations disagree about what a sketch image means. A C++ sketch built with start_full_size=true serializes flag FULL_SIZE, which Java heapifies with its REBUILD_CURMIN_NUM_KXQ_MASK (union rebuild) flag set (observed for HLL_4 and HLL_6; for HLL_8 Java's heapify clears it immediately) . Going the other way, a Java union-gadget image is read here as full size. This is a serious cross-language bug.

In C++, the persisted bit exists for one purpose: so that reset() on a deserialized sketch returns to a full-size array rather than to LIST. It also leaked into unions, because union_impl replaces the gadget via copyAs()/copy() and the rvalue update() overload moves a sketch in wholesale, both of which carry the startFullSize_ across. A union's reset behavior therefore depended on which sketches had been merged into it. This is a bug internal to C++. A user that happened to merge in a sketch that was configured full-size would permanently configure his sketch to full-size mode without his knowledge.

Fixing this collision between Java and C++ involves minimizing the impact surface to both Java backwards compatibility and C++ backwards compatibility. On the C++ side this Full-Size capability was never fully implemented and never tested, and it contained a bug in the union implementation as well.

Fix Strategy: Make full-size a property of the call that creates the state instead. This means if the user wants this feature it must be requested when the sketch is created and requested upon reset (if reset is required). This feature is a runtime dynamic and never persisted, which removes the requirement to have the bit for C++. This fixes this bug going forward, there is not much we can do about historical C++ sketch images.

• add hll_sketch_alloc::reset(bool full_size = false); reset() returns to coupon collection mode as Java does, reset(true) to an empty full-size HLL array
• remove startFullSize_, isStartFullSize(), and the bool parameter on the HllArray/Hll4Array/Hll6Array/Hll8Array constructors and newHll()
• stop writing and reading bit 32; rename the constant RESERVED_FLAG_MASK_32 and record the collision, noting bits 64 and 128 are free
• hll_union_alloc::reset() resets its gadget to LIST explicitly

Removing the state fixes the union leak by construction. Reading is unaffected: older images still deserialize and the bit is ignored. Output is byte-identical to master except for sketches created with start_full_size=true, verified over a 1047-record corpus spanning lg_k 4..21, all three target types, 17 sizes across LIST/SET/HLL, round trips and 80 union scenarios. One deliberate behavior change: a full-size sketch that has been serialized and deserialized now resets to LIST; call reset(true) for the previous effect.

Adds HllFullSizeTest.cpp. The five assertions that also compile against the previous headers fail there and pass here.

[coveralls]

Coverage Report for CI Build 34049496700

Warning

Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes.
Quick fix: rebase this PR. Learn more →

Coverage increased (+0.005%) to 82.321%

Details

• Coverage increased (+0.005%) from the base build.
• Patch coverage: 28 of 28 lines across 9 files are fully covered (100%).
• 2 coverage regressions across 1 file.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

2 previously-covered lines in 1 file lost coverage.

File	Lines Losing Coverage	Coverage
common/include/serde.hpp	2	86.14%

---

Coverage Stats

Relevant Lines:	21477
Covered Lines:	17680
Line Coverage:	82.32%
Coverage Strength:	1359149.61 hits per line

---

💛 - Coveralls

[proost]

Thank you for fixing the bug.

Breaking change is inevitable, but my main concern is that the fix is more breaking than it needs to be, in a way that is easy to miss.

[proost]

Removing the trailing const bool startFullSize_ is a silent ABI break. How about keeping the member and simply not serializing? it reaches the same stated goal.

[proost]

Most users don't user it. But if some user use it, then it makes a hard compile break.

One of more safe choice is introducing new const and make deprecation. And next release remove it.

[leerho]

@proost,
Thanks for the careful review — this is exactly the right thing to be
checking, and you're right on the second point. I've pushed a fix for it.
On the first point I think the measurements argue the other way; details
below. ABI issues can be subtle to detect and fix. So I asked Claude for help
on this one.

---

FULL_SIZE_FLAG_MASK → RESERVED_FLAG_MASK_32 — agreed, fixed

You're right, and this is the one break in the PR that costs nothing to
avoid. It's also the only loud one: it breaks the build rather than
misbehaving quietly. Fixed in eeb8d1f by keeping the old name as an alias:

static const uint8_t RESERVED_FLAG_MASK_32    = 32;
// Deprecated alias for the bit above, retained for source compatibility. Do not use.
// (No [[deprecated]] attribute: this library targets C++11, where it is unavailable.)
static const uint8_t FULL_SIZE_FLAG_MASK      = RESERVED_FLAG_MASK_32;

I couldn't use [[deprecated]] as you suggested — the library is
CMAKE_CXX_STANDARD 11 and that attribute is C++14 — so the deprecation
is a comment. With this in, the PR introduces no source break at all.

Removing startFullSize_ — I'd like to keep it removed

On the layout claim: you're right that something shifts, though not in
the classes one would expect. From Clang's -fdump-record-layouts,
before vs. after:

class	sizeof	member offsets
HllSketchImpl<A>	24 → 24	dsize 21 → 20
HllArray, Hll4/6/8Array	unchanged	unchanged (own members are 8-byte-aligned doubles; still start at 24)
CouponList<A>	56 → 56	couponCount_ 24 → 20, oooFlag_ 28 → 24
CouponHashSet<A>	unchanged	same 4-byte shift

So there is a real shift, in the coupon classes, and sizeof hides it.

But it can't be observed. Every accessor on CouponList /
CouponHashSet is virtual (CouponList.hpp lines 43–79) — there is no
non-virtual inline member access anywhere in the hierarchy. Member reads
therefore only ever happen inside one link-time-selected definition, and
since sizeof is unchanged, allocation and deallocation sizes still
match. I linked a TU built against master headers against a TU built
against this branch, at -O0, -O2 and -O3, in both link orders:
identical results every time, no corruption. The shift is unreachable by
construction, not by luck.

On "keeping the member reaches the same goal" — I implemented your
suggestion and measured it. Keeping the member, the ctor and
isStartFullSize(), and only dropping the read/write of bit 32:

	master	keep-the-member	this PR
born full-size → reset()	full-size	full-size	LIST
same sketch round-tripped → reset()	full-size	LIST	LIST
round-trip consistent?	yes	no	yes
bit 32 written	0x20	no	no
union fed a full-size sketch → reset()	full-size (bug)	full-size (bug remains)	LIST (fixed)

Two problems with the minimal version. It doesn't fix the union bug,
which is the PR's second stated goal — a union that has had a full-size
sketch merged into it still resets its gadget to full-size. And it
introduces a new silent inconsistency: a sketch and its own
deserialized clone now disagree about what reset() does. Removing the
persistence of a sticky flag while keeping the flag makes behaviour
depend on unserialized history, which seems to me squarely in the
"easy to miss" category you're trying to avoid.

On scope: keeping the member addresses 1 of the 12 changed symbols.
Diffing emitted symbols for the default allocator, this PR removes:

• HllSketchImpl::isStartFullSize() ← the only one from the data member
• HllSketchImpl::reset(), hll_sketch_alloc::reset()
• HllSketchImpl, HllArray, Hll4Array×2, Hll6Array×2, Hll8Array×2 ctors
• HllSketchImplFactory::newHll(..., bool, ...)

The other 11 are signature changes, and they're all loud. (Note
hll_sketch_alloc::reset() → reset(bool = false) is source-compatible
for every caller; it only breaks a pointer-to-member.)

On ABI policy generally

Worth separating from this PR: every target in the build is
add_library(... INTERFACE) — 15 of them, header-only, no SONAME, no
compiled artifact, no explicit template instantiations. In that model any
change to any inline function body is already an ODR hazard for anyone
mixing header versions in one link, so the practical rule has always been
"recompile everything," and I don't think this PR changes the exposure.

There's also precedent already on master for 5.3.0 — private data members
removed from public template classes that shipped in 5.2.0:

• tdigest<T,A>::internal_k_ and ::buffer_capacity_ (315e50b)
• theta_update_sketch_base::buf_i_ (eb3200e)

Same class of change. I'd rather not hold startFullSize_ to a stricter
standard than those unless we adopt a policy for the whole library — which
might be a good dev@ thread, but probably shouldn't be settled per-PR.

What recompiling does and doesn't cover

Since the practical rule here is "recompile everything," it's worth being
explicit that recompiling only certifies one of three independent
compatibility axes:

axis	what it means	caught by recompiling?	status in this PR
API / source	does downstream code still compile	yes, loudly	clean, once the alias above went back in
ABI / binary	can old and new object files be linked together	moot if you recompile — and safe here regardless (sizeof unchanged, all accessors virtual)	verified safe
Serialization / wire	can old and new read each other's bytes	no — invisible to the compiler	verified compatible both directions

The middle row is the one we've been discussing, and it's moot for anyone
who follows the rule. The third row hasn't come up in this thread, and for
a sketching library it's the one that would actually hurt: sketches get
persisted and shipped between systems and across languages, so a mismatch
there is something the compiler can never warn about.

So I checked it. lg_k=12, 5000 updates, HLL_8, serializing with master
headers and deserializing with this branch, and vice versa:

• all four cross-version combinations produce bit-identical estimates and
  bounds (est=4943.6008367398, matching to the last digit)
• with start_full_size=false, the serialized bytes are byte-for-byte
  identical between the two versions
• with start_full_size=true, exactly one byte of 4136 differs:
  offset 5, the flags byte, 0x20 → 0x00. That is the single bit this
  PR is vacating. Nothing else moves.

Old readers accept new bytes, new readers accept old bytes, and the
estimates agree in both directions.

For completeness: the one thing in this PR that does change silently —
compiles clean, no diagnostic, different result — is reset() on a sketch
born full-size, which now returns to LIST rather than a full-size HLL
array. That is the intended fix and the previous behaviour was the bug,
but it is the only genuinely silent change here, and it isn't an ABI
question.

Happy to discuss any of this, especially if you can see a way
to fix the union bug that keeps the member.