ariesdevil commented on code in PR #168:
URL: https://github.com/apache/datasketches-rust/pull/168#discussion_r3664020806


##########
datasketches/tests/tuple_test/a_not_b.rs:
##########
@@ -15,228 +15,155 @@
 // specific language governing permissions and limitations
 // under the License.
 
-//! Behavioral tests for the Tuple a-not-B (set difference) operator.
-//!
-//! The result of `a and not b` retains the keys of `a` that are absent from 
`b`, keeping the
-//! summaries from `a`. These tests use a `u64` summary with the default 
additive update policy,
-//! so the distinct-count behavior matches a plain Theta a-not-B.
-
+use datasketches::common::NumStdDev;
+use datasketches::error::ErrorKind;
 use datasketches::tuple::CompactTupleSketch;
 use datasketches::tuple::TupleANotB;
 
 use super::default_tuple_sketch_builder;
 use super::tuple_sketch_with_range;
 
+fn sorted_entries(sketch: &CompactTupleSketch<u64>) -> Vec<(u64, u64)> {
+    let mut entries: Vec<_> = sketch
+        .iter()
+        .map(|(hash, &summary)| (hash, summary))
+        .collect();
+    entries.sort_unstable();
+    entries
+}
+
 #[test]
-fn test_basic_difference_keeps_summaries_from_a() {
+fn difference_keeps_only_a_summaries() {
     let mut a = default_tuple_sketch_builder().build();
     a.update("shared", 3u64);
     a.update("only_a", 5u64);
     let mut b = default_tuple_sketch_builder().build();
     b.update("shared", 9u64);
     b.update("only_b", 7u64);
 
-    let a_not_b = TupleANotB::default();
-    let r = a_not_b.compute(&a, &b, true).unwrap();
+    let result = TupleANotB::default().compute(&a, &b, true).unwrap();
 
-    // "shared" is subtracted; "only_a" survives with A's summary.
-    assert_eq!(r.num_retained(), 1);
-    assert_eq!(r.iter().next().unwrap().1, &5);
-    assert!(!r.is_estimation_mode());
-    assert_eq!(r.estimate(), 1.0);
+    assert_eq!(result.num_retained(), 1);
+    assert_eq!(result.estimate(), 1.0);
+    assert_eq!(result.iter().next().unwrap().1, &5);
 }
 
 #[test]
-fn test_accepts_updatable_and_compact_inputs() {
+fn accepts_mutable_and_compact_inputs() {
     let a = tuple_sketch_with_range(0, 1000);
     let b = tuple_sketch_with_range(500, 1000);
+    let op = TupleANotB::default();
 
-    let a_not_b = TupleANotB::default();
-    let r = a_not_b.compute(&a.compact(true), &b, true).unwrap();
-    assert_eq!(r.num_retained(), 500);
-
-    let r = a_not_b.compute(&a, &b.compact(false), true).unwrap();
-    assert_eq!(r.num_retained(), 500);
-}
-
-#[test]
-fn test_seed_mismatch_returns_error() {
-    let mut one_other_seed = default_tuple_sketch_builder().seed(2).build();
-    one_other_seed.update("value", 1u64);
-    let good = tuple_sketch_with_range(0, 10);
-
-    let a_not_b = TupleANotB::with_seed(1);
-    assert!(a_not_b.compute(&one_other_seed, &good, true).is_err());
-    assert!(a_not_b.compute(&good, &one_other_seed, true).is_err());
-}
-
-#[test]
-fn test_seed_mismatch_ignored_for_empty_inputs() {
-    // Empty inputs carry no keys, so their seeds are not validated.
-    let empty_other_seed = default_tuple_sketch_builder().seed(2).build();
-    let good = tuple_sketch_with_range(0, 10);
-
-    let a_not_b = TupleANotB::default();
-
-    let r = a_not_b.compute(&empty_other_seed, &good, true).unwrap();
-    assert!(r.is_empty());
-
-    let r = a_not_b.compute(&good, &empty_other_seed, true).unwrap();
-    assert_eq!(r.num_retained(), 10);
-}
-
-#[test]
-fn test_empty_a_returns_empty() {
-    let empty = default_tuple_sketch_builder().build();
-    let b = tuple_sketch_with_range(0, 1000);
-
-    let a_not_b = TupleANotB::default();
-    let r = a_not_b.compute(&empty, &b, true).unwrap();
-
-    assert!(r.is_empty());
-    assert_eq!(r.num_retained(), 0);
-    assert_eq!(r.estimate(), 0.0);
-}
-
-#[test]
-fn test_empty_b_returns_a() {
-    let a = tuple_sketch_with_range(0, 1000);
-    let empty = default_tuple_sketch_builder().build();
-
-    let a_not_b = TupleANotB::default();
-    let r = a_not_b.compute(&a, &empty, true).unwrap();
-
-    assert_eq!(r.num_retained(), 1000);
-    assert_eq!(r.estimate(), 1000.0);
-}
-
-#[test]
-fn test_exact_partial_overlap_unordered() {
-    let a = tuple_sketch_with_range(0, 1000);
-    let b = tuple_sketch_with_range(500, 1000);
-
-    let a_not_b = TupleANotB::default();
-    let r = a_not_b.compute(&a, &b, true).unwrap();
+    let mutable_result = op.compute(&a, &b, true).unwrap();
+    let compact_result = op
+        .compute(&a.compact(true), &b.compact(false), true)
+        .unwrap();
 
-    // Keys 0..500 survive (exact mode).
-    assert!(!r.is_empty());
-    assert!(!r.is_estimation_mode());
-    assert_eq!(r.num_retained(), 500);
-    assert_eq!(r.estimate(), 500.0);
+    assert_eq!(
+        sorted_entries(&mutable_result),
+        sorted_entries(&compact_result)
+    );
+    assert_eq!(mutable_result.num_retained(), 500);
 }
 
 #[test]
-fn test_exact_partial_overlap_ordered() {
-    let a = tuple_sketch_with_range(0, 1000);
-    let b = tuple_sketch_with_range(500, 1000);
+fn ordered_and_unordered_inputs_produce_the_same_result() {
+    let mut a = default_tuple_sketch_builder().lg_k(8).build();
+    let mut b = default_tuple_sketch_builder().lg_k(8).build();
+    for value in 0..20_000 {
+        a.update(value, 1u64);
+    }
+    for value in 10_000..30_000 {
+        b.update(value, 1u64);
+    }
 
-    let a_not_b = TupleANotB::default();
-    let r = a_not_b
+    let op = TupleANotB::default();
+    let unordered = op.compute(&a, &b, true).unwrap();
+    let ordered = op
         .compute(&a.compact(true), &b.compact(true), true)
         .unwrap();

Review Comment:
   Seems we also need to test `ordered=false` 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]

Reply via email to