GH-48846: [C++] Read message metadata and body in one go in IPC file reader by abhishek593 · Pull Request #48975 · apache/arrow

abhishek593 · 2026-01-24T19:59:49Z

Rationale for this change

ReadMessageAsync takes a body_length parameter and reads Message metadata + body in one go, but the blocking version ReadMessage reads the body length from the Message and issues a second read for the body. This PR adds a ReadMessage overload that takes the body length as parameter and does a single read like the async version does. This reduces the number of IOs issued by the IPC file reader.

What changes are included in this PR?

Add ReadMessage overload accepting body_length
Update IPC file reader to use the new ReadMessage overload when reading full record batches

Are these changes tested?

Yes, added TestReadMessage.ReadBodyWithLength and updated other tests to use the new overload.

Are there any user-facing changes?

No.

GitHub Issue: [C++][IPC] ReadMessage could optionally take body_length #48846

github-actions · 2026-01-24T20:00:12Z

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

pitrou

Thank you for the submission @abhishek593. This looks good generally but I've made a number of suggestions below, feel free to update your PR when you have some time!

pitrou · 2026-02-16T10:33:39Z

    const int64_t offset, const int32_t metadata_length, io::RandomAccessFile* file,
    const FieldsLoaderFunction& fields_loader = {});

+/// \brief Read encapsulated RPC message from position in file


Ok, I think "RPC" is a typo, it should be "IPC", could you perhaps fix all occurrences in this file?

pitrou · 2026-02-16T10:41:50Z

-  // there are 3 read IOs before reading body:
-  // 1) read magic and footer length IO
-  // 2) read footer IO
-  // 3) read record batch metadata IO
-  EXPECT_EQ(read_ranges.size(), 3 + expected_body_read_lengths.size());


Ok, can we keep a less strict assertion to check that we can at least access the two first elements without segfaulting?
Something like:

// there are at least 2 read IOs before reading body: // 1) read magic and footer length IO // 2) read footer IO EXPECT_GE(read_ranges.size(), 2);

pitrou · 2026-02-16T10:53:31Z

+  // there are 3 read IOs before reading body:
+  // 1) read magic and footer length IO
+  // 2) read footer IO
+  // 3) read record batch metadata IO


This comment only matches one of the two code paths. Perhaps fix it or use a separate comment per code path?

pitrou · 2026-02-16T10:54:43Z

+    int64_t total_body = 0;
+    for (auto len : expected_body_read_lengths) total_body += len;
+
+    EXPECT_GT(read_ranges[2].length, total_body);


Add a small comment explaining why? Also can take inspiration from the other code path and add:

EXPECT_LE(read_ranges[2].length, total_body + footer_length);

…in one go

…dy slicing

… helper and fix documentation

pitrou

Looking excellent now, thank you @abhishek593 . I'm just going to push a minor cosmetic change and we'll be able to merge if CI passes.

pitrou · 2026-02-17T14:04:34Z

This is now merged, thanks again @abhishek593 :)

conbench-apache-arrow · 2026-02-18T19:05:35Z

After merging your PR, Conbench analyzed the 2 benchmarking runs that have been run so far on merge-commit 7c45228.

There were no benchmark performance regressions. 🎉

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

conbench-apache-arrow · 2026-02-21T02:04:59Z

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

There were no benchmark performance regressions. 🎉

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

… file reader (apache#48975) ### Rationale for this change ReadMessageAsync takes a body_length parameter and reads Message metadata + body in one go, but the blocking version ReadMessage reads the body length from the Message and issues a second read for the body. This PR adds a ReadMessage overload that takes the body length as parameter and does a single read like the async version does. This reduces the number of IOs issued by the IPC file reader. ### What changes are included in this PR? 1. Add ReadMessage overload accepting body_length 2. Update IPC file reader to use the new ReadMessage overload when reading full record batches ### Are these changes tested? Yes, added TestReadMessage.ReadBodyWithLength and updated other tests to use the new overload. ### Are there any user-facing changes? No. * GitHub Issue: apache#48846 Lead-authored-by: Abhishek Bansal <abhibansal593@gmail.com> Co-authored-by: Antoine Pitrou <antoine@python.org> Signed-off-by: Antoine Pitrou <antoine@python.org>

github-actions Bot added Component: C++ awaiting review Awaiting review labels Jan 24, 2026

pitrou requested changes Feb 16, 2026

View reviewed changes

github-actions Bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Feb 16, 2026

abhishek593 force-pushed the feature/ipc-read-message-optimization branch from 10990b2 to cc7dd86 Compare February 16, 2026 17:00

abhishek593 requested a review from pitrou February 16, 2026 20:27

abhishek593 and others added 5 commits February 17, 2026 12:21

apacheGH-48846: [C++] Optimize ReadMessage to read metadata and body …

da24cfc

…in one go

apacheGH-48846: [C++][IPC] Fix heap-buffer-overflow in ReadMessage bo…

a0e7d11

…dy slicing

chore: trigger CI

e1058fc

apacheGH-48846: [C++][IPC] Refactor ReadMessage overloads with common…

d78c383

… helper and fix documentation

Cosmetic style changes

ff6a9e5

pitrou approved these changes Feb 17, 2026

View reviewed changes

pitrou force-pushed the feature/ipc-read-message-optimization branch from cc7dd86 to ff6a9e5 Compare February 17, 2026 11:23

pitrou changed the title ~~GH-48846: [C++] Optimize ReadMessage to read metadata and body in one go~~ GH-48846: [C++] Read message metadata and body in one go in IPC file reader Feb 17, 2026

pitrou merged commit 7c45228 into apache:main Feb 17, 2026
55 of 57 checks passed

pitrou removed the awaiting committer review Awaiting committer review label Feb 17, 2026

pitrou mentioned this pull request Feb 17, 2026

[C++][IPC] ReadMessage could optionally take body_length #48846

Closed

Uh oh!

Conversation

abhishek593 commented Jan 24, 2026 • edited by pitrou Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

github-actions Bot commented Jan 24, 2026

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pitrou commented Feb 17, 2026

Uh oh!

conbench-apache-arrow Bot commented Feb 18, 2026

Uh oh!

conbench-apache-arrow Bot commented Feb 21, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

abhishek593 commented Jan 24, 2026 •

edited by pitrou

Loading