Copilot commented on code in PR #294:
URL: https://github.com/apache/datasketches-rust/pull/294#discussion_r4220009288
##########
datasketches/src/hll/estimator.rs:
##########
@@ -182,7 +182,7 @@ impl Estimator {
}
/// Selects between bias-corrected HLL and bitmap estimates using their
empirical crossover.
- fn composite_estimate(&self, lg_config_k: u8, cur_min: u8, num_at_cur_min:
u32) -> f64 {
+ pub fn composite_estimate(&self, lg_config_k: u8, cur_min: u8,
num_at_cur_min: u32) -> f64 {
Review Comment:
Changing this from private to `pub` expands the crate’s public API surface
(and potentially exposes a low-level method intended only for internal use). If
this is only needed by other modules within the crate, prefer `pub(crate)` (or
`pub(super)` depending on layout) to avoid committing to this as a stable
public API.
##########
tests-integration/tests/hll_test/estimate.rs:
##########
@@ -0,0 +1,130 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use datasketches::hll::HllSketch;
+use datasketches::hll::HllType;
+use datasketches::hll::HllUnion;
+use googletest::assert_that;
+use googletest::prelude::near;
+
+const HLL_TYPES: [HllType; 3] = [HllType::Hll4, HllType::Hll6, HllType::Hll8];
+
+fn make_sketch(hll_type: HllType, values: impl Iterator<Item = u64>) ->
HllSketch {
+ let mut sketch = HllSketch::new(11, hll_type).unwrap();
+ for value in values {
+ sketch.update(value);
+ }
+ sketch
+}
+
+#[test]
+fn composite_estimate_matches_coupon_estimate_in_list_and_set_modes() {
+ for hll_type in HLL_TYPES {
+ for n in [0, 3, 100] {
+ let sketch = make_sketch(hll_type, 0..n);
+ assert_eq!(sketch.composite_estimate(), sketch.estimate());
+ }
+ }
+}
Review Comment:
This test assumes `n=100` is always in LIST/SET mode, but if the
implementation’s promotion thresholds change (or vary by `lg_config_k`/type),
the sketch could be in HLL mode and `estimate()` may use HIP, making the
equality assertion fail. To make the test robust, constrain `n` to values
guaranteed to stay in LIST/SET for the chosen configuration, or assert/guard on
the sketch mode before checking equality.
##########
tests-integration/tests/hll_test/estimate.rs:
##########
@@ -0,0 +1,130 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use datasketches::hll::HllSketch;
+use datasketches::hll::HllType;
+use datasketches::hll::HllUnion;
+use googletest::assert_that;
+use googletest::prelude::near;
+
+const HLL_TYPES: [HllType; 3] = [HllType::Hll4, HllType::Hll6, HllType::Hll8];
+
+fn make_sketch(hll_type: HllType, values: impl Iterator<Item = u64>) ->
HllSketch {
+ let mut sketch = HllSketch::new(11, hll_type).unwrap();
+ for value in values {
+ sketch.update(value);
+ }
+ sketch
+}
+
+#[test]
+fn composite_estimate_matches_coupon_estimate_in_list_and_set_modes() {
+ for hll_type in HLL_TYPES {
+ for n in [0, 3, 100] {
+ let sketch = make_sketch(hll_type, 0..n);
+ assert_eq!(sketch.composite_estimate(), sketch.estimate());
+ }
+ }
+}
+
+#[test]
+fn composite_estimate_is_independent_of_insertion_order() {
+ for hll_type in HLL_TYPES {
+ for n in [1_000, 10_000, 100_000] {
+ let forward = make_sketch(hll_type, 0..n);
+ let reverse = make_sketch(hll_type, (0..n).rev());
+ assert_ne!(forward.estimate(), reverse.estimate());
Review Comment:
These `assert_ne!` checks are inherently flaky: HIP estimates for different
insertion orders can occasionally match exactly, and composite vs HIP can also
sometimes be equal for a given input. The tests already validate the important
invariants (composite is order-independent; calling `composite_estimate()` does
not change HIP estimate/serialization). Recommend removing these `assert_ne!`
assertions or replacing them with assertions about invariants that must hold
deterministically.
##########
tests-integration/tests/hll_test/estimate.rs:
##########
@@ -0,0 +1,130 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use datasketches::hll::HllSketch;
+use datasketches::hll::HllType;
+use datasketches::hll::HllUnion;
+use googletest::assert_that;
+use googletest::prelude::near;
+
+const HLL_TYPES: [HllType; 3] = [HllType::Hll4, HllType::Hll6, HllType::Hll8];
+
+fn make_sketch(hll_type: HllType, values: impl Iterator<Item = u64>) ->
HllSketch {
+ let mut sketch = HllSketch::new(11, hll_type).unwrap();
+ for value in values {
+ sketch.update(value);
+ }
+ sketch
+}
+
+#[test]
+fn composite_estimate_matches_coupon_estimate_in_list_and_set_modes() {
+ for hll_type in HLL_TYPES {
+ for n in [0, 3, 100] {
+ let sketch = make_sketch(hll_type, 0..n);
+ assert_eq!(sketch.composite_estimate(), sketch.estimate());
+ }
+ }
+}
+
+#[test]
+fn composite_estimate_is_independent_of_insertion_order() {
+ for hll_type in HLL_TYPES {
+ for n in [1_000, 10_000, 100_000] {
+ let forward = make_sketch(hll_type, 0..n);
+ let reverse = make_sketch(hll_type, (0..n).rev());
+ assert_ne!(forward.estimate(), reverse.estimate());
+ assert_that!(
+ forward.composite_estimate(),
+ near(reverse.composite_estimate(), 1e-9),
+ "type={hll_type:?}, n={n}"
+ );
+ }
+ }
+}
+
+#[test]
+fn composite_estimate_matches_estimate_after_register_merge() {
+ for hll_type in HLL_TYPES {
+ for n in [1_000, 10_000, 100_000] {
+ let sketch = make_sketch(hll_type, 0..n);
+ let mut union = HllUnion::new(sketch.lg_config_k()).unwrap();
+ union.update(&sketch);
+ union.update(&sketch);
+ assert_that!(sketch.composite_estimate(), near(union.estimate(),
1e-9));
+ assert_eq!(union.composite_estimate(), union.estimate());
+ for result_type in HLL_TYPES {
+ assert_that!(
+ union.to_sketch(result_type).composite_estimate(),
+ near(union.estimate(), 1e-9)
+ );
+ }
+ }
+ }
+}
+
+#[test]
+fn composite_estimate_preserves_hip_state_and_subsequent_updates() {
+ for hll_type in HLL_TYPES {
+ let mut sketch = make_sketch(hll_type, 0..10_000);
+ let mut control = sketch.clone();
+ let estimate = sketch.estimate();
+ let bytes = sketch.serialize();
+ assert_ne!(sketch.composite_estimate(), estimate);
Review Comment:
These `assert_ne!` checks are inherently flaky: HIP estimates for different
insertion orders can occasionally match exactly, and composite vs HIP can also
sometimes be equal for a given input. The tests already validate the important
invariants (composite is order-independent; calling `composite_estimate()` does
not change HIP estimate/serialization). Recommend removing these `assert_ne!`
assertions or replacing them with assertions about invariants that must hold
deterministically.
--
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]
