leerho opened a new pull request, #521: URL: https://github.com/apache/datasketches-cpp/pull/521
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. -- 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]
