Skip to content

refactor: centralize seed hash validation - #183

Merged
tisonkun merged 4 commits into
apache:mainfrom
jaideeppyne:refactor/seed-hash-check
Aug 6, 2026
Merged

refactor: centralize seed hash validation#183
tisonkun merged 4 commits into
apache:mainfrom
jaideeppyne:refactor/seed-hash-check

Conversation

@jaideeppyne

Copy link
Copy Markdown
Contributor

Closes #182.

What changed

  • add one internal check_seed_hash helper that computes the expected hash once and preserves the existing invalid-data error
  • use it in the Count-Min, CPC, compact Theta (v1/v2/v3), and compact Tuple deserialization paths
  • remove 41 lines of repeated validation while keeping the helper crate-private

Validation

  • cargo x prepare-testdata — passed (645 cross-language snapshots prepared)
  • cargo x test — passed
  • nightly Clippy with -D warnings — passed
  • nightly rustfmt check — passed
  • rustdoc with warnings denied — passed
  • Taplo format check and typos — passed

cargo x lint reached and passed all repository/code checks above, but its final local-tool bootstrap could not install hawkeye 6.5.1 with the repository's Rust 1.86 toolchain because current transitive crates require Rust 1.88+; no source/license finding was reported before that environment-only bootstrap failure.

Risk and rollback

This is an internal refactor with no public API or serialized-format change. The main risk is an accidentally changed error kind/message; the helper retains ErrorKind::InvalidData through Error::deserial and the prior message text. Reverting commit 0d6570b fully rolls it back.

AI assistance

OpenAI Codex assisted with implementation and validation. I reviewed the diff and test results.

@tisonkun tisonkun left a comment

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.

Different checks may return different error.

Perhaps a check_seed_hash(expect, actual, |expect, actual|...) help. I don't know.

@jaideeppyne

Copy link
Copy Markdown
Contributor Author

Thanks — updated in 6b193ad. check_seed_hash now accepts an error-producing closure over (expected, actual), so each caller retains control of its error contract. In particular, the CPC path still uses its prior explicit Error::new(ErrorKind::InvalidData, ...) form, while the deserialization paths retain Error::deserial(...); the helper only owns the equality check. This also keeps the mismatch path to one seed-hash computation.

Validation after the change:

  • cargo x test — passed
  • cargo +nightly clippy --tests --all-features --all-targets --workspace -- -D warnings — passed
  • cargo +nightly fmt --all --check — passed
  • RUSTDOCFLAGS="--cfg docsrs -D warnings" cargo +nightly doc --package datasketches --all-features --no-deps — passed

Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>

@tisonkun tisonkun left a comment

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.

Fixed up. LGTM.

@tisonkun
tisonkun marked this pull request as ready for review August 6, 2026 03:01
@tisonkun
tisonkun enabled auto-merge (squash) August 6, 2026 03:01
@tisonkun
tisonkun merged commit 7d433d8 into apache:main Aug 6, 2026
10 checks passed
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.

Check seed_hash can be a helper function

2 participants