GH-32613: [C++] Simplify IPC writer for dense unions

[rtadepalli]

JIRA: https://issues.apache.org/jira/browse/ARROW-17339
Closes: #32613

Rationale for this change

Dense union offsets are always non-strictly monotonic for any given child as mandated by the spec, The C++ implementation still assumes that the offsets may be in any order. This can be improved.

What changes are included in this PR?

Just a change to eliminate looping over the size of a DenseUnionArray twice.

Are these changes tested?

I am not functionally changing anything. All changes respect the spec, and behavior should be 1:1 with the existing implementation. I believe existing tests should suffice.

Are there any user-facing changes?

There are no user facing changes for this.

• Closes: [C++] Simplify IPC writer for dense unions #32613

[github-actions]

Thanks for opening a pull request!

If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose

Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project.

Then could you also rename the pull request title in the following format?

GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

or

MINOR: [${COMPONENT}] ${SUMMARY}

In the case of PARQUET issues on JIRA the title also supports:

PARQUET-${JIRA_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

See also:

• Other pull requests
• Contribution Guidelines - How to contribute patches

[github-actions]

• Closes: [C++] Simplify IPC writer for dense unions #32613

[github-actions]

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

[rtadepalli]

Hello @kou, I have fixed the broken clang-format test. Thanks.

[kou]

@github-actions crossbow submit -g cpp

[github-actions]

Revision: d787277

Submitted crossbow builds: ursacomputing/crossbow @ actions-f892614f66

Task	Status
test-alpine-linux-cpp
test-build-cpp-fuzz
test-conda-cpp
test-conda-cpp-valgrind
test-cuda-cpp
test-debian-10-cpp-amd64
test-debian-10-cpp-i386
test-debian-11-cpp-amd64
test-debian-11-cpp-i386
test-fedora-35-cpp
test-ubuntu-18.04-cpp
test-ubuntu-18.04-cpp-release
test-ubuntu-18.04-cpp-static
test-ubuntu-20.04-cpp
test-ubuntu-20.04-cpp-20
test-ubuntu-20.04-cpp-bundled
test-ubuntu-20.04-cpp-thread-sanitizer
test-ubuntu-22.04-cpp

[kou]

Do existing tests cover this case?

[rtadepalli]

I think they should. I was looking at the implementation in Go as a reference, and it doesn't seem like the PR that introduced the change (#13806) added any additional tests for finding the offset. I am of the idea right now that existing tests should break if this change is wrong. Please let me know if this is not true, and a unit test is warranted.

[kou]

If you add a bug in the target logic and there are tests for this case, these existing tests will be failed.
Could you try it? Or do you want me to do it?

[rtadepalli]

But won't those tests run as part of the CI pipeline?

I can definitely try to run them, but would need some help 😅. Would running all tests in the ipc/ package be sufficient?

[kou]

You can just push a commit that includes a bug to this branch.
Then CI runs all existing tests with the buggy code. If there are tests that cover this case, they will be failed.

[rtadepalli]

Hello @kou , I have introduced a bug in the code. I added a +1 to the array index while accessing shifted offsets.

[rtadepalli]

Looks like arrow-ipc-read-write-test is failing with my bug 👍

I will revert the bug if the test results seem satisfactory to you.

[kou]

Thanks.
It seems that we have a test for this case.

Could you revert the bug?

[rtadepalli]

I have reverted the three commits in which I pushed bugs.

[kou]

+1

[ursabot]

Benchmark runs are scheduled for baseline = 3ab246f and contender = b413ac4. b413ac4 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed ⬇️0.55% ⬆️0.03%] test-mac-arm
[Finished ⬇️2.04% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.03% ⬆️0.0%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] b413ac4f ec2-t3-xlarge-us-east-2
[Failed] b413ac4f test-mac-arm
[Finished] b413ac4f ursa-i9-9960x
[Finished] b413ac4f ursa-thinkcentre-m75q
[Finished] 3ab246f3 ec2-t3-xlarge-us-east-2
[Finished] 3ab246f3 test-mac-arm
[Finished] 3ab246f3 ursa-i9-9960x
[Finished] 3ab246f3 ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

[ursabot]

['Python', 'R'] benchmarks have high level of regressions.
ursa-i9-9960x