GH-48176: [C++][Parquet] Fix arrow-ipc-message-internal-test failure

[Vishwanatha-HD]

Rationale for this change

Fix arrow-ipc-message-internal-test on big endian systems by passing endianness explicitly.

Are these changes tested?

Yes. The changes are tested on s390x arch to make sure things are working fine. The fix is also tested on x86 arch, to make sure there is no new regression introduced.

Are there any user-facing changes?

No

• GitHub main Issue link: [C++][Parquet] Enable Parquet DB support on Big Endian (IBM Z) systems #48151
• GitHub Issue: [C++][Parquet] Fix arrow-ipc-message-internal-test failure on Big-Endian (s390x) systems #48176

[github-actions]

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

[kou]

Could you fix lint failure?

[kou]

Why do we need this check? This check didn't exist in the existing code.

[Vishwanatha-HD]

@kou,
Please find the root cause of this issue on s390x platform below:

The test was failing with a std::bad_cast exception in the ProbeBatch_ResidualFilter function in hash_join.cc at line 309. The issue occurred when the code tried to cast a Datum's scalar to a BooleanScalar using:

const auto& mask_scalar = mask.scalar_as();

The problem was that the test uses literal(NullScalar()) as one of the filter expressions, which creates a NullScalar (of type NullType), not a BooleanScalar (of type BoolType). When the code tried to cast this to a BooleanScalar, it failed with std::bad_cast on s390x architecture

The Fix
I modified the ProbeBatch_ResidualFilter function to check the scalar's type before attempting the cast

[kiszk]

@Vishwanatha-HD I think that this change makes sense for both platforms: big and little endians. We do not need to use #if ... #else ... #endif.

Or, should we fix BooleanScalar to avoid throwing std::bad_cast? cc @pitrou

[zanmato1984]

This is a very detailed explanation. Thanks @Vishwanatha-HD .

As the author of the questioning test, I can say this is the issue of the test rather than of the hash join code. That is, using an filter expression evaluating to null-type null is invalid - it should be a boolean null. The hash join code itself arbitrarily assuming the expression being boolean is OK, though a DCHECK_EQ(mask.type()->id(), Type::BOOL) would be more preferable.

I think we should in turn fix the test by simply replacing the literal(NullScalar()) with a boolean null - literal(MakeNullScalar(boolean()))

[zanmato1984]

But one question remains: The test should be equally problematic for little-endian as well. Why is it passing?

I'm now looking into it.

[zanmato1984]

OK, mystery solved: there are two implementations of hash join - regular and swiss. The regular one has wider range of supported types and platforms (including big-endian). The swiss one is more efficient, but restrictive in supported types and platform (only little-endian). So on big endian platform, the regular one is chosen which is more strict on filter expression type. Little-endian platform uses the swiss one which is less strict on filter expression type by explicitly allowing a null-type null - so our CI, always ran on little-endian platforms, has always been passing.

I'm the author of supporting residual filter in swiss join as well as the tests. I think allowing null-type null was a mistake (it was implemented at the beginning of my contribution to Arrow - and I was very naive then).

I think I can send out an issue/pr soon to address the issues discovered here:

1. The test in question needs to be revised by replacing null-type null with boolean null, as mentioned in my previous comment (so it will pass on big-endian platforms);
2. The swiss join code need to be updated by not allowing null-type.
3. Add DCHECKs to enforce the filter expression checking. (optional)

[zanmato1984]

I filed #48268 to address this problem and I'll work on a PR very soon. Once it's fixed, there shouldn't be failures of this test on big-endian platforms. So as I mentioned before, this part of change is not need (and not quite correct). Thanks.

[zanmato1984]

Should be fixed by #48272 . @Vishwanatha-HD Could you first revert the change to hash_join.cc then rebase main to see if the hash join test is passing on big-endian? Thanks.

[Vishwanatha-HD]

@zanmato1984.. I tried reverting my changes done to hash_join.cc.. With the latest main, the testcase is passing on the big-endian machine..

[zanmato1984]

That's great, thanks for the update!

[Vishwanatha-HD]

Could you fix lint failure?

I have fixed the lint failures. Thanks..

[Vishwanatha-HD]

I have taken care of the review comments and made necessary changes to the code. Pls review my changes. Thanks..

[kiszk]

Could you remove [Parquet] from the title? I think that this is not a Parquet-specific issue.

[pitrou]

Er, ok, but there is a well-defined explanation to this: a Arrow schema has an endianness attached to it, and it defaults to the platform's endianness.

To obey the intent of this test case, you should instead change the schema instantiation above to:

auto schema = ::arrow::schema({f0}, Endianness::Little, metadata);

Does this change solve the issue on s390x?

[Vishwanatha-HD]

@pitrou.. Thanks for your review comments. But this change didnt quite resolve the issue. Thanks..

[pitrou]

@Vishwanatha-HD You're right, but that should be fixed once #48239 is merged. So I would recommend waiting for that.

[Vishwanatha-HD]

@pitrou.. With the #48239 getting merged into main, I tried making the changes to schema as you suggested above, but the testcase is not passing on s390x..

./debug/arrow-ipc-message-internal-test --gtest_filter="TestMessageInternal.TestByteIdentical"
Running main() from gmock_main.cc
Note: Google Test filter = TestMessageInternal.TestByteIdentical
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from TestMessageInternal
[ RUN ] TestMessageInternal.TestByteIdentical
arrow/cpp/src/arrow/testing/gtest_util.cc:241: Failure
Expected equality of these values:
buffer.size()
Which is: 228
expected.size()
Which is: 232
Mismatching buffer size

