Skip to content

feat: add Display summaries for HLL and CPC - #232

Open
zhangxinyao88 wants to merge 1 commit into
apache:mainfrom
zhangxinyao88:feat/display-cardinality-sketches
Open

feat: add Display summaries for HLL and CPC#232
zhangxinyao88 wants to merge 1 commit into
apache:mainfrom
zhangxinyao88:feat/display-cardinality-sketches

Conversation

@zhangxinyao88

Copy link
Copy Markdown

Part of #212.

Summary

  • implement Display for HllSketch, HllUnion, CpcSketch, and CpcUnion
  • keep the output summary-only, without raw register or coupon details
  • add coverage for empty and populated sketches and update the changelog

Example output:

HLL Sketch Summary:
  lg config k       : 12
  target type       : Hll8
  current mode      : List
  lower bound       : 0
  estimate          : 0
  upper bound       : 0
CPC Sketch Summary:
  flavor            : Empty
  lg k              : 11
  merged            : false
  lower bound       : 0
  estimate          : 0
  upper bound       : 0
  num coupons       : 0

Testing

  • cargo x check
  • cargo x test
  • cargo +nightly clippy --tests --all-features --all-targets --workspace -- -D warnings
  • cargo +nightly fmt --all --check
  • cargo +nightly doc --package datasketches --all-features --no-deps

Comment thread datasketches/src/cpc/sketch.rs Outdated
Comment on lines +477 to +483
let flavor = match self.flavor() {
Flavor::Empty => "Empty",
Flavor::Sparse => "Sparse",
Flavor::Hybrid => "Hybrid",
Flavor::Pinned => "Pinned",
Flavor::Sliding => "Sliding",
};

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can either just use the Debug impl for flavor here or define a pub const fn as_str for Flavor`.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated this to use the existing Debug implementation for Flavor.

Comment thread datasketches/src/cpc/sketch.rs Outdated
writeln!(
f,
" lower bound : {}",
self.lower_bound(NumStdDev::One)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why NumStdDev::One? Could you point out the referenced implementation?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. The Java HLL summary uses one standard deviation, but the Java CPC summary does not include bounds. I removed the CPC lower and upper bounds instead of introducing that choice here.

Comment thread datasketches/src/cpc/sketch.rs Outdated
Comment on lines +497 to +498
" upper bound : {}",
self.upper_bound(NumStdDev::One)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed this together with the lower bound, since the Java CPC summary does not include either bound.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You may use insta for snapshot testing.

However, I have more to consider here now. Let me comment on the issue.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the exact Display assertions to use insta inline snapshots. I kept the populated cases as focused assertions so we do not snapshot estimator values unnecessarily.

@zhangxinyao88
zhangxinyao88 force-pushed the feat/display-cardinality-sketches branch from 326405e to 1c3e45b Compare September 1, 2026 00:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants