feat(theta): add theta union#145
Conversation
|
|
||
| let ordered = ordered || (entries.len() == 1 && theta == MAX_THETA); | ||
|
|
||
| if ordered && entries.len() > 1 { |
There was a problem hiding this comment.
seems no need check with entries.len() > 1, sort_unstable() no-op on single element array.
tisonkun
left a comment
tisonkun
left a comment
There was a problem hiding this comment.
Generally LGTM. Some follow-up suggestions inline, and:
I may prefer to name result/result_with_order to to_sketch as we do for the CpcSketch and HllSketch. Also, we may not need to have two methods but only one with a ordered param.
This is a breaking change since we already use the result_with_order API in ThetaIntersection, but it should be worthy to do the refactor.
| ThetaUnionBuilder::default() | ||
| } | ||
|
|
||
| /// Update this union with a given sketch. |
There was a problem hiding this comment.
Later we may review all seed_hash != seed_hash checks and add # Errors document.
|
Now that #146 is merged. We may merge this as well. Is there any logical conflict that should be resolved beforehand? |
Yes. Let me covert it as a generic version. |
tisonkun
left a comment
tisonkun
left a comment
There was a problem hiding this comment.
Thanks for your updates!
I'll go through all these changes and perhaps push a few commits to handle visibility issues.
Hopefully in days, but may be delay to next weekend.
| use crate::theta_common::hash_table::RawHashTableEntry; | ||
|
|
||
| pub(crate) mod private { | ||
| pub(crate) trait Sealed {} |
There was a problem hiding this comment.
BTW: As @tisonkun mentioned on X, pub impl(crate) trait Foo {} just landed in nightly version; maybe we can use this feature?
There was a problem hiding this comment.
This crate is a library and we should stay on stable toolchain. And we have an MSRV of 1.86.
3 participants
Summary
Tests