GH-48238: [C++] Actually write IPC schema endianness, not host endianness

[pitrou]

Rationale for this change

Schema objects have an endianness, but we were ignoring it when serializing a Schema to IPC, and instead writing out the host's endianness.

Are these changes tested?

Yes, by additional test.

Are there any user-facing changes?

No.

This PR contains a "Critical Fix". (If the changes fix either (a) a security vulnerability, (b) a bug that caused incorrect or invalid data to be produced, or (c) a bug that causes a crash (even when the API contract is upheld), please provide explanation. If not, you can remove this.)

• GitHub Issue: [C++] IPC schema endianness doesn't roundtrip #48238

[pitrou]

@Vishwanatha-HD@kiszk@k8ika0s

[pitrou]

CI failures are unrelated, some of them due to intermittent download failures on the GH infrastructure.

[zanmato1984]

I think we should revise the "Critical Fix" part in the PR description. The code LGTM.

One unrelated question though: How is the endianness handled for an IPC between platforms having different endiannesses?

[emkornfield]

The changes look reasonable, but it would this break the "common" case if a machine is BigEndian when writing data?

[pitrou]

I think we should revise the "Critical Fix" part in the PR description.

Currently, if you write non-native endianness data using IPC, the endianness is stored incorrectly (this could happen if you are just passing through data from an other system, think e.g. a Flight proxy). This means the data will be incorrectly byte-swapped when reading.

One unrelated question though: How is the endianness handled for an IPC between platforms having different endiannesses?

It's optionally normalized on read. Which is why it's important for the information to be recorded correctly...

arrow/cpp/src/arrow/ipc/options.h

Lines 165 to 175 in fa41b26

	/// \brief Whether to convert incoming data to platform-native endianness
	///
	/// If the endianness of the received schema is not equal to platform-native
	/// endianness, then all buffers with endian-sensitive data will be byte-swapped.
	/// This includes the value buffers of numeric types, temporal types, decimal
	/// types, as well as the offset buffers of variable-sized binary and list-like
	/// types.
	///
	/// Endianness conversion is achieved by the RecordBatchFileReader,
	/// RecordBatchStreamReader and StreamDecoder classes.
	bool ensure_native_endian = true;

[pitrou]

The changes look reasonable, but it would this break the "common" case if a machine is BigEndian when writing data?

What do you mean?

[zanmato1984]

LGTM

[emkornfield]

The changes look reasonable, but it would this break the "common" case if a machine is BigEndian when writing data?

What do you mean?

I might have been misunderstaning the code but it looks like this PR removes automatic detection of endianness and relies the the enddianess set by the client of the library. I was wondering if this bit gets set automatically in some other way (and if not if it silently reports data from big-endian machines as little endian to consumers of IPC message.)

[pitrou]

I might have been misunderstaning the code but it looks like this PR removes automatic detection of endianness and relies the the enddianess set by the client of the library.

The endianness is still set to the machine's default in most cases. For example when calling the Schema constructor without passing the corresponding optional argument.

[conbench-apache-arrow]

After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 91f6618.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

[pitrou]

Rationale for this change

Schema objects have an endianness, but we were ignoring it when serializing a Schema to IPC, and instead writing out the host's endianness.

Are these changes tested?

Yes, by additional test.

Are there any user-facing changes?

No.

This PR contains a "Critical Fix". (If the changes fix either (a) a security vulnerability, (b) a bug that caused incorrect or invalid data to be produced, or (c) a bug that causes a crash (even when the API contract is upheld), please provide explanation. If not, you can remove this.)

• GitHub Issue: [C++] IPC schema endianness doesn't roundtrip #48238

[pitrou]

@Vishwanatha-HD@kiszk@k8ika0s

[pitrou]

CI failures are unrelated, some of them due to intermittent download failures on the GH infrastructure.

[zanmato1984]

I think we should revise the "Critical Fix" part in the PR description. The code LGTM.

One unrelated question though: How is the endianness handled for an IPC between platforms having different endiannesses?

[emkornfield]

The changes look reasonable, but it would this break the "common" case if a machine is BigEndian when writing data?

[pitrou]

I think we should revise the "Critical Fix" part in the PR description.

Currently, if you write non-native endianness data using IPC, the endianness is stored incorrectly (this could happen if you are just passing through data from an other system, think e.g. a Flight proxy). This means the data will be incorrectly byte-swapped when reading.

One unrelated question though: How is the endianness handled for an IPC between platforms having different endiannesses?

It's optionally normalized on read. Which is why it's important for the information to be recorded correctly...

arrow/cpp/src/arrow/ipc/options.h

Lines 165 to 175 in fa41b26

	/// \brief Whether to convert incoming data to platform-native endianness
	///
	/// If the endianness of the received schema is not equal to platform-native
	/// endianness, then all buffers with endian-sensitive data will be byte-swapped.
	/// This includes the value buffers of numeric types, temporal types, decimal
	/// types, as well as the offset buffers of variable-sized binary and list-like
	/// types.
	///
	/// Endianness conversion is achieved by the RecordBatchFileReader,
	/// RecordBatchStreamReader and StreamDecoder classes.
	bool ensure_native_endian = true;

[pitrou]

The changes look reasonable, but it would this break the "common" case if a machine is BigEndian when writing data?

What do you mean?

[zanmato1984]

LGTM

[emkornfield]

The changes look reasonable, but it would this break the "common" case if a machine is BigEndian when writing data?

What do you mean?

I might have been misunderstaning the code but it looks like this PR removes automatic detection of endianness and relies the the enddianess set by the client of the library. I was wondering if this bit gets set automatically in some other way (and if not if it silently reports data from big-endian machines as little endian to consumers of IPC message.)

[pitrou]

I might have been misunderstaning the code but it looks like this PR removes automatic detection of endianness and relies the the enddianess set by the client of the library.

The endianness is still set to the machine's default in most cases. For example when calling the Schema constructor without passing the corresponding optional argument.

[conbench-apache-arrow]

After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 91f6618.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

[pitrou]

Rationale for this change

Schema objects have an endianness, but we were ignoring it when serializing a Schema to IPC, and instead writing out the host's endianness.

Are these changes tested?

Yes, by additional test.

Are there any user-facing changes?

No.

This PR contains a "Critical Fix". (If the changes fix either (a) a security vulnerability, (b) a bug that caused incorrect or invalid data to be produced, or (c) a bug that causes a crash (even when the API contract is upheld), please provide explanation. If not, you can remove this.)

• GitHub Issue: [C++] IPC schema endianness doesn't roundtrip #48238

[pitrou]

@Vishwanatha-HD@kiszk@k8ika0s

[pitrou]

CI failures are unrelated, some of them due to intermittent download failures on the GH infrastructure.

[zanmato1984]

I think we should revise the "Critical Fix" part in the PR description. The code LGTM.

One unrelated question though: How is the endianness handled for an IPC between platforms having different endiannesses?

[emkornfield]

The changes look reasonable, but it would this break the "common" case if a machine is BigEndian when writing data?

[pitrou]

I think we should revise the "Critical Fix" part in the PR description.

Currently, if you write non-native endianness data using IPC, the endianness is stored incorrectly (this could happen if you are just passing through data from an other system, think e.g. a Flight proxy). This means the data will be incorrectly byte-swapped when reading.

One unrelated question though: How is the endianness handled for an IPC between platforms having different endiannesses?

It's optionally normalized on read. Which is why it's important for the information to be recorded correctly...

arrow/cpp/src/arrow/ipc/options.h

Lines 165 to 175 in fa41b26

	/// \brief Whether to convert incoming data to platform-native endianness
	///
	/// If the endianness of the received schema is not equal to platform-native
	/// endianness, then all buffers with endian-sensitive data will be byte-swapped.
	/// This includes the value buffers of numeric types, temporal types, decimal
	/// types, as well as the offset buffers of variable-sized binary and list-like
	/// types.
	///
	/// Endianness conversion is achieved by the RecordBatchFileReader,
	/// RecordBatchStreamReader and StreamDecoder classes.
	bool ensure_native_endian = true;

[pitrou]

The changes look reasonable, but it would this break the "common" case if a machine is BigEndian when writing data?

What do you mean?

[zanmato1984]

LGTM

[emkornfield]

The changes look reasonable, but it would this break the "common" case if a machine is BigEndian when writing data?

What do you mean?

I might have been misunderstaning the code but it looks like this PR removes automatic detection of endianness and relies the the enddianess set by the client of the library. I was wondering if this bit gets set automatically in some other way (and if not if it silently reports data from big-endian machines as little endian to consumers of IPC message.)

[pitrou]

I might have been misunderstaning the code but it looks like this PR removes automatic detection of endianness and relies the the enddianess set by the client of the library.

The endianness is still set to the machine's default in most cases. For example when calling the Schema constructor without passing the corresponding optional argument.

[conbench-apache-arrow]

After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 91f6618.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

[pitrou]

Rationale for this change

Schema objects have an endianness, but we were ignoring it when serializing a Schema to IPC, and instead writing out the host's endianness.

Are these changes tested?

Yes, by additional test.

Are there any user-facing changes?

No.

This PR contains a "Critical Fix". (If the changes fix either (a) a security vulnerability, (b) a bug that caused incorrect or invalid data to be produced, or (c) a bug that causes a crash (even when the API contract is upheld), please provide explanation. If not, you can remove this.)

• GitHub Issue: [C++] IPC schema endianness doesn't roundtrip #48238

[pitrou]

@Vishwanatha-HD@kiszk@k8ika0s

[pitrou]

CI failures are unrelated, some of them due to intermittent download failures on the GH infrastructure.

[zanmato1984]

I think we should revise the "Critical Fix" part in the PR description. The code LGTM.

One unrelated question though: How is the endianness handled for an IPC between platforms having different endiannesses?

[emkornfield]

The changes look reasonable, but it would this break the "common" case if a machine is BigEndian when writing data?

[pitrou]

I think we should revise the "Critical Fix" part in the PR description.

Currently, if you write non-native endianness data using IPC, the endianness is stored incorrectly (this could happen if you are just passing through data from an other system, think e.g. a Flight proxy). This means the data will be incorrectly byte-swapped when reading.

One unrelated question though: How is the endianness handled for an IPC between platforms having different endiannesses?

It's optionally normalized on read. Which is why it's important for the information to be recorded correctly...

arrow/cpp/src/arrow/ipc/options.h

Lines 165 to 175 in fa41b26

	/// \brief Whether to convert incoming data to platform-native endianness
	///
	/// If the endianness of the received schema is not equal to platform-native
	/// endianness, then all buffers with endian-sensitive data will be byte-swapped.
	/// This includes the value buffers of numeric types, temporal types, decimal
	/// types, as well as the offset buffers of variable-sized binary and list-like
	/// types.
	///
	/// Endianness conversion is achieved by the RecordBatchFileReader,
	/// RecordBatchStreamReader and StreamDecoder classes.
	bool ensure_native_endian = true;

[pitrou]

The changes look reasonable, but it would this break the "common" case if a machine is BigEndian when writing data?

What do you mean?

[zanmato1984]

LGTM

[emkornfield]

The changes look reasonable, but it would this break the "common" case if a machine is BigEndian when writing data?

What do you mean?

I might have been misunderstaning the code but it looks like this PR removes automatic detection of endianness and relies the the enddianess set by the client of the library. I was wondering if this bit gets set automatically in some other way (and if not if it silently reports data from big-endian machines as little endian to consumers of IPC message.)

[pitrou]

I might have been misunderstaning the code but it looks like this PR removes automatic detection of endianness and relies the the enddianess set by the client of the library.

The endianness is still set to the machine's default in most cases. For example when calling the Schema constructor without passing the corresponding optional argument.

[conbench-apache-arrow]

After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 91f6618.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

[pitrou]

Rationale for this change

Schema objects have an endianness, but we were ignoring it when serializing a Schema to IPC, and instead writing out the host's endianness.

Are these changes tested?

Yes, by additional test.

Are there any user-facing changes?

No.

This PR contains a "Critical Fix". (If the changes fix either (a) a security vulnerability, (b) a bug that caused incorrect or invalid data to be produced, or (c) a bug that causes a crash (even when the API contract is upheld), please provide explanation. If not, you can remove this.)

• GitHub Issue: [C++] IPC schema endianness doesn't roundtrip #48238

[pitrou]

@Vishwanatha-HD@kiszk@k8ika0s

[pitrou]

CI failures are unrelated, some of them due to intermittent download failures on the GH infrastructure.

[zanmato1984]

I think we should revise the "Critical Fix" part in the PR description. The code LGTM.

One unrelated question though: How is the endianness handled for an IPC between platforms having different endiannesses?

[emkornfield]

The changes look reasonable, but it would this break the "common" case if a machine is BigEndian when writing data?

[pitrou]

I think we should revise the "Critical Fix" part in the PR description.

Currently, if you write non-native endianness data using IPC, the endianness is stored incorrectly (this could happen if you are just passing through data from an other system, think e.g. a Flight proxy). This means the data will be incorrectly byte-swapped when reading.

One unrelated question though: How is the endianness handled for an IPC between platforms having different endiannesses?

It's optionally normalized on read. Which is why it's important for the information to be recorded correctly...

arrow/cpp/src/arrow/ipc/options.h

Lines 165 to 175 in fa41b26

	/// \brief Whether to convert incoming data to platform-native endianness
	///
	/// If the endianness of the received schema is not equal to platform-native
	/// endianness, then all buffers with endian-sensitive data will be byte-swapped.
	/// This includes the value buffers of numeric types, temporal types, decimal
	/// types, as well as the offset buffers of variable-sized binary and list-like
	/// types.
	///
	/// Endianness conversion is achieved by the RecordBatchFileReader,
	/// RecordBatchStreamReader and StreamDecoder classes.
	bool ensure_native_endian = true;

[pitrou]

The changes look reasonable, but it would this break the "common" case if a machine is BigEndian when writing data?

What do you mean?

[zanmato1984]

LGTM

[emkornfield]

The changes look reasonable, but it would this break the "common" case if a machine is BigEndian when writing data?

What do you mean?

I might have been misunderstaning the code but it looks like this PR removes automatic detection of endianness and relies the the enddianess set by the client of the library. I was wondering if this bit gets set automatically in some other way (and if not if it silently reports data from big-endian machines as little endian to consumers of IPC message.)

[pitrou]

I might have been misunderstaning the code but it looks like this PR removes automatic detection of endianness and relies the the enddianess set by the client of the library.

The endianness is still set to the machine's default in most cases. For example when calling the Schema constructor without passing the corresponding optional argument.

[conbench-apache-arrow]

After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 91f6618.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

[pitrou]

Rationale for this change

Schema objects have an endianness, but we were ignoring it when serializing a Schema to IPC, and instead writing out the host's endianness.

Are these changes tested?

Yes, by additional test.

Are there any user-facing changes?

No.

This PR contains a "Critical Fix". (If the changes fix either (a) a security vulnerability, (b) a bug that caused incorrect or invalid data to be produced, or (c) a bug that causes a crash (even when the API contract is upheld), please provide explanation. If not, you can remove this.)

• GitHub Issue: [C++] IPC schema endianness doesn't roundtrip #48238

[pitrou]

@Vishwanatha-HD@kiszk@k8ika0s

[pitrou]

CI failures are unrelated, some of them due to intermittent download failures on the GH infrastructure.

[zanmato1984]

I think we should revise the "Critical Fix" part in the PR description. The code LGTM.

One unrelated question though: How is the endianness handled for an IPC between platforms having different endiannesses?

[emkornfield]

The changes look reasonable, but it would this break the "common" case if a machine is BigEndian when writing data?

[pitrou]

I think we should revise the "Critical Fix" part in the PR description.

Currently, if you write non-native endianness data using IPC, the endianness is stored incorrectly (this could happen if you are just passing through data from an other system, think e.g. a Flight proxy). This means the data will be incorrectly byte-swapped when reading.

One unrelated question though: How is the endianness handled for an IPC between platforms having different endiannesses?

It's optionally normalized on read. Which is why it's important for the information to be recorded correctly...

arrow/cpp/src/arrow/ipc/options.h

Lines 165 to 175 in fa41b26

	/// \brief Whether to convert incoming data to platform-native endianness
	///
	/// If the endianness of the received schema is not equal to platform-native
	/// endianness, then all buffers with endian-sensitive data will be byte-swapped.
	/// This includes the value buffers of numeric types, temporal types, decimal
	/// types, as well as the offset buffers of variable-sized binary and list-like
	/// types.
	///
	/// Endianness conversion is achieved by the RecordBatchFileReader,
	/// RecordBatchStreamReader and StreamDecoder classes.
	bool ensure_native_endian = true;

[pitrou]

The changes look reasonable, but it would this break the "common" case if a machine is BigEndian when writing data?

What do you mean?

[zanmato1984]

LGTM

[emkornfield]

The changes look reasonable, but it would this break the "common" case if a machine is BigEndian when writing data?

What do you mean?

I might have been misunderstaning the code but it looks like this PR removes automatic detection of endianness and relies the the enddianess set by the client of the library. I was wondering if this bit gets set automatically in some other way (and if not if it silently reports data from big-endian machines as little endian to consumers of IPC message.)

[pitrou]

I might have been misunderstaning the code but it looks like this PR removes automatic detection of endianness and relies the the enddianess set by the client of the library.

The endianness is still set to the machine's default in most cases. For example when calling the Schema constructor without passing the corresponding optional argument.

[conbench-apache-arrow]

After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 91f6618.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

[pitrou]

Rationale for this change

Schema objects have an endianness, but we were ignoring it when serializing a Schema to IPC, and instead writing out the host's endianness.

Are these changes tested?

Yes, by additional test.

Are there any user-facing changes?

No.

This PR contains a "Critical Fix". (If the changes fix either (a) a security vulnerability, (b) a bug that caused incorrect or invalid data to be produced, or (c) a bug that causes a crash (even when the API contract is upheld), please provide explanation. If not, you can remove this.)

• GitHub Issue: [C++] IPC schema endianness doesn't roundtrip #48238

[pitrou]

@Vishwanatha-HD@kiszk@k8ika0s

[pitrou]

CI failures are unrelated, some of them due to intermittent download failures on the GH infrastructure.

[zanmato1984]

I think we should revise the "Critical Fix" part in the PR description. The code LGTM.

One unrelated question though: How is the endianness handled for an IPC between platforms having different endiannesses?

[emkornfield]

The changes look reasonable, but it would this break the "common" case if a machine is BigEndian when writing data?

[pitrou]

I think we should revise the "Critical Fix" part in the PR description.

Currently, if you write non-native endianness data using IPC, the endianness is stored incorrectly (this could happen if you are just passing through data from an other system, think e.g. a Flight proxy). This means the data will be incorrectly byte-swapped when reading.

One unrelated question though: How is the endianness handled for an IPC between platforms having different endiannesses?

It's optionally normalized on read. Which is why it's important for the information to be recorded correctly...

arrow/cpp/src/arrow/ipc/options.h

Lines 165 to 175 in fa41b26

	/// \brief Whether to convert incoming data to platform-native endianness
	///
	/// If the endianness of the received schema is not equal to platform-native
	/// endianness, then all buffers with endian-sensitive data will be byte-swapped.
	/// This includes the value buffers of numeric types, temporal types, decimal
	/// types, as well as the offset buffers of variable-sized binary and list-like
	/// types.
	///
	/// Endianness conversion is achieved by the RecordBatchFileReader,
	/// RecordBatchStreamReader and StreamDecoder classes.
	bool ensure_native_endian = true;

[pitrou]

The changes look reasonable, but it would this break the "common" case if a machine is BigEndian when writing data?

What do you mean?

[zanmato1984]

LGTM

[emkornfield]

The changes look reasonable, but it would this break the "common" case if a machine is BigEndian when writing data?

What do you mean?

I might have been misunderstaning the code but it looks like this PR removes automatic detection of endianness and relies the the enddianess set by the client of the library. I was wondering if this bit gets set automatically in some other way (and if not if it silently reports data from big-endian machines as little endian to consumers of IPC message.)

[pitrou]

I might have been misunderstaning the code but it looks like this PR removes automatic detection of endianness and relies the the enddianess set by the client of the library.

The endianness is still set to the machine's default in most cases. For example when calling the Schema constructor without passing the corresponding optional argument.

[conbench-apache-arrow]

After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 91f6618.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

[pitrou]

Rationale for this change

Schema objects have an endianness, but we were ignoring it when serializing a Schema to IPC, and instead writing out the host's endianness.

Are these changes tested?

Yes, by additional test.

Are there any user-facing changes?

No.

This PR contains a "Critical Fix". (If the changes fix either (a) a security vulnerability, (b) a bug that caused incorrect or invalid data to be produced, or (c) a bug that causes a crash (even when the API contract is upheld), please provide explanation. If not, you can remove this.)

• GitHub Issue: [C++] IPC schema endianness doesn't roundtrip #48238

[pitrou]

@Vishwanatha-HD@kiszk@k8ika0s

[pitrou]

CI failures are unrelated, some of them due to intermittent download failures on the GH infrastructure.

[zanmato1984]

I think we should revise the "Critical Fix" part in the PR description. The code LGTM.

One unrelated question though: How is the endianness handled for an IPC between platforms having different endiannesses?

[emkornfield]

The changes look reasonable, but it would this break the "common" case if a machine is BigEndian when writing data?

[pitrou]

I think we should revise the "Critical Fix" part in the PR description.

Currently, if you write non-native endianness data using IPC, the endianness is stored incorrectly (this could happen if you are just passing through data from an other system, think e.g. a Flight proxy). This means the data will be incorrectly byte-swapped when reading.

One unrelated question though: How is the endianness handled for an IPC between platforms having different endiannesses?

It's optionally normalized on read. Which is why it's important for the information to be recorded correctly...

arrow/cpp/src/arrow/ipc/options.h

Lines 165 to 175 in fa41b26

	/// \brief Whether to convert incoming data to platform-native endianness
	///
	/// If the endianness of the received schema is not equal to platform-native
	/// endianness, then all buffers with endian-sensitive data will be byte-swapped.
	/// This includes the value buffers of numeric types, temporal types, decimal
	/// types, as well as the offset buffers of variable-sized binary and list-like
	/// types.
	///
	/// Endianness conversion is achieved by the RecordBatchFileReader,
	/// RecordBatchStreamReader and StreamDecoder classes.
	bool ensure_native_endian = true;

[pitrou]

The changes look reasonable, but it would this break the "common" case if a machine is BigEndian when writing data?

What do you mean?

[zanmato1984]

LGTM

[emkornfield]

The changes look reasonable, but it would this break the "common" case if a machine is BigEndian when writing data?

What do you mean?

I might have been misunderstaning the code but it looks like this PR removes automatic detection of endianness and relies the the enddianess set by the client of the library. I was wondering if this bit gets set automatically in some other way (and if not if it silently reports data from big-endian machines as little endian to consumers of IPC message.)

[pitrou]

I might have been misunderstaning the code but it looks like this PR removes automatic detection of endianness and relies the the enddianess set by the client of the library.

The endianness is still set to the machine's default in most cases. For example when calling the Schema constructor without passing the corresponding optional argument.

[conbench-apache-arrow]

After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 91f6618.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.