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

Fix endian issue in CityHash for s390x #46096

Merged
merged 1 commit into from
Feb 12, 2023
Merged

Fix endian issue in CityHash for s390x #46096

merged 1 commit into from
Feb 12, 2023

Conversation

HarryLeeIBM
Copy link
Contributor

On s390x, 00322_disable_checksumming fails because of the following:
CityHash128 returns same uint128 values on both little endian and big endian machines(after a fix in contrib/cityhash102/src/config.h), however the byte order of low and high 64 bit integers of uint128 are reversed on big endian machines compared with those on little endian machines. It means that verification of the binary checksum data generated in little endian machine will fail on big endian machine. Taking checksum in 00322_disable_checksumming as an example:
In big endian:

0xdb 0x8a 0xe9 0x59 0xf2 0x32 0x74 0x50
0x39 0xc4 0x22 0xfb 0xa7 0x4a 0xc6 0x37

In little endian:

0x50 0x74 0x32 0xf2 0x59 0xe9 0x8a 0xdb
0x37 0xc6 0x4a 0xa7 0xfb 0x22 0xc4 0x39	

The fix does the following:

  • Fix contrib/cityhash102/src/config.h so that it uses big endian version of library on big endian machine.
  • In the script of functional test 00322_disable_checksumming, use checksum generated in big endian machine for big endian machines like s390x.

Changelog category (leave one):

  • Build Improvement

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

Fixed endian issue in CityHash for s390x.

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-clickhouse-ci-1 robot-clickhouse-ci-1 added pr-build Pull request with build/testing/packaging improvement submodule changed At least one submodule changed in this PR. labels Feb 6, 2023
@evillique evillique added the can be tested Allows running workflows for external contributors label Feb 7, 2023
@evillique evillique self-assigned this Feb 7, 2023
@evillique
Copy link
Member

Test failures are unrelated

@evillique evillique merged commit a7603aa into ClickHouse:master Feb 12, 2023
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-build Pull request with build/testing/packaging improvement submodule changed At least one submodule changed in this PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants