leerho commented on PR #524: URL: https://github.com/apache/datasketches-cpp/pull/524#issuecomment-5610261460
No reason at all — it was an omission, and a good catch. Rather than add the line, I removed the cause. `theta_union_base::get_result` and this new trim path were two copies of one algorithm, and the newer copy had already drifted from the older one. e13672e extracts it into a single `trim_to_nominal` in `theta_helpers.hpp` and calls it from both sites. Since `theta_union_base` is instantiated by both `theta_union` and `tuple_union`, all three paths now run identical code — theta's `compact` is just the degenerate case where the entry is the key. It mattered most exactly where it was missing. An update sketch retains up to `15/16 * 2k` entries between rebuilds, and the entries vector is reserved to that count before being erased down to `k`, so a trimmed result kept the untrimmed allocation: at `lg_k=12`, 7674 retained against 4096 kept, holding 1.87x the memory it reports with 46.6% of the allocation dead. A bounded result is the entire point of `trim`, so the caller was paying the accuracy cost without getting the size back. The extraction is behaviour-preserving — serialized bytes of both the trimmed compact result and the union result are identical before and after, across ordered and unordered at 5000, 26989 and 40000 updates. Details in the description, along with a note on why the same flag is not being added to `update_tuple_sketch::compact()` in this PR. -- 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]
