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

Update naming of bool template parameters, move internal methods to private #5

Conversation

westonpace
Copy link

This is just a suggestion. It was easier typing this up than trying to communicate everything in a PR comment. Feel free to reject.

I was still having trouble compiling because there were a few places using combine_hashes (I think this is probably a bug in clang because it goes away in clang-11).

Before I suggested COMBINE_HASHES_T but, in retrospect, _T doesn't make sense as it isn't a type. Furthermore, it seems we have some inconsistency between template <bool foo>, template <bool FOO>, and template <bool kFoo> (all three are used in the code base). Based on an old ML discussion I think kFoo is correct so I am suggesting that here. Feel free to pick another style if that appeals to you.

Also, I had been playing around with making hash_fixed and hash_varlen private. This seems to work ok but feel free to ignore if there is a reason these need to be public.

michalursa and others added 2 commits March 6, 2022 23:39
@github-actions
Copy link

Thanks for opening a pull request!

If this is not a minor PR. Could you open an issue for this pull request on JIRA? https://issues.apache.org/jira/browse/ARROW

Opening JIRAs ahead of time contributes to the Openness of the Apache Arrow project.

Then could you also rename pull request title in the following format?

ARROW-${JIRA_ID}: [${COMPONENT}] ${SUMMARY}

or

MINOR: [${COMPONENT}] ${SUMMARY}

See also:

@michalursa michalursa force-pushed the ARROW-14182-hash-join2 branch 2 times, most recently from 21849e2 to 419a14b Compare March 16, 2022 06:49
@westonpace westonpace closed this Mar 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants