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 partition ID byte order for s390x #47769

Merged
merged 1 commit into from
Mar 21, 2023

Conversation

HarryLeeIBM
Copy link
Contributor

@HarryLeeIBM HarryLeeIBM commented Mar 20, 2023

On s390x, the partition ID has reversed byte order compared to those on little-endian machines. The fix is to reverse the byte order so that partition IDs keep the same on both little-endian and big-endian machines.

Changelog category (leave one):

  • Not for changelog

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

Fixed the byte order of partition IDs on s390x so that partition IDs keep the same on both little-endian and big-endian machines. This change does not affect other platforms.

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 robot-clickhouse added the pr-build Pull request with build/testing/packaging improvement label Mar 20, 2023
@@ -261,8 +261,11 @@ String MergeTreePartition::getID(const Block & partition_key_sample) const
hash.get128(hash_data);
result.resize(32);
for (size_t i = 0; i < 16; ++i)
#if __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
Copy link
Member

Choose a reason for hiding this comment

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

if constexpr (std::endian::native == std::endian::big) would be nicer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if constexpr (std::endian::native == std::endian::big) only works when it is in a template function. Since getID() is not a template function, using this way would cause compiling error.

@tavplubix tavplubix self-assigned this Mar 20, 2023
@tavplubix
Copy link
Member

Also let's change the changelog category to "Backward Incompatible Change" and make the changelog entry more detailed

@tavplubix
Copy link
Member

And do we have the same issue with other usages of writeHexByteLowercase and writeHexByteUppercase? For example, here:

String salt;
salt.resize(sizeof(key) * 2);
char * buf_pos = salt.data();
for (uint8_t k : key)
{
writeHexByteUppercase(k, buf_pos);
buf_pos += 2;
}
value.append(salt);
auth_data.setSalt(salt);

@robot-ch-test-poll4 robot-ch-test-poll4 added pr-backward-incompatible Pull request with backwards incompatible changes and removed pr-build Pull request with build/testing/packaging improvement labels Mar 20, 2023
@HarryLeeIBM
Copy link
Contributor Author

And do we have the same issue with other usages of writeHexByteLowercase and writeHexByteUppercase? For example, here:

String salt;
salt.resize(sizeof(key) * 2);
char * buf_pos = salt.data();
for (uint8_t k : key)
{
writeHexByteUppercase(k, buf_pos);
buf_pos += 2;
}
value.append(salt);
auth_data.setSalt(salt);

This issue is unique because getID() uses SipHash128 to generate 128 bit wide integers for the partition ID. Even though the generated 128 bit integers are the same for both little-endian and big-endian machines, directly using the data of 128 bit integers would cause byte order issue in s390x.

@HarryLeeIBM
Copy link
Contributor Author

Also let's change the changelog category to "Backward Incompatible Change" and make the changelog entry more detailed

Done.

@tavplubix tavplubix added the can be tested Allows running workflows for external contributors label Mar 20, 2023
@tavplubix
Copy link
Member

Integration tests (tsan) [5/6] - #47839
Stateless tests (debug, s3 storage) [6/6] - #47866

@tavplubix tavplubix merged commit 8a71e42 into ClickHouse:master Mar 21, 2023
@alexey-milovidov
Copy link
Member

This is not a backward incompatible change.

@alexey-milovidov alexey-milovidov removed the pr-backward-incompatible Pull request with backwards incompatible changes label Mar 27, 2023
@tavplubix
Copy link
Member

This is not a backward incompatible change.

@alexey-milovidov, why? It changes partition ids on s390x platform. Of course, it does not affect other platforms, but it is backward incompatible for s390x.

1 similar comment
@tavplubix
Copy link
Member

This is not a backward incompatible change.

@alexey-milovidov, why? It changes partition ids on s390x platform. Of course, it does not affect other platforms, but it is backward incompatible for s390x.

@tavplubix tavplubix added the pr-backward-incompatible Pull request with backwards incompatible changes label Mar 27, 2023
@alexey-milovidov alexey-milovidov removed the pr-backward-incompatible Pull request with backwards incompatible changes label Mar 27, 2023
@alexey-milovidov
Copy link
Member

s390x is not supported for ClickHouse.

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants