tisonkun commented on code in PR #91:
URL: https://github.com/apache/datasketches-rust/pull/91#discussion_r2806700237
##########
datasketches/src/bloom/sketch.rs:
##########
@@ -432,27 +430,30 @@ impl BloomFilter {
.map_err(|_| Error::insufficient_data("flags"))?;
// Validate
- if family_id != BLOOM_FAMILY_ID {
- return Err(Error::invalid_family(
- BLOOM_FAMILY_ID,
- family_id,
- "BloomFilter",
- ));
- }
+ Family::BLOOMFILTER.validate_id(family_id)?;
if serial_version != SERIAL_VERSION {
return Err(Error::unsupported_serial_version(
SERIAL_VERSION,
serial_version,
));
}
- if preamble_longs != PREAMBLE_LONGS_EMPTY && preamble_longs !=
PREAMBLE_LONGS_STANDARD {
- return Err(Error::invalid_preamble_longs(
- PREAMBLE_LONGS_STANDARD,
- preamble_longs,
- ));
+ if
!(Family::BLOOMFILTER.min_pre_longs..=Family::BLOOMFILTER.max_pre_longs)
+ .contains(&preamble_longs)
+ {
+ return Err(Error::deserial(format!(
+ "invalid preamble longs: expected [{}, {}], got {}",
+ Family::BLOOMFILTER.min_pre_longs,
+ Family::BLOOMFILTER.max_pre_longs,
+ preamble_longs
+ )));
}
let is_empty = (flags & EMPTY_FLAG_MASK) != 0;
+ if is_empty {
+ debug_assert_eq!(preamble_longs,
Family::BLOOMFILTER.min_pre_longs);
Review Comment:
I've removed these checks since other impls do not have it (yet?).
Logically, it can be a runtime check (and thus throwing errors) on
deserialization. Or else the preamble_longs value is never used.
I'd appreciate it if you can open an issue to coordinate the impl among
langs on this (how to use preamble_longs value of bloomfilter). Or I'll start a
discussion when I find some time later.
--
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]
