Skip to content

Fix count-min sketches test snapshots should use binary mode#505

Open
tisonkun wants to merge 2 commits into
masterfrom
count-min-binary-output
Open

Fix count-min sketches test snapshots should use binary mode#505
tisonkun wants to merge 2 commits into
masterfrom
count-min-binary-output

Conversation

@tisonkun

Copy link
Copy Markdown
Member

... otherwise \n would be \r\n on Windows and cause apache/datasketches-rust#92 to fail.

Signed-off-by: tison <wander4096@gmail.com>
@tisonkun tisonkun requested review from Copilot, leerho and proost June 24, 2026 18:42
@tisonkun

Copy link
Copy Markdown
Member Author

Seems other snapshot generations already use binary mode.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR ensures Count-Min sketch test snapshot files are written in binary mode so serialized output is consistent across platforms (notably avoiding CRLF translation on Windows).

Changes:

  • Open snapshot std::ofstream streams with std::ios::binary for the empty and non-empty serialization tests.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@PsiACE

PsiACE commented Jun 24, 2026

Copy link
Copy Markdown
Member

since it looks like all the other files end with ".sk" , only count min with .bin

@coveralls

coveralls commented Jun 24, 2026

Copy link
Copy Markdown

Coverage Report for CI Build 28152728714

Coverage remained the same at 82.199%

Details

  • Coverage remained the same as the base build.
  • Patch coverage: No coverable lines changed in this PR.
  • No coverage regressions found.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

No coverage regressions found.


Coverage Stats

Coverage Status
Relevant Lines: 21476
Covered Lines: 17653
Line Coverage: 82.2%
Coverage Strength: 1354329.76 hits per line

💛 - Coveralls

@tisonkun

Copy link
Copy Markdown
Member Author

since it looks like all the other files end with ".sk" , only count min with .bin

Good point. As other impls may also depend on the name, I'd sync with @leerho @proost @freakyzoidberg first to make the unify if any.

@proost proost 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.

Thank you! I agree with changing file extension to ".sk".

@tisonkun

Copy link
Copy Markdown
Member Author

Thank you! I agree with changing file extension to ".sk".

I agree with this. I can update it in this PR or do it in a follow-up.

One possible impact is that datasketches-java and datasketches-go, IIRC, test against these snapshot files. So they may be affected by the name changes as well. But for sure we can update there cascadingly, just need to inform all the related devs.

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

Copy link
Copy Markdown
Member Author

Pushed 7c993ed for the rename.

I'm going to merge this patch later today or tomorrow and fix any impact downstream.

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.

5 participants