ZENOTME commented on PR #142: URL: https://github.com/apache/datasketches-rust/pull/142#issuecomment-4894898388
> 1. Java's `ThetaSketch::getRetainedEntries` has a `valid` param, but our `CompactThetaSketch::num_retained` doesn't. Could you describe a bit what the design difference between the Java impl and the Rust impl? Java has a specific update implementation, AlphaSketch, where theta can be reduced without immediately rebuilding the internal cache. This means some cached entries may become invalid under the new theta but remain in the cache until a later rebuild. Therefore, getRetainedEntries(true) returns the valid entries used for estimation, while getRetainedEntries(false) returns the raw internal cache count, including possibly invalid entries. Personally, I do not think this is an ideal public API shape. num_retained / getRetainedEntries should mean the logical retained count used for estimation and set operations. If the raw cache count needs to be exposed, it would be clearer as a separate API. > 2. This PR has a fake `ThetaUnion` impl. Did we have an equivalent ThetaUnion impl so far? I found it strange to have a `pub(super) struct ThetaUnion;`. We don't have ThetaUnion so far. I think it maybe better to implement it first. -- 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]