[ FAILED ] TestMessageInternal.TestByteIdentical (1 ms)
[----------] 1 test from TestMessageInternal (1 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test suite ran. (1 ms total)
[ PASSED ] 0 tests.
[ FAILED ] 1 test, listed below:
[ FAILED ] TestMessageInternal.TestByteIdentical

1 FAILED TEST

[pitrou]

@Vishwanatha-HD You must also undo your own changes.

[pitrou]

I think it's enough to only test Endianness::Little. Roundtripping is tested elsewhere.

[Vishwanatha-HD]

Ok.. sure.. I will make the necessary changes and push the code again.. Thanks for your comments @pitrou.. !!

[Vishwanatha-HD]

@pitrou, as per our discussion yesterday, I have made the necessary changes to message_internal_test.cc file to only test Endianness::Little.. I have reverted the code that I added for BE architecture. Thanks..

[pitrou]

@Vishwanatha-HD Are you sure you pushed your changes? I see lots of CI failures.

[Vishwanatha-HD]

@pitrou.. Apologies.. It was my bad that I had both Endianness in the for loop.. I have made the necessary code changes and pushed the changes again.. Thanks for pointing this out.. !!

[pitrou]

Also see #48239

[Vishwanatha-HD]

Could you remove [Parquet] from the title? I think that this is not a Parquet-specific issue.

Hi @kiszk.. My idea of having the Parquet is that the overall changes are intended for Parquet component and in order to enable support to Parquet DB on s390x..

[kiszk]

Could you remove [Parquet] from the title? I think that this is not a Parquet-specific issue.

Hi @kiszk.. My idea of having the Parquet is that the overall changes are intended for Parquet component and in order to enable support to Parquet DB on s390x..

I understand your idea. On the other hand, this test case is not for Parquet. The current title may confuse others.

[Vishwanatha-HD]

I have addressed all the review comments. Thanks.

[pitrou]

Hey @Vishwanatha-HD I have two requests:

1. can you rebase/merge the latest git main into this?
2. Please don't mark discussions as resolved as soon as you have answered them; they should only be marked resolved if there is common agreement that there's nothing to add

[Vishwanatha-HD]

Hey @Vishwanatha-HD I have two requests:

1. can you rebase/merge the latest git main into this?
2. Please don't mark discussions as resolved as soon as you have answered them; they should only be marked resolved if there is common agreement that there's nothing to add

Hi @pitrou.. Sure.. I will do the rebase with the latest git main and get the latest changes into this..
Also, I am sorry for marking the discussions resolved, as I thought I should close it as soon as I work on it & resolve it..

[Vishwanatha-HD]

Hey @Vishwanatha-HD I have two requests:

1. can you rebase/merge the latest git main into this?
2. Please don't mark discussions as resolved as soon as you have answered them; they should only be marked resolved if there is common agreement that there's nothing to add

Hi @pitrou.. Sure.. I will do the rebase with the latest git main and get the latest changes into this.. Also, I am sorry for marking the discussions resolved, as I thought I should close it as soon as I work on it & resolve it..

@pitrou.. I did the git rebase to merge the latest git main changes into this..

[Vishwanatha-HD]

With the changes done as part of #48272, the latest main works fine without the hash_join.cc changes on big-endian architecture.. Hence reverting the changes done as part of this PR..

[pitrou]

Also, please fix the PR title and description to match the PR contents.

[Vishwanatha-HD]

Also, please fix the PR title and description to match the PR contents.

@pitrou.. I have fixed the PR title and the description to match the PR contents as you rightly pointed out.. Thanks for that.. !!

[pitrou]

LGTM, I'll just wait for CI before merging

[conbench-apache-arrow]

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

There were no benchmark performance regressions. 🎉

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

[Vishwanatha-HD]

Rationale for this change

Fix arrow-ipc-message-internal-test on big endian systems by passing endianness explicitly.

Are these changes tested?

Yes. The changes are tested on s390x arch to make sure things are working fine. The fix is also tested on x86 arch, to make sure there is no new regression introduced.

Are there any user-facing changes?

No

• GitHub main Issue link: [C++][Parquet] Enable Parquet DB support on Big Endian (IBM Z) systems #48151
• GitHub Issue: [C++][Parquet] Fix arrow-ipc-message-internal-test failure on Big-Endian (s390x) systems #48176

[github-actions]

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

[kou]

Could you fix lint failure?

[kou]

Why do we need this check? This check didn't exist in the existing code.

[Vishwanatha-HD]

@kou,
Please find the root cause of this issue on s390x platform below:

The test was failing with a std::bad_cast exception in the ProbeBatch_ResidualFilter function in hash_join.cc at line 309. The issue occurred when the code tried to cast a Datum's scalar to a BooleanScalar using:

const auto& mask_scalar = mask.scalar_as();

The problem was that the test uses literal(NullScalar()) as one of the filter expressions, which creates a NullScalar (of type NullType), not a BooleanScalar (of type BoolType). When the code tried to cast this to a BooleanScalar, it failed with std::bad_cast on s390x architecture

The Fix
I modified the ProbeBatch_ResidualFilter function to check the scalar's type before attempting the cast

[kiszk]

@Vishwanatha-HD I think that this change makes sense for both platforms: big and little endians. We do not need to use #if ... #else ... #endif.

Or, should we fix BooleanScalar to avoid throwing std::bad_cast? cc @pitrou

[zanmato1984]

This is a very detailed explanation. Thanks @Vishwanatha-HD .

As the author of the questioning test, I can say this is the issue of the test rather than of the hash join code. That is, using an filter expression evaluating to null-type null is invalid - it should be a boolean null. The hash join code itself arbitrarily assuming the expression being boolean is OK, though a DCHECK_EQ(mask.type()->id(), Type::BOOL) would be more preferable.

I think we should in turn fix the test by simply replacing the literal(NullScalar()) with a boolean null - literal(MakeNullScalar(boolean()))

[zanmato1984]

But one question remains: The test should be equally problematic for little-endian as well. Why is it passing?

I'm now looking into it.

[zanmato1984]

OK, mystery solved: there are two implementations of hash join - regular and swiss. The regular one has wider range of supported types and platforms (including big-endian). The swiss one is more efficient, but restrictive in supported types and platform (only little-endian). So on big endian platform, the regular one is chosen which is more strict on filter expression type. Little-endian platform uses the swiss one which is less strict on filter expression type by explicitly allowing a null-type null - so our CI, always ran on little-endian platforms, has always been passing.

I'm the author of supporting residual filter in swiss join as well as the tests. I think allowing null-type null was a mistake (it was implemented at the beginning of my contribution to Arrow - and I was very naive then).

I think I can send out an issue/pr soon to address the issues discovered here:

1. The test in question needs to be revised by replacing null-type null with boolean null, as mentioned in my previous comment (so it will pass on big-endian platforms);
2. The swiss join code need to be updated by not allowing null-type.
3. Add DCHECKs to enforce the filter expression checking. (optional)

[zanmato1984]

I filed #48268 to address this problem and I'll work on a PR very soon. Once it's fixed, there shouldn't be failures of this test on big-endian platforms. So as I mentioned before, this part of change is not need (and not quite correct). Thanks.

[zanmato1984]

Should be fixed by #48272 . @Vishwanatha-HD Could you first revert the change to hash_join.cc then rebase main to see if the hash join test is passing on big-endian? Thanks.

[Vishwanatha-HD]

@zanmato1984.. I tried reverting my changes done to hash_join.cc.. With the latest main, the testcase is passing on the big-endian machine..

[zanmato1984]

That's great, thanks for the update!

[Vishwanatha-HD]

Could you fix lint failure?

I have fixed the lint failures. Thanks..

[Vishwanatha-HD]

I have taken care of the review comments and made necessary changes to the code. Pls review my changes. Thanks..

[kiszk]

Could you remove [Parquet] from the title? I think that this is not a Parquet-specific issue.

[pitrou]

Er, ok, but there is a well-defined explanation to this: a Arrow schema has an endianness attached to it, and it defaults to the platform's endianness.

To obey the intent of this test case, you should instead change the schema instantiation above to:

auto schema = ::arrow::schema({f0}, Endianness::Little, metadata);

Does this change solve the issue on s390x?

[Vishwanatha-HD]

@pitrou.. Thanks for your review comments. But this change didnt quite resolve the issue. Thanks..

[pitrou]

@Vishwanatha-HD You're right, but that should be fixed once #48239 is merged. So I would recommend waiting for that.

[Vishwanatha-HD]

@pitrou.. With the #48239 getting merged into main, I tried making the changes to schema as you suggested above, but the testcase is not passing on s390x..

./debug/arrow-ipc-message-internal-test --gtest_filter="TestMessageInternal.TestByteIdentical"
Running main() from gmock_main.cc
Note: Google Test filter = TestMessageInternal.TestByteIdentical
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from TestMessageInternal
[ RUN ] TestMessageInternal.TestByteIdentical
arrow/cpp/src/arrow/testing/gtest_util.cc:241: Failure
Expected equality of these values:
buffer.size()
Which is: 228
expected.size()
Which is: 232
Mismatching buffer size

[ FAILED ] TestMessageInternal.TestByteIdentical (1 ms)
[----------] 1 test from TestMessageInternal (1 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test suite ran. (1 ms total)
[ PASSED ] 0 tests.
[ FAILED ] 1 test, listed below:
[ FAILED ] TestMessageInternal.TestByteIdentical

1 FAILED TEST

[pitrou]

@Vishwanatha-HD You must also undo your own changes.

[pitrou]

I think it's enough to only test Endianness::Little. Roundtripping is tested elsewhere.

[Vishwanatha-HD]

Ok.. sure.. I will make the necessary changes and push the code again.. Thanks for your comments @pitrou.. !!

[Vishwanatha-HD]

@pitrou, as per our discussion yesterday, I have made the necessary changes to message_internal_test.cc file to only test Endianness::Little.. I have reverted the code that I added for BE architecture. Thanks..

[pitrou]

@Vishwanatha-HD Are you sure you pushed your changes? I see lots of CI failures.

[Vishwanatha-HD]

@pitrou.. Apologies.. It was my bad that I had both Endianness in the for loop.. I have made the necessary code changes and pushed the changes again.. Thanks for pointing this out.. !!

[pitrou]

Also see #48239

[Vishwanatha-HD]

Could you remove [Parquet] from the title? I think that this is not a Parquet-specific issue.

Hi @kiszk.. My idea of having the Parquet is that the overall changes are intended for Parquet component and in order to enable support to Parquet DB on s390x..

[kiszk]

Could you remove [Parquet] from the title? I think that this is not a Parquet-specific issue.

Hi @kiszk.. My idea of having the Parquet is that the overall changes are intended for Parquet component and in order to enable support to Parquet DB on s390x..

I understand your idea. On the other hand, this test case is not for Parquet. The current title may confuse others.

[Vishwanatha-HD]

I have addressed all the review comments. Thanks.

[pitrou]

Hey @Vishwanatha-HD I have two requests:

1. can you rebase/merge the latest git main into this?
2. Please don't mark discussions as resolved as soon as you have answered them; they should only be marked resolved if there is common agreement that there's nothing to add

[Vishwanatha-HD]

Hey @Vishwanatha-HD I have two requests:

1. can you rebase/merge the latest git main into this?
2. Please don't mark discussions as resolved as soon as you have answered them; they should only be marked resolved if there is common agreement that there's nothing to add

Hi @pitrou.. Sure.. I will do the rebase with the latest git main and get the latest changes into this..
Also, I am sorry for marking the discussions resolved, as I thought I should close it as soon as I work on it & resolve it..

[Vishwanatha-HD]

Hey @Vishwanatha-HD I have two requests:

1. can you rebase/merge the latest git main into this?
2. Please don't mark discussions as resolved as soon as you have answered them; they should only be marked resolved if there is common agreement that there's nothing to add

Hi @pitrou.. Sure.. I will do the rebase with the latest git main and get the latest changes into this.. Also, I am sorry for marking the discussions resolved, as I thought I should close it as soon as I work on it & resolve it..

@pitrou.. I did the git rebase to merge the latest git main changes into this..

[Vishwanatha-HD]

With the changes done as part of #48272, the latest main works fine without the hash_join.cc changes on big-endian architecture.. Hence reverting the changes done as part of this PR..

[pitrou]

Also, please fix the PR title and description to match the PR contents.

[Vishwanatha-HD]

Also, please fix the PR title and description to match the PR contents.

@pitrou.. I have fixed the PR title and the description to match the PR contents as you rightly pointed out.. Thanks for that.. !!

[pitrou]

LGTM, I'll just wait for CI before merging

[conbench-apache-arrow]

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

There were no benchmark performance regressions. 🎉

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

[Vishwanatha-HD]

Rationale for this change

Fix arrow-ipc-message-internal-test on big endian systems by passing endianness explicitly.

Are these changes tested?

Yes. The changes are tested on s390x arch to make sure things are working fine. The fix is also tested on x86 arch, to make sure there is no new regression introduced.

Are there any user-facing changes?

No

• GitHub main Issue link: [C++][Parquet] Enable Parquet DB support on Big Endian (IBM Z) systems #48151
• GitHub Issue: [C++][Parquet] Fix arrow-ipc-message-internal-test failure on Big-Endian (s390x) systems #48176

[github-actions]

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

[kou]

Could you fix lint failure?

[kou]

Why do we need this check? This check didn't exist in the existing code.

[Vishwanatha-HD]

@kou,
Please find the root cause of this issue on s390x platform below:

The test was failing with a std::bad_cast exception in the ProbeBatch_ResidualFilter function in hash_join.cc at line 309. The issue occurred when the code tried to cast a Datum's scalar to a BooleanScalar using:

const auto& mask_scalar = mask.scalar_as();

The problem was that the test uses literal(NullScalar()) as one of the filter expressions, which creates a NullScalar (of type NullType), not a BooleanScalar (of type BoolType). When the code tried to cast this to a BooleanScalar, it failed with std::bad_cast on s390x architecture

The Fix
I modified the ProbeBatch_ResidualFilter function to check the scalar's type before attempting the cast

[kiszk]

@Vishwanatha-HD I think that this change makes sense for both platforms: big and little endians. We do not need to use #if ... #else ... #endif.

Or, should we fix BooleanScalar to avoid throwing std::bad_cast? cc @pitrou

[zanmato1984]

This is a very detailed explanation. Thanks @Vishwanatha-HD .

As the author of the questioning test, I can say this is the issue of the test rather than of the hash join code. That is, using an filter expression evaluating to null-type null is invalid - it should be a boolean null. The hash join code itself arbitrarily assuming the expression being boolean is OK, though a DCHECK_EQ(mask.type()->id(), Type::BOOL) would be more preferable.

I think we should in turn fix the test by simply replacing the literal(NullScalar()) with a boolean null - literal(MakeNullScalar(boolean()))

[zanmato1984]

But one question remains: The test should be equally problematic for little-endian as well. Why is it passing?

I'm now looking into it.

[zanmato1984]

OK, mystery solved: there are two implementations of hash join - regular and swiss. The regular one has wider range of supported types and platforms (including big-endian). The swiss one is more efficient, but restrictive in supported types and platform (only little-endian). So on big endian platform, the regular one is chosen which is more strict on filter expression type. Little-endian platform uses the swiss one which is less strict on filter expression type by explicitly allowing a null-type null - so our CI, always ran on little-endian platforms, has always been passing.

I'm the author of supporting residual filter in swiss join as well as the tests. I think allowing null-type null was a mistake (it was implemented at the beginning of my contribution to Arrow - and I was very naive then).

I think I can send out an issue/pr soon to address the issues discovered here:

1. The test in question needs to be revised by replacing null-type null with boolean null, as mentioned in my previous comment (so it will pass on big-endian platforms);
2. The swiss join code need to be updated by not allowing null-type.
3. Add DCHECKs to enforce the filter expression checking. (optional)

[zanmato1984]

I filed #48268 to address this problem and I'll work on a PR very soon. Once it's fixed, there shouldn't be failures of this test on big-endian platforms. So as I mentioned before, this part of change is not need (and not quite correct). Thanks.

[zanmato1984]

Should be fixed by #48272 . @Vishwanatha-HD Could you first revert the change to hash_join.cc then rebase main to see if the hash join test is passing on big-endian? Thanks.

[Vishwanatha-HD]

@zanmato1984.. I tried reverting my changes done to hash_join.cc.. With the latest main, the testcase is passing on the big-endian machine..

[zanmato1984]

That's great, thanks for the update!

[Vishwanatha-HD]

Could you fix lint failure?

I have fixed the lint failures. Thanks..

[Vishwanatha-HD]

I have taken care of the review comments and made necessary changes to the code. Pls review my changes. Thanks..

[kiszk]

Could you remove [Parquet] from the title? I think that this is not a Parquet-specific issue.

[pitrou]

Er, ok, but there is a well-defined explanation to this: a Arrow schema has an endianness attached to it, and it defaults to the platform's endianness.

To obey the intent of this test case, you should instead change the schema instantiation above to:

auto schema = ::arrow::schema({f0}, Endianness::Little, metadata);

Does this change solve the issue on s390x?

[Vishwanatha-HD]

@pitrou.. Thanks for your review comments. But this change didnt quite resolve the issue. Thanks..

[pitrou]

@Vishwanatha-HD You're right, but that should be fixed once #48239 is merged. So I would recommend waiting for that.

[Vishwanatha-HD]

@pitrou.. With the #48239 getting merged into main, I tried making the changes to schema as you suggested above, but the testcase is not passing on s390x..

./debug/arrow-ipc-message-internal-test --gtest_filter="TestMessageInternal.TestByteIdentical"
Running main() from gmock_main.cc
Note: Google Test filter = TestMessageInternal.TestByteIdentical
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from TestMessageInternal
[ RUN ] TestMessageInternal.TestByteIdentical
arrow/cpp/src/arrow/testing/gtest_util.cc:241: Failure
Expected equality of these values:
buffer.size()
Which is: 228
expected.size()
Which is: 232
Mismatching buffer size

[ FAILED ] TestMessageInternal.TestByteIdentical (1 ms)
[----------] 1 test from TestMessageInternal (1 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test suite ran. (1 ms total)
[ PASSED ] 0 tests.
[ FAILED ] 1 test, listed below:
[ FAILED ] TestMessageInternal.TestByteIdentical

1 FAILED TEST

[pitrou]

@Vishwanatha-HD You must also undo your own changes.

[pitrou]

I think it's enough to only test Endianness::Little. Roundtripping is tested elsewhere.

[Vishwanatha-HD]

Ok.. sure.. I will make the necessary changes and push the code again.. Thanks for your comments @pitrou.. !!

[Vishwanatha-HD]

@pitrou, as per our discussion yesterday, I have made the necessary changes to message_internal_test.cc file to only test Endianness::Little.. I have reverted the code that I added for BE architecture. Thanks..

[pitrou]

@Vishwanatha-HD Are you sure you pushed your changes? I see lots of CI failures.

[Vishwanatha-HD]

@pitrou.. Apologies.. It was my bad that I had both Endianness in the for loop.. I have made the necessary code changes and pushed the changes again.. Thanks for pointing this out.. !!

[pitrou]

Also see #48239

[Vishwanatha-HD]

Could you remove [Parquet] from the title? I think that this is not a Parquet-specific issue.

Hi @kiszk.. My idea of having the Parquet is that the overall changes are intended for Parquet component and in order to enable support to Parquet DB on s390x..

[kiszk]

Could you remove [Parquet] from the title? I think that this is not a Parquet-specific issue.

Hi @kiszk.. My idea of having the Parquet is that the overall changes are intended for Parquet component and in order to enable support to Parquet DB on s390x..

I understand your idea. On the other hand, this test case is not for Parquet. The current title may confuse others.

[Vishwanatha-HD]

I have addressed all the review comments. Thanks.

[pitrou]

Hey @Vishwanatha-HD I have two requests:

1. can you rebase/merge the latest git main into this?
2. Please don't mark discussions as resolved as soon as you have answered them; they should only be marked resolved if there is common agreement that there's nothing to add

[Vishwanatha-HD]

Hey @Vishwanatha-HD I have two requests:

1. can you rebase/merge the latest git main into this?
2. Please don't mark discussions as resolved as soon as you have answered them; they should only be marked resolved if there is common agreement that there's nothing to add

Hi @pitrou.. Sure.. I will do the rebase with the latest git main and get the latest changes into this..
Also, I am sorry for marking the discussions resolved, as I thought I should close it as soon as I work on it & resolve it..

[Vishwanatha-HD]

Hey @Vishwanatha-HD I have two requests:

1. can you rebase/merge the latest git main into this?
2. Please don't mark discussions as resolved as soon as you have answered them; they should only be marked resolved if there is common agreement that there's nothing to add

Hi @pitrou.. Sure.. I will do the rebase with the latest git main and get the latest changes into this.. Also, I am sorry for marking the discussions resolved, as I thought I should close it as soon as I work on it & resolve it..

@pitrou.. I did the git rebase to merge the latest git main changes into this..

[Vishwanatha-HD]

With the changes done as part of #48272, the latest main works fine without the hash_join.cc changes on big-endian architecture.. Hence reverting the changes done as part of this PR..

[pitrou]

Also, please fix the PR title and description to match the PR contents.

[Vishwanatha-HD]

Also, please fix the PR title and description to match the PR contents.

@pitrou.. I have fixed the PR title and the description to match the PR contents as you rightly pointed out.. Thanks for that.. !!

[pitrou]

LGTM, I'll just wait for CI before merging

[conbench-apache-arrow]

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

There were no benchmark performance regressions. 🎉

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

[Vishwanatha-HD]

Rationale for this change

Fix arrow-ipc-message-internal-test on big endian systems by passing endianness explicitly.

Are these changes tested?

Yes. The changes are tested on s390x arch to make sure things are working fine. The fix is also tested on x86 arch, to make sure there is no new regression introduced.

Are there any user-facing changes?

No

• GitHub main Issue link: [C++][Parquet] Enable Parquet DB support on Big Endian (IBM Z) systems #48151
• GitHub Issue: [C++][Parquet] Fix arrow-ipc-message-internal-test failure on Big-Endian (s390x) systems #48176

[github-actions]

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

[kou]

Could you fix lint failure?

[kou]

Why do we need this check? This check didn't exist in the existing code.

[Vishwanatha-HD]

@kou,
Please find the root cause of this issue on s390x platform below:

The test was failing with a std::bad_cast exception in the ProbeBatch_ResidualFilter function in hash_join.cc at line 309. The issue occurred when the code tried to cast a Datum's scalar to a BooleanScalar using:

const auto& mask_scalar = mask.scalar_as();

The problem was that the test uses literal(NullScalar()) as one of the filter expressions, which creates a NullScalar (of type NullType), not a BooleanScalar (of type BoolType). When the code tried to cast this to a BooleanScalar, it failed with std::bad_cast on s390x architecture

The Fix
I modified the ProbeBatch_ResidualFilter function to check the scalar's type before attempting the cast

[kiszk]

@Vishwanatha-HD I think that this change makes sense for both platforms: big and little endians. We do not need to use #if ... #else ... #endif.

Or, should we fix BooleanScalar to avoid throwing std::bad_cast? cc @pitrou

[zanmato1984]

This is a very detailed explanation. Thanks @Vishwanatha-HD .

As the author of the questioning test, I can say this is the issue of the test rather than of the hash join code. That is, using an filter expression evaluating to null-type null is invalid - it should be a boolean null. The hash join code itself arbitrarily assuming the expression being boolean is OK, though a DCHECK_EQ(mask.type()->id(), Type::BOOL) would be more preferable.

I think we should in turn fix the test by simply replacing the literal(NullScalar()) with a boolean null - literal(MakeNullScalar(boolean()))

[zanmato1984]

But one question remains: The test should be equally problematic for little-endian as well. Why is it passing?

I'm now looking into it.

[zanmato1984]

OK, mystery solved: there are two implementations of hash join - regular and swiss. The regular one has wider range of supported types and platforms (including big-endian). The swiss one is more efficient, but restrictive in supported types and platform (only little-endian). So on big endian platform, the regular one is chosen which is more strict on filter expression type. Little-endian platform uses the swiss one which is less strict on filter expression type by explicitly allowing a null-type null - so our CI, always ran on little-endian platforms, has always been passing.

I'm the author of supporting residual filter in swiss join as well as the tests. I think allowing null-type null was a mistake (it was implemented at the beginning of my contribution to Arrow - and I was very naive then).

I think I can send out an issue/pr soon to address the issues discovered here:

1. The test in question needs to be revised by replacing null-type null with boolean null, as mentioned in my previous comment (so it will pass on big-endian platforms);
2. The swiss join code need to be updated by not allowing null-type.
3. Add DCHECKs to enforce the filter expression checking. (optional)

[zanmato1984]

I filed #48268 to address this problem and I'll work on a PR very soon. Once it's fixed, there shouldn't be failures of this test on big-endian platforms. So as I mentioned before, this part of change is not need (and not quite correct). Thanks.

[zanmato1984]

Should be fixed by #48272 . @Vishwanatha-HD Could you first revert the change to hash_join.cc then rebase main to see if the hash join test is passing on big-endian? Thanks.

[Vishwanatha-HD]

@zanmato1984.. I tried reverting my changes done to hash_join.cc.. With the latest main, the testcase is passing on the big-endian machine..

[zanmato1984]

That's great, thanks for the update!

[Vishwanatha-HD]

Could you fix lint failure?

I have fixed the lint failures. Thanks..

[Vishwanatha-HD]

I have taken care of the review comments and made necessary changes to the code. Pls review my changes. Thanks..

[kiszk]

Could you remove [Parquet] from the title? I think that this is not a Parquet-specific issue.

[pitrou]

Er, ok, but there is a well-defined explanation to this: a Arrow schema has an endianness attached to it, and it defaults to the platform's endianness.

To obey the intent of this test case, you should instead change the schema instantiation above to:

auto schema = ::arrow::schema({f0}, Endianness::Little, metadata);

Does this change solve the issue on s390x?

[Vishwanatha-HD]

@pitrou.. Thanks for your review comments. But this change didnt quite resolve the issue. Thanks..

[pitrou]

@Vishwanatha-HD You're right, but that should be fixed once #48239 is merged. So I would recommend waiting for that.

[Vishwanatha-HD]

@pitrou.. With the #48239 getting merged into main, I tried making the changes to schema as you suggested above, but the testcase is not passing on s390x..

./debug/arrow-ipc-message-internal-test --gtest_filter="TestMessageInternal.TestByteIdentical"
Running main() from gmock_main.cc
Note: Google Test filter = TestMessageInternal.TestByteIdentical
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from TestMessageInternal
[ RUN ] TestMessageInternal.TestByteIdentical
arrow/cpp/src/arrow/testing/gtest_util.cc:241: Failure
Expected equality of these values:
buffer.size()
Which is: 228
expected.size()
Which is: 232
Mismatching buffer size

[ FAILED ] TestMessageInternal.TestByteIdentical (1 ms)
[----------] 1 test from TestMessageInternal (1 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test suite ran. (1 ms total)
[ PASSED ] 0 tests.
[ FAILED ] 1 test, listed below:
[ FAILED ] TestMessageInternal.TestByteIdentical

1 FAILED TEST

[pitrou]

@Vishwanatha-HD You must also undo your own changes.

[pitrou]

I think it's enough to only test Endianness::Little. Roundtripping is tested elsewhere.

[Vishwanatha-HD]

Ok.. sure.. I will make the necessary changes and push the code again.. Thanks for your comments @pitrou.. !!

[Vishwanatha-HD]

@pitrou, as per our discussion yesterday, I have made the necessary changes to message_internal_test.cc file to only test Endianness::Little.. I have reverted the code that I added for BE architecture. Thanks..

[pitrou]

@Vishwanatha-HD Are you sure you pushed your changes? I see lots of CI failures.

[Vishwanatha-HD]

@pitrou.. Apologies.. It was my bad that I had both Endianness in the for loop.. I have made the necessary code changes and pushed the changes again.. Thanks for pointing this out.. !!

[pitrou]

Also see #48239

[Vishwanatha-HD]

Could you remove [Parquet] from the title? I think that this is not a Parquet-specific issue.

Hi @kiszk.. My idea of having the Parquet is that the overall changes are intended for Parquet component and in order to enable support to Parquet DB on s390x..

[kiszk]

Could you remove [Parquet] from the title? I think that this is not a Parquet-specific issue.

Hi @kiszk.. My idea of having the Parquet is that the overall changes are intended for Parquet component and in order to enable support to Parquet DB on s390x..

I understand your idea. On the other hand, this test case is not for Parquet. The current title may confuse others.

[Vishwanatha-HD]

I have addressed all the review comments. Thanks.

[pitrou]

Hey @Vishwanatha-HD I have two requests:

1. can you rebase/merge the latest git main into this?
2. Please don't mark discussions as resolved as soon as you have answered them; they should only be marked resolved if there is common agreement that there's nothing to add

[Vishwanatha-HD]

Hey @Vishwanatha-HD I have two requests:

1. can you rebase/merge the latest git main into this?
2. Please don't mark discussions as resolved as soon as you have answered them; they should only be marked resolved if there is common agreement that there's nothing to add

Hi @pitrou.. Sure.. I will do the rebase with the latest git main and get the latest changes into this..
Also, I am sorry for marking the discussions resolved, as I thought I should close it as soon as I work on it & resolve it..

[Vishwanatha-HD]

Hey @Vishwanatha-HD I have two requests:

1. can you rebase/merge the latest git main into this?
2. Please don't mark discussions as resolved as soon as you have answered them; they should only be marked resolved if there is common agreement that there's nothing to add

Hi @pitrou.. Sure.. I will do the rebase with the latest git main and get the latest changes into this.. Also, I am sorry for marking the discussions resolved, as I thought I should close it as soon as I work on it & resolve it..

@pitrou.. I did the git rebase to merge the latest git main changes into this..

[Vishwanatha-HD]

With the changes done as part of #48272, the latest main works fine without the hash_join.cc changes on big-endian architecture.. Hence reverting the changes done as part of this PR..

[pitrou]

Also, please fix the PR title and description to match the PR contents.

[Vishwanatha-HD]

Also, please fix the PR title and description to match the PR contents.

@pitrou.. I have fixed the PR title and the description to match the PR contents as you rightly pointed out.. Thanks for that.. !!

[pitrou]

LGTM, I'll just wait for CI before merging

[conbench-apache-arrow]

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

There were no benchmark performance regressions. 🎉

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

[Vishwanatha-HD]

Rationale for this change

Fix arrow-ipc-message-internal-test on big endian systems by passing endianness explicitly.

Are these changes tested?

Yes. The changes are tested on s390x arch to make sure things are working fine. The fix is also tested on x86 arch, to make sure there is no new regression introduced.

Are there any user-facing changes?

No

• GitHub main Issue link: [C++][Parquet] Enable Parquet DB support on Big Endian (IBM Z) systems #48151
• GitHub Issue: [C++][Parquet] Fix arrow-ipc-message-internal-test failure on Big-Endian (s390x) systems #48176

[github-actions]

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

[kou]

Could you fix lint failure?

[kou]

Why do we need this check? This check didn't exist in the existing code.

[Vishwanatha-HD]

@kou,
Please find the root cause of this issue on s390x platform below:

The test was failing with a std::bad_cast exception in the ProbeBatch_ResidualFilter function in hash_join.cc at line 309. The issue occurred when the code tried to cast a Datum's scalar to a BooleanScalar using:

const auto& mask_scalar = mask.scalar_as();

The problem was that the test uses literal(NullScalar()) as one of the filter expressions, which creates a NullScalar (of type NullType), not a BooleanScalar (of type BoolType). When the code tried to cast this to a BooleanScalar, it failed with std::bad_cast on s390x architecture

The Fix
I modified the ProbeBatch_ResidualFilter function to check the scalar's type before attempting the cast

[kiszk]

@Vishwanatha-HD I think that this change makes sense for both platforms: big and little endians. We do not need to use #if ... #else ... #endif.

Or, should we fix BooleanScalar to avoid throwing std::bad_cast? cc @pitrou

[zanmato1984]

This is a very detailed explanation. Thanks @Vishwanatha-HD .

As the author of the questioning test, I can say this is the issue of the test rather than of the hash join code. That is, using an filter expression evaluating to null-type null is invalid - it should be a boolean null. The hash join code itself arbitrarily assuming the expression being boolean is OK, though a DCHECK_EQ(mask.type()->id(), Type::BOOL) would be more preferable.

I think we should in turn fix the test by simply replacing the literal(NullScalar()) with a boolean null - literal(MakeNullScalar(boolean()))

[zanmato1984]

But one question remains: The test should be equally problematic for little-endian as well. Why is it passing?

I'm now looking into it.

[zanmato1984]

OK, mystery solved: there are two implementations of hash join - regular and swiss. The regular one has wider range of supported types and platforms (including big-endian). The swiss one is more efficient, but restrictive in supported types and platform (only little-endian). So on big endian platform, the regular one is chosen which is more strict on filter expression type. Little-endian platform uses the swiss one which is less strict on filter expression type by explicitly allowing a null-type null - so our CI, always ran on little-endian platforms, has always been passing.

I'm the author of supporting residual filter in swiss join as well as the tests. I think allowing null-type null was a mistake (it was implemented at the beginning of my contribution to Arrow - and I was very naive then).

I think I can send out an issue/pr soon to address the issues discovered here:

1. The test in question needs to be revised by replacing null-type null with boolean null, as mentioned in my previous comment (so it will pass on big-endian platforms);
2. The swiss join code need to be updated by not allowing null-type.
3. Add DCHECKs to enforce the filter expression checking. (optional)

[zanmato1984]

I filed #48268 to address this problem and I'll work on a PR very soon. Once it's fixed, there shouldn't be failures of this test on big-endian platforms. So as I mentioned before, this part of change is not need (and not quite correct). Thanks.

[zanmato1984]

Should be fixed by #48272 . @Vishwanatha-HD Could you first revert the change to hash_join.cc then rebase main to see if the hash join test is passing on big-endian? Thanks.

[Vishwanatha-HD]

@zanmato1984.. I tried reverting my changes done to hash_join.cc.. With the latest main, the testcase is passing on the big-endian machine..

[zanmato1984]

That's great, thanks for the update!

[Vishwanatha-HD]

Could you fix lint failure?

I have fixed the lint failures. Thanks..

[Vishwanatha-HD]

I have taken care of the review comments and made necessary changes to the code. Pls review my changes. Thanks..

[kiszk]

Could you remove [Parquet] from the title? I think that this is not a Parquet-specific issue.

[pitrou]

Er, ok, but there is a well-defined explanation to this: a Arrow schema has an endianness attached to it, and it defaults to the platform's endianness.

To obey the intent of this test case, you should instead change the schema instantiation above to:

auto schema = ::arrow::schema({f0}, Endianness::Little, metadata);

Does this change solve the issue on s390x?

[Vishwanatha-HD]

@pitrou.. Thanks for your review comments. But this change didnt quite resolve the issue. Thanks..

[pitrou]

@Vishwanatha-HD You're right, but that should be fixed once #48239 is merged. So I would recommend waiting for that.

[Vishwanatha-HD]

@pitrou.. With the #48239 getting merged into main, I tried making the changes to schema as you suggested above, but the testcase is not passing on s390x..

./debug/arrow-ipc-message-internal-test --gtest_filter="TestMessageInternal.TestByteIdentical"
Running main() from gmock_main.cc
Note: Google Test filter = TestMessageInternal.TestByteIdentical
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from TestMessageInternal
[ RUN ] TestMessageInternal.TestByteIdentical
arrow/cpp/src/arrow/testing/gtest_util.cc:241: Failure
Expected equality of these values:
buffer.size()
Which is: 228
expected.size()
Which is: 232
Mismatching buffer size

[ FAILED ] TestMessageInternal.TestByteIdentical (1 ms)
[----------] 1 test from TestMessageInternal (1 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test suite ran. (1 ms total)
[ PASSED ] 0 tests.
[ FAILED ] 1 test, listed below:
[ FAILED ] TestMessageInternal.TestByteIdentical

1 FAILED TEST

[pitrou]

@Vishwanatha-HD You must also undo your own changes.

[pitrou]

I think it's enough to only test Endianness::Little. Roundtripping is tested elsewhere.

[Vishwanatha-HD]

Ok.. sure.. I will make the necessary changes and push the code again.. Thanks for your comments @pitrou.. !!

[Vishwanatha-HD]

@pitrou, as per our discussion yesterday, I have made the necessary changes to message_internal_test.cc file to only test Endianness::Little.. I have reverted the code that I added for BE architecture. Thanks..

[pitrou]

@Vishwanatha-HD Are you sure you pushed your changes? I see lots of CI failures.

[Vishwanatha-HD]

@pitrou.. Apologies.. It was my bad that I had both Endianness in the for loop.. I have made the necessary code changes and pushed the changes again.. Thanks for pointing this out.. !!

[pitrou]

Also see #48239

[Vishwanatha-HD]

Could you remove [Parquet] from the title? I think that this is not a Parquet-specific issue.

Hi @kiszk.. My idea of having the Parquet is that the overall changes are intended for Parquet component and in order to enable support to Parquet DB on s390x..

[kiszk]

Could you remove [Parquet] from the title? I think that this is not a Parquet-specific issue.

Hi @kiszk.. My idea of having the Parquet is that the overall changes are intended for Parquet component and in order to enable support to Parquet DB on s390x..

I understand your idea. On the other hand, this test case is not for Parquet. The current title may confuse others.

[Vishwanatha-HD]

I have addressed all the review comments. Thanks.

[pitrou]

Hey @Vishwanatha-HD I have two requests:

1. can you rebase/merge the latest git main into this?
2. Please don't mark discussions as resolved as soon as you have answered them; they should only be marked resolved if there is common agreement that there's nothing to add

[Vishwanatha-HD]

Hey @Vishwanatha-HD I have two requests:

1. can you rebase/merge the latest git main into this?
2. Please don't mark discussions as resolved as soon as you have answered them; they should only be marked resolved if there is common agreement that there's nothing to add

Hi @pitrou.. Sure.. I will do the rebase with the latest git main and get the latest changes into this..
Also, I am sorry for marking the discussions resolved, as I thought I should close it as soon as I work on it & resolve it..

[Vishwanatha-HD]

Hey @Vishwanatha-HD I have two requests:

1. can you rebase/merge the latest git main into this?
2. Please don't mark discussions as resolved as soon as you have answered them; they should only be marked resolved if there is common agreement that there's nothing to add

Hi @pitrou.. Sure.. I will do the rebase with the latest git main and get the latest changes into this.. Also, I am sorry for marking the discussions resolved, as I thought I should close it as soon as I work on it & resolve it..

@pitrou.. I did the git rebase to merge the latest git main changes into this..

[Vishwanatha-HD]

With the changes done as part of #48272, the latest main works fine without the hash_join.cc changes on big-endian architecture.. Hence reverting the changes done as part of this PR..

[pitrou]

Also, please fix the PR title and description to match the PR contents.

[Vishwanatha-HD]

Also, please fix the PR title and description to match the PR contents.

@pitrou.. I have fixed the PR title and the description to match the PR contents as you rightly pointed out.. Thanks for that.. !!

[pitrou]

LGTM, I'll just wait for CI before merging

[conbench-apache-arrow]

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

There were no benchmark performance regressions. 🎉

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

[Vishwanatha-HD]

Rationale for this change

Fix arrow-ipc-message-internal-test on big endian systems by passing endianness explicitly.

Are these changes tested?

Yes. The changes are tested on s390x arch to make sure things are working fine. The fix is also tested on x86 arch, to make sure there is no new regression introduced.

Are there any user-facing changes?

No

• GitHub main Issue link: [C++][Parquet] Enable Parquet DB support on Big Endian (IBM Z) systems #48151
• GitHub Issue: [C++][Parquet] Fix arrow-ipc-message-internal-test failure on Big-Endian (s390x) systems #48176

[github-actions]

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

[kou]

Could you fix lint failure?

[kou]

Why do we need this check? This check didn't exist in the existing code.

[Vishwanatha-HD]

@kou,
Please find the root cause of this issue on s390x platform below:

The test was failing with a std::bad_cast exception in the ProbeBatch_ResidualFilter function in hash_join.cc at line 309. The issue occurred when the code tried to cast a Datum's scalar to a BooleanScalar using:

const auto& mask_scalar = mask.scalar_as();

The problem was that the test uses literal(NullScalar()) as one of the filter expressions, which creates a NullScalar (of type NullType), not a BooleanScalar (of type BoolType). When the code tried to cast this to a BooleanScalar, it failed with std::bad_cast on s390x architecture

The Fix
I modified the ProbeBatch_ResidualFilter function to check the scalar's type before attempting the cast

[kiszk]

@Vishwanatha-HD I think that this change makes sense for both platforms: big and little endians. We do not need to use #if ... #else ... #endif.

Or, should we fix BooleanScalar to avoid throwing std::bad_cast? cc @pitrou

[zanmato1984]

This is a very detailed explanation. Thanks @Vishwanatha-HD .

As the author of the questioning test, I can say this is the issue of the test rather than of the hash join code. That is, using an filter expression evaluating to null-type null is invalid - it should be a boolean null. The hash join code itself arbitrarily assuming the expression being boolean is OK, though a DCHECK_EQ(mask.type()->id(), Type::BOOL) would be more preferable.

I think we should in turn fix the test by simply replacing the literal(NullScalar()) with a boolean null - literal(MakeNullScalar(boolean()))

[zanmato1984]

But one question remains: The test should be equally problematic for little-endian as well. Why is it passing?

I'm now looking into it.

[zanmato1984]

OK, mystery solved: there are two implementations of hash join - regular and swiss. The regular one has wider range of supported types and platforms (including big-endian). The swiss one is more efficient, but restrictive in supported types and platform (only little-endian). So on big endian platform, the regular one is chosen which is more strict on filter expression type. Little-endian platform uses the swiss one which is less strict on filter expression type by explicitly allowing a null-type null - so our CI, always ran on little-endian platforms, has always been passing.

I'm the author of supporting residual filter in swiss join as well as the tests. I think allowing null-type null was a mistake (it was implemented at the beginning of my contribution to Arrow - and I was very naive then).

I think I can send out an issue/pr soon to address the issues discovered here:

1. The test in question needs to be revised by replacing null-type null with boolean null, as mentioned in my previous comment (so it will pass on big-endian platforms);
2. The swiss join code need to be updated by not allowing null-type.
3. Add DCHECKs to enforce the filter expression checking. (optional)

[zanmato1984]

I filed #48268 to address this problem and I'll work on a PR very soon. Once it's fixed, there shouldn't be failures of this test on big-endian platforms. So as I mentioned before, this part of change is not need (and not quite correct). Thanks.

[zanmato1984]

Should be fixed by #48272 . @Vishwanatha-HD Could you first revert the change to hash_join.cc then rebase main to see if the hash join test is passing on big-endian? Thanks.

[Vishwanatha-HD]

@zanmato1984.. I tried reverting my changes done to hash_join.cc.. With the latest main, the testcase is passing on the big-endian machine..

[zanmato1984]

That's great, thanks for the update!

[Vishwanatha-HD]

Could you fix lint failure?

I have fixed the lint failures. Thanks..

[Vishwanatha-HD]

I have taken care of the review comments and made necessary changes to the code. Pls review my changes. Thanks..

[kiszk]

Could you remove [Parquet] from the title? I think that this is not a Parquet-specific issue.

[pitrou]

Er, ok, but there is a well-defined explanation to this: a Arrow schema has an endianness attached to it, and it defaults to the platform's endianness.

To obey the intent of this test case, you should instead change the schema instantiation above to:

auto schema = ::arrow::schema({f0}, Endianness::Little, metadata);

Does this change solve the issue on s390x?

[Vishwanatha-HD]

@pitrou.. Thanks for your review comments. But this change didnt quite resolve the issue. Thanks..

[pitrou]

@Vishwanatha-HD You're right, but that should be fixed once #48239 is merged. So I would recommend waiting for that.

[Vishwanatha-HD]

@pitrou.. With the #48239 getting merged into main, I tried making the changes to schema as you suggested above, but the testcase is not passing on s390x..

./debug/arrow-ipc-message-internal-test --gtest_filter="TestMessageInternal.TestByteIdentical"
Running main() from gmock_main.cc
Note: Google Test filter = TestMessageInternal.TestByteIdentical
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from TestMessageInternal
[ RUN ] TestMessageInternal.TestByteIdentical
arrow/cpp/src/arrow/testing/gtest_util.cc:241: Failure
Expected equality of these values:
buffer.size()
Which is: 228
expected.size()
Which is: 232
Mismatching buffer size

[ FAILED ] TestMessageInternal.TestByteIdentical (1 ms)
[----------] 1 test from TestMessageInternal (1 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test suite ran. (1 ms total)
[ PASSED ] 0 tests.
[ FAILED ] 1 test, listed below:
[ FAILED ] TestMessageInternal.TestByteIdentical

1 FAILED TEST

[pitrou]

@Vishwanatha-HD You must also undo your own changes.

[pitrou]

I think it's enough to only test Endianness::Little. Roundtripping is tested elsewhere.

[Vishwanatha-HD]

Ok.. sure.. I will make the necessary changes and push the code again.. Thanks for your comments @pitrou.. !!

[Vishwanatha-HD]

@pitrou, as per our discussion yesterday, I have made the necessary changes to message_internal_test.cc file to only test Endianness::Little.. I have reverted the code that I added for BE architecture. Thanks..

[pitrou]

@Vishwanatha-HD Are you sure you pushed your changes? I see lots of CI failures.

[Vishwanatha-HD]

@pitrou.. Apologies.. It was my bad that I had both Endianness in the for loop.. I have made the necessary code changes and pushed the changes again.. Thanks for pointing this out.. !!

[pitrou]

Also see #48239

[Vishwanatha-HD]

Could you remove [Parquet] from the title? I think that this is not a Parquet-specific issue.

Hi @kiszk.. My idea of having the Parquet is that the overall changes are intended for Parquet component and in order to enable support to Parquet DB on s390x..

[kiszk]

Could you remove [Parquet] from the title? I think that this is not a Parquet-specific issue.

Hi @kiszk.. My idea of having the Parquet is that the overall changes are intended for Parquet component and in order to enable support to Parquet DB on s390x..

I understand your idea. On the other hand, this test case is not for Parquet. The current title may confuse others.

[Vishwanatha-HD]

I have addressed all the review comments. Thanks.

[pitrou]

Hey @Vishwanatha-HD I have two requests:

1. can you rebase/merge the latest git main into this?
2. Please don't mark discussions as resolved as soon as you have answered them; they should only be marked resolved if there is common agreement that there's nothing to add

[Vishwanatha-HD]

Hey @Vishwanatha-HD I have two requests:

1. can you rebase/merge the latest git main into this?
2. Please don't mark discussions as resolved as soon as you have answered them; they should only be marked resolved if there is common agreement that there's nothing to add

Hi @pitrou.. Sure.. I will do the rebase with the latest git main and get the latest changes into this..
Also, I am sorry for marking the discussions resolved, as I thought I should close it as soon as I work on it & resolve it..

[Vishwanatha-HD]

Hey @Vishwanatha-HD I have two requests:

1. can you rebase/merge the latest git main into this?
2. Please don't mark discussions as resolved as soon as you have answered them; they should only be marked resolved if there is common agreement that there's nothing to add

Hi @pitrou.. Sure.. I will do the rebase with the latest git main and get the latest changes into this.. Also, I am sorry for marking the discussions resolved, as I thought I should close it as soon as I work on it & resolve it..

@pitrou.. I did the git rebase to merge the latest git main changes into this..

[Vishwanatha-HD]

With the changes done as part of #48272, the latest main works fine without the hash_join.cc changes on big-endian architecture.. Hence reverting the changes done as part of this PR..

[pitrou]

Also, please fix the PR title and description to match the PR contents.

[Vishwanatha-HD]

Also, please fix the PR title and description to match the PR contents.

@pitrou.. I have fixed the PR title and the description to match the PR contents as you rightly pointed out.. Thanks for that.. !!

[pitrou]

LGTM, I'll just wait for CI before merging

[conbench-apache-arrow]

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

There were no benchmark performance regressions. 🎉

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

[Vishwanatha-HD]

Rationale for this change

Fix arrow-ipc-message-internal-test on big endian systems by passing endianness explicitly.

Are these changes tested?

Yes. The changes are tested on s390x arch to make sure things are working fine. The fix is also tested on x86 arch, to make sure there is no new regression introduced.

Are there any user-facing changes?

No

• GitHub main Issue link: [C++][Parquet] Enable Parquet DB support on Big Endian (IBM Z) systems #48151
• GitHub Issue: [C++][Parquet] Fix arrow-ipc-message-internal-test failure on Big-Endian (s390x) systems #48176

[github-actions]

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

[kou]

Could you fix lint failure?

[kou]

Why do we need this check? This check didn't exist in the existing code.

[Vishwanatha-HD]

@kou,
Please find the root cause of this issue on s390x platform below:

The test was failing with a std::bad_cast exception in the ProbeBatch_ResidualFilter function in hash_join.cc at line 309. The issue occurred when the code tried to cast a Datum's scalar to a BooleanScalar using:

const auto& mask_scalar = mask.scalar_as();

The problem was that the test uses literal(NullScalar()) as one of the filter expressions, which creates a NullScalar (of type NullType), not a BooleanScalar (of type BoolType). When the code tried to cast this to a BooleanScalar, it failed with std::bad_cast on s390x architecture

The Fix
I modified the ProbeBatch_ResidualFilter function to check the scalar's type before attempting the cast

[kiszk]

@Vishwanatha-HD I think that this change makes sense for both platforms: big and little endians. We do not need to use #if ... #else ... #endif.

Or, should we fix BooleanScalar to avoid throwing std::bad_cast? cc @pitrou

[zanmato1984]

This is a very detailed explanation. Thanks @Vishwanatha-HD .

As the author of the questioning test, I can say this is the issue of the test rather than of the hash join code. That is, using an filter expression evaluating to null-type null is invalid - it should be a boolean null. The hash join code itself arbitrarily assuming the expression being boolean is OK, though a DCHECK_EQ(mask.type()->id(), Type::BOOL) would be more preferable.

I think we should in turn fix the test by simply replacing the literal(NullScalar()) with a boolean null - literal(MakeNullScalar(boolean()))

[zanmato1984]

But one question remains: The test should be equally problematic for little-endian as well. Why is it passing?

I'm now looking into it.

[zanmato1984]

OK, mystery solved: there are two implementations of hash join - regular and swiss. The regular one has wider range of supported types and platforms (including big-endian). The swiss one is more efficient, but restrictive in supported types and platform (only little-endian). So on big endian platform, the regular one is chosen which is more strict on filter expression type. Little-endian platform uses the swiss one which is less strict on filter expression type by explicitly allowing a null-type null - so our CI, always ran on little-endian platforms, has always been passing.

I'm the author of supporting residual filter in swiss join as well as the tests. I think allowing null-type null was a mistake (it was implemented at the beginning of my contribution to Arrow - and I was very naive then).

I think I can send out an issue/pr soon to address the issues discovered here:

1. The test in question needs to be revised by replacing null-type null with boolean null, as mentioned in my previous comment (so it will pass on big-endian platforms);
2. The swiss join code need to be updated by not allowing null-type.
3. Add DCHECKs to enforce the filter expression checking. (optional)

[zanmato1984]

I filed #48268 to address this problem and I'll work on a PR very soon. Once it's fixed, there shouldn't be failures of this test on big-endian platforms. So as I mentioned before, this part of change is not need (and not quite correct). Thanks.

[zanmato1984]

Should be fixed by #48272 . @Vishwanatha-HD Could you first revert the change to hash_join.cc then rebase main to see if the hash join test is passing on big-endian? Thanks.

[Vishwanatha-HD]

@zanmato1984.. I tried reverting my changes done to hash_join.cc.. With the latest main, the testcase is passing on the big-endian machine..

[zanmato1984]

That's great, thanks for the update!

[Vishwanatha-HD]

Could you fix lint failure?

I have fixed the lint failures. Thanks..

[Vishwanatha-HD]

I have taken care of the review comments and made necessary changes to the code. Pls review my changes. Thanks..

[kiszk]

Could you remove [Parquet] from the title? I think that this is not a Parquet-specific issue.

[pitrou]

Er, ok, but there is a well-defined explanation to this: a Arrow schema has an endianness attached to it, and it defaults to the platform's endianness.

To obey the intent of this test case, you should instead change the schema instantiation above to:

auto schema = ::arrow::schema({f0}, Endianness::Little, metadata);

Does this change solve the issue on s390x?

[Vishwanatha-HD]

@pitrou.. Thanks for your review comments. But this change didnt quite resolve the issue. Thanks..

[pitrou]

@Vishwanatha-HD You're right, but that should be fixed once #48239 is merged. So I would recommend waiting for that.

[Vishwanatha-HD]

@pitrou.. With the #48239 getting merged into main, I tried making the changes to schema as you suggested above, but the testcase is not passing on s390x..

./debug/arrow-ipc-message-internal-test --gtest_filter="TestMessageInternal.TestByteIdentical"
Running main() from gmock_main.cc
Note: Google Test filter = TestMessageInternal.TestByteIdentical
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from TestMessageInternal
[ RUN ] TestMessageInternal.TestByteIdentical
arrow/cpp/src/arrow/testing/gtest_util.cc:241: Failure
Expected equality of these values:
buffer.size()
Which is: 228
expected.size()
Which is: 232
Mismatching buffer size

[ FAILED ] TestMessageInternal.TestByteIdentical (1 ms)
[----------] 1 test from TestMessageInternal (1 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test suite ran. (1 ms total)
[ PASSED ] 0 tests.
[ FAILED ] 1 test, listed below:
[ FAILED ] TestMessageInternal.TestByteIdentical

1 FAILED TEST

[pitrou]

@Vishwanatha-HD You must also undo your own changes.

[pitrou]

I think it's enough to only test Endianness::Little. Roundtripping is tested elsewhere.

[Vishwanatha-HD]

Ok.. sure.. I will make the necessary changes and push the code again.. Thanks for your comments @pitrou.. !!

[Vishwanatha-HD]

@pitrou, as per our discussion yesterday, I have made the necessary changes to message_internal_test.cc file to only test Endianness::Little.. I have reverted the code that I added for BE architecture. Thanks..

[pitrou]

@Vishwanatha-HD Are you sure you pushed your changes? I see lots of CI failures.

[Vishwanatha-HD]

@pitrou.. Apologies.. It was my bad that I had both Endianness in the for loop.. I have made the necessary code changes and pushed the changes again.. Thanks for pointing this out.. !!

[pitrou]

Also see #48239

[Vishwanatha-HD]

Could you remove [Parquet] from the title? I think that this is not a Parquet-specific issue.

Hi @kiszk.. My idea of having the Parquet is that the overall changes are intended for Parquet component and in order to enable support to Parquet DB on s390x..

[kiszk]

Could you remove [Parquet] from the title? I think that this is not a Parquet-specific issue.

Hi @kiszk.. My idea of having the Parquet is that the overall changes are intended for Parquet component and in order to enable support to Parquet DB on s390x..

I understand your idea. On the other hand, this test case is not for Parquet. The current title may confuse others.

[Vishwanatha-HD]

I have addressed all the review comments. Thanks.

[pitrou]

Hey @Vishwanatha-HD I have two requests:

1. can you rebase/merge the latest git main into this?
2. Please don't mark discussions as resolved as soon as you have answered them; they should only be marked resolved if there is common agreement that there's nothing to add

[Vishwanatha-HD]

Hey @Vishwanatha-HD I have two requests:

1. can you rebase/merge the latest git main into this?
2. Please don't mark discussions as resolved as soon as you have answered them; they should only be marked resolved if there is common agreement that there's nothing to add

Hi @pitrou.. Sure.. I will do the rebase with the latest git main and get the latest changes into this..
Also, I am sorry for marking the discussions resolved, as I thought I should close it as soon as I work on it & resolve it..

[Vishwanatha-HD]

Hey @Vishwanatha-HD I have two requests:

1. can you rebase/merge the latest git main into this?
2. Please don't mark discussions as resolved as soon as you have answered them; they should only be marked resolved if there is common agreement that there's nothing to add

Hi @pitrou.. Sure.. I will do the rebase with the latest git main and get the latest changes into this.. Also, I am sorry for marking the discussions resolved, as I thought I should close it as soon as I work on it & resolve it..

@pitrou.. I did the git rebase to merge the latest git main changes into this..

[Vishwanatha-HD]

With the changes done as part of #48272, the latest main works fine without the hash_join.cc changes on big-endian architecture.. Hence reverting the changes done as part of this PR..

[pitrou]

Also, please fix the PR title and description to match the PR contents.

[Vishwanatha-HD]

Also, please fix the PR title and description to match the PR contents.

@pitrou.. I have fixed the PR title and the description to match the PR contents as you rightly pointed out.. Thanks for that.. !!

[pitrou]

LGTM, I'll just wait for CI before merging

[conbench-apache-arrow]

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

There were no benchmark performance regressions. 🎉

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

[Vishwanatha-HD]

Rationale for this change

Fix arrow-ipc-message-internal-test on big endian systems by passing endianness explicitly.

Are these changes tested?

Yes. The changes are tested on s390x arch to make sure things are working fine. The fix is also tested on x86 arch, to make sure there is no new regression introduced.

Are there any user-facing changes?

No

• GitHub main Issue link: [C++][Parquet] Enable Parquet DB support on Big Endian (IBM Z) systems #48151
• GitHub Issue: [C++][Parquet] Fix arrow-ipc-message-internal-test failure on Big-Endian (s390x) systems #48176

[github-actions]

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

[kou]

Could you fix lint failure?

[kou]

Why do we need this check? This check didn't exist in the existing code.

[Vishwanatha-HD]

@kou,
Please find the root cause of this issue on s390x platform below:

The test was failing with a std::bad_cast exception in the ProbeBatch_ResidualFilter function in hash_join.cc at line 309. The issue occurred when the code tried to cast a Datum's scalar to a BooleanScalar using:

const auto& mask_scalar = mask.scalar_as();

The problem was that the test uses literal(NullScalar()) as one of the filter expressions, which creates a NullScalar (of type NullType), not a BooleanScalar (of type BoolType). When the code tried to cast this to a BooleanScalar, it failed with std::bad_cast on s390x architecture

The Fix
I modified the ProbeBatch_ResidualFilter function to check the scalar's type before attempting the cast

[kiszk]

@Vishwanatha-HD I think that this change makes sense for both platforms: big and little endians. We do not need to use #if ... #else ... #endif.

Or, should we fix BooleanScalar to avoid throwing std::bad_cast? cc @pitrou

[zanmato1984]

This is a very detailed explanation. Thanks @Vishwanatha-HD .

As the author of the questioning test, I can say this is the issue of the test rather than of the hash join code. That is, using an filter expression evaluating to null-type null is invalid - it should be a boolean null. The hash join code itself arbitrarily assuming the expression being boolean is OK, though a DCHECK_EQ(mask.type()->id(), Type::BOOL) would be more preferable.

I think we should in turn fix the test by simply replacing the literal(NullScalar()) with a boolean null - literal(MakeNullScalar(boolean()))

[zanmato1984]

But one question remains: The test should be equally problematic for little-endian as well. Why is it passing?

I'm now looking into it.

[zanmato1984]

OK, mystery solved: there are two implementations of hash join - regular and swiss. The regular one has wider range of supported types and platforms (including big-endian). The swiss one is more efficient, but restrictive in supported types and platform (only little-endian). So on big endian platform, the regular one is chosen which is more strict on filter expression type. Little-endian platform uses the swiss one which is less strict on filter expression type by explicitly allowing a null-type null - so our CI, always ran on little-endian platforms, has always been passing.

I'm the author of supporting residual filter in swiss join as well as the tests. I think allowing null-type null was a mistake (it was implemented at the beginning of my contribution to Arrow - and I was very naive then).

I think I can send out an issue/pr soon to address the issues discovered here:

1. The test in question needs to be revised by replacing null-type null with boolean null, as mentioned in my previous comment (so it will pass on big-endian platforms);
2. The swiss join code need to be updated by not allowing null-type.
3. Add DCHECKs to enforce the filter expression checking. (optional)

[zanmato1984]

I filed #48268 to address this problem and I'll work on a PR very soon. Once it's fixed, there shouldn't be failures of this test on big-endian platforms. So as I mentioned before, this part of change is not need (and not quite correct). Thanks.

[zanmato1984]

Should be fixed by #48272 . @Vishwanatha-HD Could you first revert the change to hash_join.cc then rebase main to see if the hash join test is passing on big-endian? Thanks.

[Vishwanatha-HD]

@zanmato1984.. I tried reverting my changes done to hash_join.cc.. With the latest main, the testcase is passing on the big-endian machine..

[zanmato1984]

That's great, thanks for the update!

[Vishwanatha-HD]

Could you fix lint failure?

I have fixed the lint failures. Thanks..

[Vishwanatha-HD]

I have taken care of the review comments and made necessary changes to the code. Pls review my changes. Thanks..

[kiszk]

Could you remove [Parquet] from the title? I think that this is not a Parquet-specific issue.

[pitrou]

Er, ok, but there is a well-defined explanation to this: a Arrow schema has an endianness attached to it, and it defaults to the platform's endianness.

To obey the intent of this test case, you should instead change the schema instantiation above to:

auto schema = ::arrow::schema({f0}, Endianness::Little, metadata);

Does this change solve the issue on s390x?

[Vishwanatha-HD]

@pitrou.. Thanks for your review comments. But this change didnt quite resolve the issue. Thanks..

[pitrou]

@Vishwanatha-HD You're right, but that should be fixed once #48239 is merged. So I would recommend waiting for that.

[Vishwanatha-HD]

@pitrou.. With the #48239 getting merged into main, I tried making the changes to schema as you suggested above, but the testcase is not passing on s390x..

./debug/arrow-ipc-message-internal-test --gtest_filter="TestMessageInternal.TestByteIdentical"
Running main() from gmock_main.cc
Note: Google Test filter = TestMessageInternal.TestByteIdentical
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from TestMessageInternal
[ RUN ] TestMessageInternal.TestByteIdentical
arrow/cpp/src/arrow/testing/gtest_util.cc:241: Failure
Expected equality of these values:
buffer.size()
Which is: 228
expected.size()
Which is: 232
Mismatching buffer size

[ FAILED ] TestMessageInternal.TestByteIdentical (1 ms)
[----------] 1 test from TestMessageInternal (1 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test suite ran. (1 ms total)
[ PASSED ] 0 tests.
[ FAILED ] 1 test, listed below:
[ FAILED ] TestMessageInternal.TestByteIdentical

1 FAILED TEST

[pitrou]

@Vishwanatha-HD You must also undo your own changes.

[pitrou]

I think it's enough to only test Endianness::Little. Roundtripping is tested elsewhere.

[Vishwanatha-HD]

Ok.. sure.. I will make the necessary changes and push the code again.. Thanks for your comments @pitrou.. !!

[Vishwanatha-HD]

@pitrou, as per our discussion yesterday, I have made the necessary changes to message_internal_test.cc file to only test Endianness::Little.. I have reverted the code that I added for BE architecture. Thanks..

[pitrou]

@Vishwanatha-HD Are you sure you pushed your changes? I see lots of CI failures.

[Vishwanatha-HD]

@pitrou.. Apologies.. It was my bad that I had both Endianness in the for loop.. I have made the necessary code changes and pushed the changes again.. Thanks for pointing this out.. !!

[pitrou]

Also see #48239

[Vishwanatha-HD]

Could you remove [Parquet] from the title? I think that this is not a Parquet-specific issue.

Hi @kiszk.. My idea of having the Parquet is that the overall changes are intended for Parquet component and in order to enable support to Parquet DB on s390x..

[kiszk]

Could you remove [Parquet] from the title? I think that this is not a Parquet-specific issue.

Hi @kiszk.. My idea of having the Parquet is that the overall changes are intended for Parquet component and in order to enable support to Parquet DB on s390x..

I understand your idea. On the other hand, this test case is not for Parquet. The current title may confuse others.

[Vishwanatha-HD]

I have addressed all the review comments. Thanks.

[pitrou]

Hey @Vishwanatha-HD I have two requests:

1. can you rebase/merge the latest git main into this?
2. Please don't mark discussions as resolved as soon as you have answered them; they should only be marked resolved if there is common agreement that there's nothing to add

[Vishwanatha-HD]

Hey @Vishwanatha-HD I have two requests:

1. can you rebase/merge the latest git main into this?
2. Please don't mark discussions as resolved as soon as you have answered them; they should only be marked resolved if there is common agreement that there's nothing to add

Hi @pitrou.. Sure.. I will do the rebase with the latest git main and get the latest changes into this..
Also, I am sorry for marking the discussions resolved, as I thought I should close it as soon as I work on it & resolve it..

[Vishwanatha-HD]

Hey @Vishwanatha-HD I have two requests:

1. can you rebase/merge the latest git main into this?
2. Please don't mark discussions as resolved as soon as you have answered them; they should only be marked resolved if there is common agreement that there's nothing to add

Hi @pitrou.. Sure.. I will do the rebase with the latest git main and get the latest changes into this.. Also, I am sorry for marking the discussions resolved, as I thought I should close it as soon as I work on it & resolve it..

@pitrou.. I did the git rebase to merge the latest git main changes into this..

[Vishwanatha-HD]

With the changes done as part of #48272, the latest main works fine without the hash_join.cc changes on big-endian architecture.. Hence reverting the changes done as part of this PR..

[pitrou]

Also, please fix the PR title and description to match the PR contents.

[Vishwanatha-HD]

Also, please fix the PR title and description to match the PR contents.

@pitrou.. I have fixed the PR title and the description to match the PR contents as you rightly pointed out.. Thanks for that.. !!

[pitrou]

LGTM, I'll just wait for CI before merging

[conbench-apache-arrow]

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

There were no benchmark performance regressions. 🎉

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