Skip to content

Conversation

lmangani
Copy link
Contributor

@lmangani lmangani commented Jan 1, 2023

This PR adds contrib/qxip/hash package exposing basic go hashing functions to flux scripts.
Included readme explains the functionality.

This proposal might require adaptions and is very much intended for discussion and extension as core maintainers see fit.

Done checklist:

  • Documentation
  • Basic Tests

@lmangani lmangani marked this pull request as ready for review January 2, 2023 09:29
@lmangani lmangani requested review from a team as code owners January 2, 2023 09:29
@lmangani lmangani requested review from wolffcm and lwandzura and removed request for a team January 2, 2023 09:29
Copy link
Contributor

@sanderson sanderson left a comment

Choose a reason for hiding this comment

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

@lmangani Looks great. Some minor doc suggestions, but that's it. Thanks!

@lmangani
Copy link
Contributor Author

lmangani commented Jan 3, 2023

Thanks @sanderson for the review and for the doc corrections!
Just to be sure - is the cityhash source include method considered "proper" (as opposed to using the library) for inclusion in Fluxlib and as reference for future contribs with go code?

@lmangani lmangani requested review from sanderson and removed request for wolffcm and lwandzura January 3, 2023 18:53
@wolffcm
Copy link

wolffcm commented Jan 3, 2023

@lmangani Did you copy the code for CityHash instead of using the library for any particular reason? It would be preferable to import it in the normal way, with an import statement and an entry in go.mod.

Copy link

@wolffcm wolffcm left a comment

Choose a reason for hiding this comment

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

I'd like to understand how/if we can import CityHash in the normal way.

@lmangani
Copy link
Contributor Author

lmangani commented Jan 3, 2023

@lmangani Did you copy the code for CityHash instead of using the library for any particular reason? It would be preferable to import it in the normal way, with an import statement and an entry in go.mod.

Just following the contribution guidelines

A third-party package is defined as one that is not part of the standard Go distribution. Generally speaking, we prefer to minimize our use of third-party packages and avoid them unless absolutely necessarily. We'll often write a little bit of code rather than pull in a third-party package. To maximize the chance your change will be accepted, use only the standard libraries, or the third-party packages we have decided to use.

Edit: Generally speaking, it would make sense to import the library if it has any other applications present of future and think having cityhash64/128 in Flux would be quite useful on top of xxhash for high collision use cases, but since this is just a sideload contrib we had no such ambition. Curious to see what you guys think - Thanks for the time!

@wolffcm
Copy link

wolffcm commented Jan 3, 2023

@lmangani This makes sense. Thanks for the rationale there. It's been a while since I've looked at the contribution guidelines.

@wolffcm wolffcm self-requested a review January 3, 2023 20:05
@lmangani
Copy link
Contributor Author

lmangani commented Jan 3, 2023

Thanks! Shall CityHash or any other hashing library become part of the Flux core, I'll update the code to use it/them.

@wolffcm wolffcm merged commit 69e2736 into influxdata:master Jan 3, 2023
@lmangani lmangani deleted the contrib-hash branch January 3, 2023 21:51
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.

3 participants