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

GH-44767: [C++] Fix Float16.To{Little,Big}Endian on big endian machines #44768

Merged
merged 1 commit into from
Nov 18, 2024

Conversation

QuLogic
Copy link
Contributor

@QuLogic QuLogic commented Nov 18, 2024

Rationale for this change

See issue.

What changes are included in this PR?

For ToLittleEndian/ToBigEndian, the result should always be in the specified endianness, not depend on host order.

In the test, instead of casting the uint8_t data into a uint16_t (with unspecified endianness handling), compare the bytes directly in their expected orders.

Are these changes tested?

Tested on little-endian, still building for big-endian.

Are there any user-facing changes?

Fixes #44767

For `ToLittleEndian`/`ToBigEndian`, the result should always be in the
specified endianness, not depend on host order.

In the test ,istead of casting the `uint8_t` data into a `uint16_t`
(with unspecified endianness handling), compare the bytes directly in
their expected orders.
Copy link

⚠️ GitHub issue #44767 has been automatically assigned in GitHub to PR creator.

@QuLogic
Copy link
Contributor Author

QuLogic commented Nov 18, 2024

Confirmed to pass on a big-endian machine as well.

@QuLogic QuLogic marked this pull request as ready for review November 18, 2024 09:43
@github-actions github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Nov 18, 2024
Copy link
Member

@pitrou pitrou left a comment

Choose a reason for hiding this comment

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

Thanks a lot @QuLogic . For the record, did you try running the entire test suite on a big-endian machine?

@pitrou pitrou merged commit 59decc3 into apache:main Nov 18, 2024
40 of 41 checks passed
@pitrou pitrou removed the awaiting committer review Awaiting committer review label Nov 18, 2024
Copy link

After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit 59decc3.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 1 possible false positive for unstable benchmarks that are known to sometimes produce them.

@QuLogic QuLogic deleted the big-endian-float16 branch November 18, 2024 22:33
@QuLogic
Copy link
Contributor Author

QuLogic commented Nov 19, 2024

For the record, did you try running the entire test suite on a big-endian machine?

Yes, I've run them all, but there are still several failures to go:

90% tests passed, 9 tests failed out of 89

The following tests FAILED:
	 29 - arrow-compute-aggregate-test (Failed)
	 68 - arrow-dataset-file-parquet-test (Failed)
	 73 - arrow-flight-test (Failed)
	 76 - arrow-ipc-read-write-test (Failed)
	 81 - parquet-internals-test (Failed)
	 82 - parquet-reader-test (Failed)
	 84 - parquet-arrow-test (Failed)
	 85 - parquet-arrow-internals-test (Failed)
	 87 - parquet-encryption-key-management-test (Failed)

@pitrou
Copy link
Member

pitrou commented Nov 19, 2024

@QuLogic Thanks. Parquet failures are expected unfortunately, as the work of making Parquet C++ big endian-compatible has not been done.

The IPC and compute failures are a bit more worrying.

For the record, are you working for a company that provides big endian systems?

@QuLogic
Copy link
Contributor Author

QuLogic commented Nov 19, 2024

Oops, I forgot to set the test environment variables; there's no arrow-flight-test failure:

91% tests passed, 8 tests failed out of 89

The following tests FAILED:
	 29 - arrow-compute-aggregate-test (Failed)
	 68 - arrow-dataset-file-parquet-test (Failed)
	 69 - arrow-dataset-file-parquet-encryption-test (Failed)
	 81 - parquet-internals-test (Failed)
	 82 - parquet-reader-test (Failed)
	 84 - parquet-arrow-test (Failed)
	 85 - parquet-arrow-internals-test (Failed)
	 87 - parquet-encryption-key-management-test (Failed)

For the record, are you working for a company that provides big endian systems?

No, I'm trying to fix geopandas on Fedora s390x.

@pitrou
Copy link
Member

pitrou commented Nov 19, 2024

Could you run arrow-compute-aggregate-test in verbose mode and open a new issue with the failures perhaps?

@QuLogic
Copy link
Contributor Author

QuLogic commented Nov 19, 2024

Ah, those are #12681 (comment)

@pitrou
Copy link
Member

pitrou commented Nov 19, 2024

Ah, thanks. These are minor precision issues in the test, it seems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[C++] Float16.{ToBytes,FromBytes} fail on big-endian machines
3 participants