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

insert UInt32 Hashvalue in reverse order on big endian machine #48764

Merged
merged 2 commits into from
Apr 20, 2023

Conversation

SuzyWangIBMer
Copy link
Contributor

Changelog category (leave one):

  • Not for changelog (changelog entry is not required)

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

...

Fix Hashvalue order on big endian machine

Documentation entry for user-facing changes

  • Documentation is written (mandatory for new features)

Information about CI checks: https://clickhouse.com/docs/en/development/continuous-integration/

@robot-ch-test-poll4 robot-ch-test-poll4 added the pr-not-for-changelog This PR should not be mentioned in the changelog label Apr 13, 2023
Copy link
Member

@alexey-milovidov alexey-milovidov left a comment

Choose a reason for hiding this comment

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

If it is needed for serialized state compatibility, let's do it on ser/de, as it will be more efficient.

@alexey-milovidov
Copy link
Member

Please add a comment in the code with the motivation.

@alexey-milovidov alexey-milovidov self-assigned this Apr 14, 2023
@SuzyWangIBMer
Copy link
Contributor Author

If it is needed for serialized state compatibility, let's do it on ser/de, as it will be more efficient.

I am open to any alternative solutions. The problem I see now is that to do it on ser/de, I need to know the index of the hashvalue stored in buf, I can see it is calculated using inline size_t place(HashValue x) const { return (x >> UNIQUES_HASH_BITS_FOR_SKIP) & mask(); }. Not sure if I am allowed to move this method to AggregationFunctionsNull.h. Any suggestion on the implementation?

@alexey-milovidov alexey-milovidov added the can be tested Allows running workflows for external contributors label Apr 17, 2023
Copy link
Member

@alexey-milovidov alexey-milovidov left a comment

Choose a reason for hiding this comment

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

Sounds reasonable, thank you!

@SuzyWangIBMer
Copy link
Contributor Author

Can someone merge this in, please?

@robot-ch-test-poll3 robot-ch-test-poll3 merged commit f32ab26 into ClickHouse:master Apr 20, 2023
138 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
can be tested Allows running workflows for external contributors pr-not-for-changelog This PR should not be mentioned in the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants