jaideeppyne commented on PR #517: URL: https://github.com/apache/datasketches-cpp/pull/517#issuecomment-5472939192
Write half is up as #518, stacked on this branch. One thing came out of doing it that is worth flagging here, because it affects the order. On its own the write change is a regression, and this PR is what makes it safe. `theta_set_difference_base::compute` checks B's seed hash, and its early return only covers an empty B when A has retained entries, so an A that is non-empty with zero retained still reaches the check. Today both sides carry the same computed hash so it passes; once an empty B reports 0 it would throw. On a real sketch: ``` A: is_empty=0 retained=0 seed_hash=37836 B: is_empty=1 seed_hash=37836 early return taken? no a_not_b: OK ``` So #518 wants this one in first. For the record on the rest of the surface: `theta_union_base::update` returns early on empty, `theta_intersection_base::update` already has `!sketch.is_empty() &&`, and `deserialize_v3` has `if (!is_empty) check_seed_hash(...)`, so a zero hash on an empty image reads back cleanly. `deserialize_v1`/`v2` check unconditionally, but nothing writes an empty sketch in those formats. After #518 an empty compact sketch serializes to `1 3 3 0 0 30 0 0`, which is `EMPTY_COMPACT_SKETCH_ARR` byte for byte. -- 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]
