Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add murmur3 to hash module #122

Merged
merged 1 commit into from
Sep 4, 2024
Merged

Add murmur3 to hash module #122

merged 1 commit into from
Sep 4, 2024

Conversation

aadi-stripe
Copy link
Contributor

@aadi-stripe aadi-stripe commented Sep 3, 2024

As per the description, we wanted to use murmur3, hence we'd like to add it to the hash module. We are experimenting with it internally at Stripe for some CI tooling, given its known better performance for fair distribution of a small set of inputs.

@aadi-stripe aadi-stripe force-pushed the aadi/test-murmur branch 2 times, most recently from 4368b14 to 422768a Compare September 3, 2024 21:03
@aadi-stripe aadi-stripe changed the title WIP - Add murmur3 to hash module Add murmur3 to hash module Sep 3, 2024
@aadi-stripe aadi-stripe marked this pull request as ready for review September 3, 2024 21:36
// md5,
// sha1,
// sha256,
// murmur3,

Choose a reason for hiding this comment

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

Indentation looks off here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yeah that's my formatter. Can fix

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

"md5": starlark.NewBuiltin("hash.md5", fnHash(md5.New)),
"sha1": starlark.NewBuiltin("hash.sha1", fnHash(sha1.New)),
"sha256": starlark.NewBuiltin("hash.sha256", fnHash(sha256.New)),
"murmur3_64": starlark.NewBuiltin("hash.murmur3_64", fnHash64(murmur3.New64)),

Choose a reason for hiding this comment

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

murmur3 above but murmur3_64 here. Former seems better to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@aadi-stripe aadi-stripe merged commit 9ab159c into trunk Sep 4, 2024
7 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.

5 participants