Copilot commented on code in PR #239:
URL: https://github.com/apache/datasketches-rust/pull/239#discussion_r3885166259
##########
tests-integration/tests/hll_test/union.rs:
##########
@@ -708,3 +710,49 @@ fn test_union_estimated_size() {
union.update(&sketch);
assert_eq!(union.estimated_size(), 1120);
}
+
+#[test]
+fn test_union_of_single_sketch_reproduces_its_estimate_and_bounds() {
+ for hll_type in HLL_TYPES {
+ let sketch = make_hll_sketch(hll_type, 8, 0, 1_000);
+ let mut union = HllUnion::new(10).unwrap();
+ union.update(&sketch);
+
+ assert_eq!(union.estimate(), sketch.estimate(), "{hll_type:?}
estimate");
+ assert_eq!(
+ union.to_sketch(HllType::Hll8).estimate(),
+ sketch.estimate(),
+ "{hll_type:?} result estimate",
+ );
+
+ for num_std_dev in [NumStdDev::One, NumStdDev::Two, NumStdDev::Three] {
+ assert_eq!(
+ union.lower_bound(num_std_dev),
+ sketch.lower_bound(num_std_dev),
+ "{hll_type:?} lower bound at {num_std_dev:?}",
+ );
+ assert_eq!(
+ union.upper_bound(num_std_dev),
+ sketch.upper_bound(num_std_dev),
+ "{hll_type:?} upper bound at {num_std_dev:?}",
+ );
+ }
+ }
+}
+
+#[test]
+fn test_union_of_single_sketch_reproduces_its_estimate_when_downsampling() {
+ for hll_type in HLL_TYPES {
+ let sketch = make_hll_sketch(hll_type, 12, 0, 10_000);
+ let mut union = HllUnion::new(8).unwrap();
+ union.update(&sketch);
+
+ assert_eq!(union.lg_config_k(), 8, "{hll_type:?} lg_config_k");
+ assert_eq!(union.estimate(), sketch.estimate(), "{hll_type:?}
estimate");
+ assert_eq!(
+ union.to_sketch(HllType::Hll8).estimate(),
+ sketch.estimate(),
+ "{hll_type:?} result estimate",
+ );
Review Comment:
This test asserts exact equality between a high-precision sketch estimate
(lg_k=12) and a downsampled union estimate (lg_k=8). Downsampling changes the
sketch precision/estimator path, so exact equality is not a stable or generally
guaranteed property (note earlier downsampling tests in this file use
ranges/tolerances instead). Consider asserting the estimate is close to the
true cardinality instead.
##########
datasketches/src/hll/union.rs:
##########
@@ -601,12 +617,15 @@ fn copy_or_downsample(src_mode: &Mode, src_lg_k: u8,
tgt_lg_k: u8) -> Array8 {
}
}
- result.set_hip_accum(src_hip);
result
} else {
// Downsample from src to tgt
let mut result = Array8::new(tgt_lg_k);
merge_array_with_downsample(&mut result, tgt_lg_k, src_mode, src_lg_k);
result
- }
+ };
+
+ result.set_out_of_order(src_out_of_order);
+ result.set_hip_accum(src_hip);
+ result
Review Comment:
`copy_or_downsample` forces `Array8` to rebuild via
`merge_array_with_downsample` when `src_lg_k > tgt_lg_k`, which marks the
estimator out-of-order (bulk operation). Overriding that with
`src_out_of_order` can incorrectly leave the result in-order after
downsampling, causing subsequent `update_value` calls to apply HIP updates
starting from an invalid accumulator/register state.
--
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]
