Copilot commented on code in PR #118:
URL: https://github.com/apache/datasketches-rust/pull/118#discussion_r3199543771
##########
datasketches/src/hll/mod.rs:
##########
@@ -204,14 +204,17 @@ impl Coupon {
self.0
}
- /// Compute the HLL coupon for a hashable value.
+ /// Compute the HLL coupon for a hash value.
+ ///
+ /// You may use [`hash_value`](crate::hash_value) for generating canonical
hash values, and/or
+ /// being compatible with other datasketches implementations.
///
/// Hashes `v` using MurmurHash3 128-bit and packs the result into a
coupon:
Review Comment:
`Coupon::from_hash` hashes its argument internally, so the doc line "for a
hash value" is misleading (it reads as though `v` is already hashed). Consider
rewording to "for a hashable value" or similar, while keeping the note about
using `hash_value` wrappers for canonicalization.
##########
datasketches/src/theta/sketch.rs:
##########
@@ -105,50 +104,27 @@ impl ThetaSketch {
ThetaSketchBuilder::default()
}
- /// Update the sketch with a hashable value.
+ /// Update the sketch with a hash value.
///
- /// For `f32`/`f64` values, use `update_f32`/`update_f64` instead.
+ /// You may use [`hash_value`](crate::hash_value) for generating canonical
hash values, and/or
+ /// being compatible with other datasketches implementations.
Review Comment:
The docs say this method updates the sketch with a "hash value", but the
implementation still takes an arbitrary `T: Hash` and hashes it internally
(MurmurHash3 in `ThetaHashTable::hash`). Consider changing the wording back to
"hashable value" / "value to be hashed" to avoid implying callers must pass a
pre-hashed value.
##########
datasketches/src/cpc/sketch.rs:
##########
@@ -170,9 +169,27 @@ impl CpcSketch {
self.num_coupons == 0
}
- /// Update the sketch with a hashable value.
+ /// Update the sketch with a hash value.
///
- /// For `f32`/`f64` values, use `update_f32`/`update_f64` instead.
+ /// You may use [`hash_value`](crate::hash_value) for generating canonical
hash values, and/or
+ /// being compatible with other datasketches implementations.
+ ///
Review Comment:
The docs describe `update` as accepting a "hash value", but `update<T:
Hash>` hashes the input internally. This wording is misleading (sounds like the
caller must pass a pre-hashed value); consider describing it as updating with a
hashable value and mentioning `hash_value::Canonical` wrappers for
cross-language compatibility.
##########
datasketches/src/hash_value/mod.rs:
##########
@@ -0,0 +1,64 @@
+// 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.
+
+//! Hashable value wrappers for sketches.
+//!
+//! Sketch update APIs accept any value that implements [`Hash`]. For most
Rust values,
+//! passing the value directly is sufficient. This module provides
[`Canonical`] wrappers for
+//! cases where the input must follow compatible canonicalization rules as
other datasketches
+//! implementation.
Review Comment:
Grammar: "rules as other datasketches implementation" should be
plural/rewritten (e.g., "rules as other datasketches implementations").
--
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]
